Make it impossible to exit a root state #3487
Conversation
# Conflicts: # packages/core/src/StateNode.ts
# Conflicts: # packages/core/src/StateNode.ts # packages/core/src/types.ts
🦋 Changeset detectedLatest commit: f9a8fde 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 |
|
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 f9a8fde:
|
👇 Click on the image for a new way to code review
Legend |
| @@ -998,25 +1003,6 @@ export function removeConflictingTransitions< | |||
| return Array.from(filteredTransitions); | |||
| } | |||
|
|
|||
| function findLCCA<TContext extends MachineContext, TEvent extends EventObject>( | |||
There was a problem hiding this comment.
I've inlined this logic into getTransitionDomain because it now can return the root state. The LCCA stands for the Least Common Compound Ancestor. We were never limiting ourselves here to return a compound state - so the name was already incorrect. We could have changed it to findLCA where LCA stands for Least Common Ancestor. However, when returning the root state from here it's not always even an ancestor of the input stateNodes (for example, findLCA([root]) would return root and the root is not an ancestor of itself). So I've concluded that it's best to inline this function altogether to avoid confusion and debate.
| current = current.filter((sn) => path.includes(sn)); | ||
| } | ||
|
|
||
| const domain = current[current.length - 1]; |
There was a problem hiding this comment.
We sort of had a bug here previously. We could return undefined from here, whereas the annotated return type is StateNode<TContext, TEvent> | null. Everything was working OK as .parent === domainButActuallyUndefined (and similar) was also a good stop for most of our internal while loops etc.
| marker = marker.parent; | ||
| } | ||
|
|
||
| return path; | ||
| return path.reverse(); |
There was a problem hiding this comment.
Conceptually it's a little bit better to push to an array than to shift it all the time (each shift has to move all the existing items). So I've switched to pushing here but to correct the actual result I need to .reverse() at the end.
| if (!marker) { | ||
| return [stateNode]; | ||
| } |
There was a problem hiding this comment.
By always including the root here we don't allow it to be ever exited.
packages/core/src/stateUtils.ts
Outdated
| @@ -1578,7 +1582,8 @@ export function resolveMicroTransition< | |||
| currentState.context, | |||
| new Set(prevConfig), | |||
| machine, | |||
| _event | |||
| _event, | |||
| new Set(!currentState._initial ? [] : [machine.root]) | |||
There was a problem hiding this comment.
By moving mutStatesToEnter initialization to here we make it easier to add the root as a state that has to be entered during the fake~ initial transition.
The usual expectation is that in a transition similar to this the addAncestorStatesToEnter should add all ancestors of the target state to mutStatesToEnter (if needed, ofc). However, this relies on getProperAncestors and that algorithm doesn't include the toStateNode in its result, so by using this logic alone we were not able to add the root state to the initial mutStatesToEnter and the root's invokes and entry actions rely on being included in this set for that initial transition.
SCXML doesn't account for this case in their algorithms because <scxml/> itself can't have <invoke/>, <onentry/>, etc. We are extending the spec by allowing those on our root state and that raises the need to make adjustments to the algorithms.
There was a problem hiding this comment.
This was going to be my main question - where this differs from the SCXML spec etc. Thanks for this!
| id: 'root', | ||
| invoke: { | ||
| src: () => | ||
| fromPromise(() => { |
There was a problem hiding this comment.
Wondering if it would be better/more explicit to test using entry/exit rather than an invocation.
If for some reason, the implementation changes in the future that invocations are started async, then the test will still pass even though the faulty behavior will be there. (Not saying that it will, but just thinking about potential edge-cases)
There was a problem hiding this comment.
I will add a test for entry/exit actions too 👍 Until we change how invocations are handled it's good to have the existing test in place too.
Co-authored-by: David Khourshid <davidkpiano@gmail.com>
# Conflicts: # packages/core/src/stateUtils.ts
closes #3072