Fixed an issue with targeted ancestor reentrancy

[Andarist]

fixes #3409

[changeset-bot]

🦋 Changeset detected

Latest commit: 4a300b6

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

This PR includes changesets to release 1 package

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

[ghost]

👇 Click on the image for a new way to code review

• Make big changes easier — review code in small groups of related files

• Know where to start — see the whole change at a glance

• Take a code tour — explore the change with an interactive tour

• Make comments and review — all fully sync’ed with github

  Try it now!

Legend

[codesandbox-ci]

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 4a300b6:

Sandbox	Source
XState Example Template	Configuration
XState React Template	Configuration
distracted-wilson-nmlkjs	Issue #3409

[Andarist]

Note the change here from allNextStateNodes to nextStateNodes.

nextStateNodes represent the target state nodes literally used in the transition definition whereas allNextStateNodes represent "resolved" targets (initial states etc already resolved). To compute correctly what has to be reentered we need to consider the target states before resolving them.

[Andarist]

somewhat neat trick with comparing those orders here:

• targetNode.order > this.order means that targetNode is either a descendant of this OR it's somewhere outside of it, within the latter siblings of one of its ancestors
• targetNode.order < this.order means that targetNode is either the ancestor of this OR it's somewhere outside of it, within the former siblings of one of its ancestors

Since this function returns [] when those two do not form an ancestor<->descendant relationship we can safely rely on this as we are not even concerned here with finding a common ancestor of the two, we only want to find out if one is the ancestor of the other one and collect all nodes in between if that's the case (including the detected ancestor node).

[davidkpiano]

Very clever! To prevent any edge-cases, we should ensure that order is not configurable (I think it technically is right now)

[Andarist]

I don't think there is currently any risk here because order is a local variable initialized to a static 0 and only assigned within the dfs call here:

xstate/packages/core/src/StateNode.ts

Lines 374 to 385 in e35493f

	// Document order
	let order = 0;
	function dfs(
	stateNode: StateNode<TContext, any, TEvent, any, any, any>
	): void {
	stateNode.order = order++;
	for (const child of getChildren(stateNode)) {
	dfs(child);
	}
	}

[Andarist]

Observation - this sort of uses the parent of the found ancestor as the transition domain which means that we are using LCA algorithm here whereas SCXML is using LCCA. I'm not exactly sure why SCXML has switched to LCCA because I think that they have been using LCA at the beginning. It feels to me that LCA is an easier rule and LCCA is a little bit harder to explain so I wonder what made them switch those algorithms.

[davidkpiano]

Would this break any SCXML tests?

[Andarist]

Using LCA over LCCA? Sure. Using LCCA can lead to some weird behaviors when parallel states are involved though and I'm not sure why those weird behaviors would be desirable in the first place. We should dig into this more to understand the motivation for using LCCA in UML.

[davidkpiano]

The code + test makes sense. We just have to ensure this is working in v5, and maybe even copy the test over.

[Andarist]

We just have to ensure this is working in v5,

I've checked - it already works OK there.

and maybe even copy the test over.

That will just happen automatically~ when we get to merging main to next, so no specific additional actions are required here.