Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added interop observable symbols to ActorRef #2835

Merged

Conversation

woutermont
Copy link
Contributor

@woutermont woutermont commented Nov 25, 2021

Closes #2834

Signed-off-by: Wouter Termont <woutermont@gmail.com>
@changeset-bot
Copy link

changeset-bot bot commented Nov 25, 2021

🦋 Changeset detected

Latest commit: 444553c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
xstate Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ghost
Copy link

ghost commented Nov 25, 2021

CodeSee Review Map:

Review these changes using an interactive CodeSee Map

Review in an interactive map

View more CodeSee Maps

Legend

CodeSee Map Legend

woutermont and others added 4 commits November 25, 2021 18:03
Co-authored-by: David Khourshid <davidkpiano@gmail.com>
Signed-off-by: Wouter Termont <woutermont@gmail.com>
Signed-off-by: Wouter Termont <woutermont@gmail.com>
@woutermont woutermont changed the title Added interop observable symbols to ActorRef and Subscribable Added interop observable symbols to ActorRef Nov 25, 2021
@woutermont
Copy link
Contributor Author

Making Subscribable be an InteropObservable is obviously not the way to go, so I reverted that part, simply added the InteropObservable from RxJS and let only ActorRef extend it.

@davidkpiano
Copy link
Member

@Andarist LGTY?

packages/core/src/types.ts Outdated Show resolved Hide resolved
Comment on lines +539 to +540
// this gets stripped by Babel to avoid having "undefined" property in environments without this non-standard Symbol
// it has to be here to be included in the generated .d.ts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you confirmed that this works? is the Symbol.observable emitted in the .d.ts? is this property in the dist files from here by Babel?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, only [symbolObservable] is used in the ES file, and [Symbol.observable] is present in the type declarations.

toSCXMLEvent,
toEventObject,
toObserver,
interopSymbols
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be compatibility issues between mismatched versions of xstate and @xstate/inspect - I would just touch the core directory as part of this PR and revert the changes here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you suggest to do this? InspectReceiver of @xstate/inspect is an alias for an ActorRef, which is impacted by these changes in the core.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can revise these in a follow-up PR.

Copy link
Member

@Andarist Andarist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if we could add some tests for this.

woutermont and others added 4 commits December 8, 2021 18:48
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Signed-off-by: Wouter Termont <woutermont@gmail.com>
@woutermont
Copy link
Contributor Author

That would indeed be great, but the only thing this PR does is adding an interface, and making an existing interface extend from it (with some sugar to implement it) ... where/how should I test this?

@woutermont
Copy link
Contributor Author

@Andarist, can you confirm my changes and comments are satisfactory, or reply once more to them?

@davidkpiano
Copy link
Member

Confirmed to be working. Here is without the change:

CleanShot 2021-12-30 at 18 36 28

And here is with this PR:

CleanShot 2021-12-30 at 18 35 35

@Andarist
Copy link
Member

Andarist commented Feb 18, 2022

@woutermont are you using this feature? I'm wondering how you are using this at runtime because I have a suspicion that this might not work correctly for all the cases. So I wonder - for which exact cases is this working for you (I can think of one).

EDIT:// actually, after further analysis I think that it's somewhat the other way around. I've found a single place for which this wasn't working quite OK.

@woutermont
Copy link
Contributor Author

We're using this to combine XState ActorRefs (specifically the Interpreter, I believe) with RxLitElement. I'm currently on holiday, but can provide more info when back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let ActorRef be an RxJS InteropObservable
3 participants