Fixed an issue with exit actions being called in random order when stopping a machine

[Andarist]

No description provided.

[changeset-bot]

🦋 Changeset detected

Latest commit: 3352231

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

[Andarist]

I've just tweaked this as usually we don't call those 'handlers" in the docs so this could be confusing for people

[ghost]

CodeSee Review Map:

Review in an interactive map

View more CodeSee Maps

Legend

[Andarist]

I've considered sorting this when we create States:

xstate/packages/core/src/State.ts

Line 251 in b6dde90

this.configuration = config.configuration;

but this has resulted in some test failures. I think that all of them were either snapshot tests (so we could just update them) or were related to rehydration. When dealing with the latter it turns out that config.configuration can be undefined. Well, our tests say that this can be undefined but our types assume that this is a required property:

xstate/packages/core/src/types.ts

Line 1247 in b6dde90

configuration: Array<StateNode<TContext, any, TEvent>>;

I could just default this to an empty array but this would also potentially cover up another problem. The problem is that state.configuration might be invalid/incomplete. I think this is "patched" for most cases within our resolveState:

xstate/packages/core/src/StateNode.ts

Lines 709 to 722 in b6dde90

	public resolveState(
	state: State<TContext, TEvent, any, any>
	): State<TContext, TEvent, TStateSchema, TTypestate> {
	const configuration = Array.from(
	getConfiguration([], this.getStateNodes(state.value))
	);
	return new State({
	...state,
	value: this.resolve(state.value),
	configuration,
	done: isInFinalState(configuration, this),
	tags: getTagsFromConfiguration(configuration)
	});
	}

I've added test cases for this as part of this PR here:
c4781eb

I've decided not to patch it and sort it in the constructor of the State for now though as that would incur the sorting cost for all created States (and we create them after each transition). By only sorting this here we make this operation "lazy" in a sense that it's only performed when it is actually needed

[Andarist]

Actually... I've made a huge mistake at first and I've sorted those exit actions in the document order while this should be done in the reversed document order (just how it's done when sorting & calling exit actions based on the exitSet during regular transitions).

The exitOrder is defined here as follows:

exitOrder // Descendants precede ancestors, with reverse document order being used to break ties
(Note: since descendants follow ancestors, this is equivalent to reverse document order.)

So sorting this in the State's constructor wouldn't be good anyway because most likely a document order would be better suited there while we need the reversed document order here. But, of course, if that would be sorted in the documented order then we could just iterate in reverse rather than sorting it but the "lazy" argument still stands.

[Andarist]

I've moved those to the new describe blocks as those were nested in the "choose" block which didn't quite make sense for those tests.

[Andarist]

This is the only test added in this file. It's sort of basic and acts as a regression test here. Its implementation depends on the fact that transitions "mess up" the state.configuration order and this is an implementation detail - I think I'm going to add in a sec another, similar, test for parallel states, just in case (I know it will pass right now)

[Andarist]

Added those tests for parallel regions here:
6b5a57c
I've created 3 very similar tests but each one of them is "messing" up the state.configuration in a different order so I've decided to include all of those cases