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

Ensure exit actions are called for finalized invoked machines. Fixes #1109 #1566

Merged
merged 4 commits into from Oct 19, 2020

Conversation

davidkpiano
Copy link
Member

This PR fixes #1109 by executing exit actions when an invoked machine reaches its final state.

@changeset-bot
Copy link

changeset-bot bot commented Oct 18, 2020

🦋 Changeset detected

Latest commit: dd591ae

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

This PR includes changesets to release 3 packages
Name Type
xstate Patch

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

@davidkpiano davidkpiano changed the title @davidkpiano Ensure exit actions are called for finalized invoked machines. Fixes #1109 Ensure exit actions are called for finalized invoked machines. Fixes #1109 Oct 18, 2020
@@ -275,6 +275,13 @@ export class Interpreter<
const isDone = isInFinalState(state.configuration || [], this.machine);

if (this.state.configuration && isDone) {
// exit interpreter procedure: https://www.w3.org/TR/scxml/#exitInterpreter
this.state.configuration.forEach((stateNode) => {
Copy link
Member

Choose a reason for hiding this comment

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

Those exit handlers should be executed in "exit order". I'm not sure if this.state.configuration holds any guarantee about the order of items in it. It would actually be quite cool to rely on it without having to sort stuff here, I'm just not sure if we can safely make such an assumption now.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the next branch, I believe these are sorted by exit order (I think there are some SCXML tests ensuring this). I'll see what I can do here to have the same behavior.

Copy link
Member

Choose a reason for hiding this comment

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

The only SCXML I could find regarding this is: https://www.w3.org/Voice/2013/scxml-irp/250/test250.txml and it's classified as manual (it's one of the tests that requires the tester to look at the log output)

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 better suited for the next branch, since that more faithfully follows the SCXML algorithm. There would probably need to be substantial changes in order to ensure exit action order; let's scope this PR to just ensuring that they are called.

davidkpiano and others added 2 commits October 18, 2020 23:02
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
@davidkpiano davidkpiano merged commit ebd91ce into master Oct 19, 2020
@davidkpiano davidkpiano deleted the davidkpiano/1109 branch October 19, 2020 18:13
This was referenced Oct 19, 2020
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.

exit action is not triggered on child machine
2 participants