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

Add xstate.stop event and fix exit handlers execution #3126

Merged
merged 9 commits into from
Aug 2, 2022

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented Mar 7, 2022

TODO:

  • changesets
  • establish how we rollout this, especially in consideration with the typegen (cc @mattpocock )
  • discuss the minor TODO comments left in the code

fixes #2880
fixes #2450
fixes #2057
fixes #3053
fixes #3221

cc @huan @ptaranto @rlaffers @Jimmy89 @erikras @AvivLocalize @AntonS86

@changeset-bot
Copy link

changeset-bot bot commented Mar 7, 2022

🦋 Changeset detected

Latest commit: ea02bb8

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 Mar 7, 2022

CodeSee Review Map:

Review these changes using an interactive CodeSee Map

Review in an interactive map

View more CodeSee Maps

Legend

CodeSee Map Legend

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 7, 2022

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.

Latest deployment of this branch, based on commit ea02bb8:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration
xstate-typescript-template (forked) Issue #2057
elated-lalande-9u8ws7 Issue #2450
patient-dust-g93dp2 Issue #2880
xstate-stop-parent-child-grandchild Issue #3053
jovial-golick-qtjvz2 Issue #3221

);

const newState = new State<TContext, TEvent, TStateSchema, TTypestate>({
value: this.state.value,
Copy link
Member Author

Choose a reason for hiding this comment

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

Initially, I've used null here but it has failed on some logic within XState so I've tried {} but that also has broken some assumptions about final states. So I've ended up reusing the current state.value. Which in turn means that I probably should do the same for state.tags and perhaps for some other stuff too, maybe state.historyValue shouldn't be cleared either?

This gets somewhat weird because we clear state.configuration, as required by the exitInterpreter procedure. Maybe we shouldn't do that though? It feels weird that state.value and state.configuration don't reflect each other.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can deviate from SCXML here, since we assume the final state to be the state before exiting all states of the machine completely, and having a state of "nothingness" (empty configuration, null state value) is not very practical, especially since state.done === true implicitly gives us that information already.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've adjusted the implementation slightly and now state.done is equivalent to isInFinalState(state) - so a stopped machine doesn't end up with state.done === true

@huan
Copy link
Contributor

huan commented Mar 8, 2022

Thanks for creating this PR to fix the issue, looking forward to using the merged new version, and also the v5 as well! 😄

history: this.state,
// this filtering is somewhat questionable and we might reconsider this in the future
// we definitely don't want to execute raised events as the stop event is the last thing that should be processed by this machine
// the question is if we should allow sending events from here to other actors *while* preventing the parent that has stopped this machine from receiving an event from here
Copy link
Member

Choose a reason for hiding this comment

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

I think we should allow events to be sent to other machines from here.

If the recipient is the parent, it should ignore all events after stopping (I think):

  1. Parent starts "stop procedure" (🛑 no events can be received from this point)
  2. Child receives xstate.stop event (🛑 no events can be received from this point)
  3. Final actions are executed, which may include sending to parent
  • Any event sent to parent as an action is ignored by parent (child doesn't handle this logic)
  1. Procedure finished; machines terminated

Copy link
Member Author

Choose a reason for hiding this comment

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

Any event sent to parent as an action is ignored by parent (child doesn't handle this logic)

Right, this is somewhat hard to handle right now though - I will take another look at this to see if I can make it happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've managed to cover this use case, however, the implementation is slightly different. I'm filtering what is being sent out from a done/stopped machine (where the original suggestion was to reject the events on the receiver's end). I couldn't find a clear way in the parent to check if the received event was sent by one of its children. I don't think this makes any practical difference though

@@ -1018,6 +1080,9 @@ export class Interpreter<
name: string,
options?: SpawnOptions
): ActorRef<any> {
if (this.status !== InterpreterStatus.Running) {
Copy link
Member

@davidkpiano davidkpiano Mar 8, 2022

Choose a reason for hiding this comment

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

This also should not do anything if the status is InterpreterStatus.Stopped

Also, when will this occur (spawning before the interpreter is started) relevant to this issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Within this PR it's not as much about spawning before the interpreter is started but rather about spawning from within exit action where the interpreter is already stopped.

interpreter.send({ type: 'STOP_CHILD' });
});

it('actors spawned in exit handlers of a stopped child should not be started', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Likewise, this test should still pass by virtue of not spawning any actor after being stopped, rather than spawning a deferred actor

Copy link
Member Author

Choose a reason for hiding this comment

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

This would be problematic because some actions might refer to the "spawned" actor, like for example here:

exit: [
  assign({
    childRef: () => spawn(child)
  }),
  send({ type: 'GRAB_SOME_DATA_FROM_ME' }, { to: ctx => ctx.childRef })
]

So we need to create something within this spawn call but also we shouldn't really spawn a real child. This also brings up the fact that we should introduce a warning like this to more places:

const isLazyEntity = isMachine(entity) || isFunction(entity);
warn(
!!service || isLazyEntity,
`Attempted to spawn an Actor (ID: "${
isMachine(entity) ? entity.id : 'undefined'
}") outside of a service. This will have no effect.`
);

Ideally, we'd always receive "lazy" entities from users within assign. This doesn't hold true right now and we allow eager entities but we should probably require all of them to be lazy. To avoid breaking changes we still need to process those eager entities but that should be accompanied by a dev-only warning. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's add a dev-only warning.

In v5 (the spawn improvements branch), this is fixed by making all actors "not started" at first, and then starting them with their given behavior, while using the exact same reference. All actors are lazy.

Copy link
Member

@davidkpiano davidkpiano left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just some comments/questions for clarification

@Andarist
Copy link
Member Author

@davidkpiano it's ready for the re-review. I've addressed the important part of the feedback and gonna add a changeset tomorrow. Let me know if you think that there is anything else blocking this PR.

Copy link
Member

@davidkpiano davidkpiano left a comment

Choose a reason for hiding this comment

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

No further blockers, looks good to me

@@ -0,0 +1,7 @@
---
'xstate': minor
Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a changeset here - @davidkpiano maybe you could review the wording and the level of the detail included here

Copy link
Member

Choose a reason for hiding this comment

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

Some nits, LGTM

@@ -0,0 +1,7 @@
---
'xstate': minor
Copy link
Member

Choose a reason for hiding this comment

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

Some nits, LGTM

Andarist and others added 2 commits April 15, 2022 19:20
Co-authored-by: David Khourshid <davidkpiano@gmail.com>
@davidkpiano
Copy link
Member

@Andarist Can we merge this?

@Andarist
Copy link
Member Author

Andarist commented May 1, 2022

@davidkpiano just need to adjust the logic in how we generate typegen metadata so both could be released in tandem

@Jimmy89
Copy link

Jimmy89 commented Jun 14, 2022

@Andarist is there any ETA about when this change is being released?

@Andarist
Copy link
Member Author

@Jimmy89 Next weeks seems to be realistic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment