-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[core] Fix restarted root-level invocations #3073
Conversation
🦋 Changeset detectedLatest commit: 9e1d615 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
CodeSee Review Map:Review in an interactive map View more CodeSee Maps Legend |
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 9e1d615:
|
const reentryNodes: AnyStateNode[] = []; | ||
|
||
if (!isInternal) { | ||
allNextStateNodes.forEach((n1) => { |
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 is the actual change
d52dcb5
to
6a385f4
Compare
I've taken a look at this from the SCXML perspective and I think that this is a good change. Let's start first with how this compares to the spec text. We can read that It's also worth noting that
This clearly means that we can't "exit" the root "state". The difference between SCXML and XState though is that in XState we actually extend what the root ( Ok, so I think we are in the agreement that this is something that we'd like to do. However, I wonder if we really should do this in v4. The current behavior was with us here for quite a long time and it's also not a deal-breaker, it's just something that we'd like to change - but by no means this is a pressing matter. I know that we've defined the semver policy about breaking changes so we have some freedom to change this now. That doesn't mean though that we should introduce such changes lightly. So my question is - is the RoI of this change sufficiently high that we are OK with this "breaking change" (cause it definitely might be for some state machines out there, it's hardly an edge case)? Isn't this something that we should just change in v5? |
@@ -2905,6 +2905,41 @@ describe('invoke', () => { | |||
done(); | |||
}, 0); | |||
}); | |||
|
|||
// https://github.com/statelyai/xstate/issues/3072 | |||
it('root invocations should not restart on external transitions', (done) => { |
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.
We should also add a separate test that would test, in a more explicit way, that the root state is never being reentered here. That invoke
's implementation is piggybacking on entry/exit actions is an implementation detail so I don't think that we should assume that this is also testing that more "basic" scenario.
: flatten(allNextStateNodes.map((n) => this.nodesFromChild(n))); | ||
const reentryNodes: AnyStateNode[] = []; | ||
|
||
if (!isInternal) { |
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.
Note that this algorithm is also broken in other subtle ways. States are not only reentered for external transitions - internal/external only change what states are potentially reentered.
For instance, on this codesandbox this entry action should technically be called twice. Even though the transition is internal, this target state should be reentered.
And I'm not saying that we should fix this right now. I'm just observing the fact. I somewhat don't see how fixing non-critical bugs in v4 gets us closer to anything. As mentioned before - I would rather halt the v4 development (just fix pressing issues) and focus on v5 (or on the "satellite" packages, as that development is mostly orthogonal)
--- | ||
'xstate': patch | ||
--- | ||
|
||
The `AnyStateNode` type, which represents a `StateNode` of any context, event, etc., has been introduced. |
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 is a somewhat internal change - I'm not sure how relevant this is for our users and I would probably skip "documenting" it:
--- | |
'xstate': patch | |
--- | |
The `AnyStateNode` type, which represents a `StateNode` of any context, event, etc., has been introduced. | |
--- | |
--- |
6a385f4
to
429eaeb
Compare
@davidkpiano I've started merging this into the I've verified that it was passing accidentally and I've recognized 2 issues:
I'm going to tackle both and then merge this PR to the next branch and fix the linked ticket there. |
I've created #3487 as a successor PR to this one. The new one is targeting the |
External root-level transitions (which should be invalid – only internal root-level transitions are allowed) will no longer restart root-level invocations. See #3072 for more details.
Fixes #3072