-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
support for system as actor #4677
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 841d26c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
packages/core/src/system.ts
Outdated
@@ -91,6 +106,8 @@ export interface ActorSystem<T extends ActorSystemInfo> { | |||
|
|||
export type AnyActorSystem = ActorSystem<any>; | |||
|
|||
const rootSubscriptionKey = '@xstate.system.root' as const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a root subscription key that seemed unique enough while following some conventions I found.
This would be used in the case actor.system.subscribe(event => ...)
was used with no systemId
.
packages/core/src/system.ts
Outdated
|
||
export interface ActorRegisteredEvent< | ||
TActorRef extends AnyActorRef, | ||
TSystemId extends string = string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This made sense to me if there were any plans to allow for extracting systemId
types from a machine.
Let me if it doesn't!
packages/core/src/system.ts
Outdated
let listener = | ||
typeof maybeSystemIdOrListener === 'function' | ||
? maybeSystemIdOrListener | ||
: maybeListener!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this is okay, typescript would prevent this but there's no runtime check
packages/core/src/system.ts
Outdated
? rootSubscriptionKey | ||
: maybeSystemIdOrListener; | ||
|
||
let subscriptions = systemSubscriptions.get(systemId) || new Set(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could also design this differently by using a single set, and overriding the listeners if a systemId key is present. This would also mean passing the systemId directly in the event so its easier to parse
something like:
let listener =
typeof maybeSystemIdOrListener === 'function' ?
maybeSystemIdOrListener
:
(event) => {
if (event.systemId === maybeSystemIdOrListener) maybeListener(event)
}
what do you think is preferable here?
…, modify listener when systemId present in subscribe to check for systemId
packages/core/src/system.ts
Outdated
completeListener | ||
); | ||
|
||
if (rootActor._processingStatus !== ProcessingStatus.Stopped) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied the implementation in createActor
. I think it would make sense to keep this since system whose root actor is stopped won't be receiving updates, right?
packages/core/src/system.ts
Outdated
reportUnhandledError(err); | ||
} | ||
break; | ||
// can this error? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would an error look like here?
Disregard the resolved comments, did a refactoring to make it use the same logic as |
Please include some motivation behind this feature in the PR's description. |
@Andarist Oops! Back to you |
The motivation mentions that today it's tedious to access such actors in React but it doesn't show how this PR solves that. Could you include before/after comparison? |
@Andarist You're right. Updated it with some examples! |
I think this particular design doesn't fit XState well. When you subscribe to an actor you are subscribed to its snapshot values but here, when subscribing to the system, you are subscribed to events. A proposal that would bring this closer to how things work with actors would fit XState better. |
@Andarist Fair enough. What are you thinking would be closer? There is What do you think? |
The proposed implementation of Currently const notifier = useSelector(actorRef.system, system => system.get('notifier')) On the other hand, the system is mutable today and that doesn't play with Could you write down how you want to use this in your app? What's the rough sketch of the architecture that you have there and why do you find yourself in a position of reaching for an actor that might not exist at a time etc. |
@Andarist I think this problem manifests itself only if you orchestrate the entirety of your application state in XState. The best example I can give is one I face currently: App has three states: Cover | Menu | Checkout Cover has a bunch of child states Menu has a couple of child states as well And each child state can potentially have its own child state as well. These states are all machines that are instantiated by its parent machine using The only missing piece to |
I think option 3 would also be to have each child state just be part of the root machine rather than a separate machine that is invoked. This comes with added complexity in types, which tends to be a deal breaker as the app scales. |
@Andarist What would you say the root issue is? Am I just holding xstate wrong, and should rather let React do the instantiations? |
I think I stand by what I described in: #4677 (comment) . To improve the situation here we'd have to actor-ify the system further. Its current snapshot should be cached, each change in the registered actors should update it (in an immutable manner) and notify the subscribers about the change. This way you should be able to get this working: -const notifier = useSelector(actorRef.system, system => system.get('notifier'))
+const notifier = useSelector(actorRef.system, s => s.actors.notifier) |
@Andarist Sounds good to me. What would a snapshot look like in this case? Would we move |
Also by making this an actor, do you mean to hook the system into the rootActor's Currently looking at |
Not necessarily. I'd like to keep the snapshot as lean as possible so it should only be extended with
No. I'm sorry for not being explicit enough. I meant that its implementation should get closer to an actor - not necessarily that it should literally use |
b80c647
to
afb5807
Compare
@Andarist back to you! There are better tests that can be written, if you could give me some test cases you'd like to see I can get that done. One question I have is: what would an |
Anything I can do to get this past the finish line? |
Just a question - in |
unfortunately, not yet. the system is still untyped. you'd have to annotate it manually in the selector or in the function |
@cevr thank you for taking this on, being able to access deeply nested actors from anywhere in the system/UI was one of the biggest struggles I've had with building our app in XState (v4 mind you). |
Anything I can do to improve this pr? @Andarist @davidkpiano |
This adds support for subscribing to an actor's system.
Motivation
XState is great for orchestrating complex changes within an app. This can eventually lead to having many levels of nested actors within a system.
The problem comes when trying to access an actor that is many levels deep. Things like
actor.children.a.children.b.children.c
become common, and that isn't accounting for ensuring that each child is present and latest. In React, this becomes quite tedious.Fortunately, this is solved through using the root actor's
system
, since it's a flat registry of all actors within its scope (granted that unique systemIds are provided). The above instead becomesactor.system.get('c')
.Unfortunately, there is no way to know when an actor has registered or unregistered within the system in a straightforward manner.
This PR seeks to fix that by adding support for subscribing to a system, providing updates on register/unregister events.
Here's an example of how this could be used to improve the React usecase:
before:
You could change the above to use empty actors if it's not defined, but it doesn't change the fact that you need to use 3 selectors to get the child
c
. Which is a lot syncing. Also, it may not be desirable if you relystate.value
for some states rather thanstate.matches
.With the changes in this pr, an actor's system becomes an actor meaning you can pass it into
useSelector
.and then rewrite the
useCActorRef
to look like this:The benefit here is that there's only one subscription and it is easier to reason about.