Implement predictableActionArguments feature flag

[Andarist]

Although this PR's main goal wasn't that - it can be used to supersede #3210 and thus it fixes the problem described here

The main goal of this work is to:

• always call/resolve actions with a source event - the event directly responsible for a given transition (currently builtin actions are called with that but custom actions are called with events responsible for triggering the current macro transition)
• do not call/resolve actions with an explicit NULL event ({ type: '' }). Instead, we are calling them with the source event as the always transitions are also called eventless transitions (this matches SCXML spec etc)
• allow us to implement an improved & correct algorithm for generating typegen information (I think that once we land this PR and the stop event PR then the algorithm in the typegen should be adjusted to account for those changes and that typegen should enforce using this feature flag).

[linear]

STA-1446

[changeset-bot]

🦋 Changeset detected

Latest commit: bb0e0fb

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

This PR includes changesets to release 1 package

Name	Type
xstate	Minor

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 bb0e0fb:

Sandbox	Source
XState Example Template	Configuration
XState React Template	Configuration

[Andarist]

This is very unfortunate because it means that initial actions will actually be called with a completely resolved context. Perhaps I can use a similar logic that is implemented here to overcome this problem:

xstate/packages/core/src/actions.ts

Lines 769 to 775 in 430362d

	const contextIndex = preservedContexts.length - 1;
	resolvedActionObject = {
	...resolvedActionObject,
	exec: (_ctx, ...args) => {
	exec(preservedContexts[contextIndex], ...args);
	}
	};

But I wonder how we could avoid this sort of thing in v5

[Andarist]

Actually - right now this just works:

const actual: number[] = [];

const m = createMachine({
  predictableActionArguments: true,
  context: { count: 0 },
  entry: [
    (ctx) => actual.push(ctx.count),
    assign({ count: 1 }),
    (ctx) => actual.push(ctx.count),
    assign({ count: 2 }),
    (ctx) => actual.push(ctx.count),
  ],
});

const s = interpret(m);
expect(s.initialState.context).toEqual({ count: 2 });

expect(actual).toEqual([]);

s.start();

expect(actual).toEqual([0, 1, 2]);

But it relies on this exact logic that I've linked and I think that ideally, we should get rid of this exec mutation in v5. It's a problem for future us though.

[Andarist]

This leads to mutating this.state.children where this.state has not yet been updated with the new state. I need to figure out how I can avoid this problem.

[Andarist]

I think this is OK given the current state of things - .children are already mutated and somewhat unstable: https://codesandbox.io/s/keen-cookies-vfte0i?file=/src/index.js:798-972

I think though that this should be fixed in v5

[davidkpiano]

Bikeshed: predictableActionArguments is a pretty vague description of what this does, since it's mainly for preserving events in transient states. Maybe preserveEvents? Would be similar to preserveActionOrder

[Andarist]

I've chosen that because it also enables the preserveActionOrder automatically - so it's both about context and event arguments. I'm OK with doing changes to that though.

[Andarist]

@davidkpiano any thoughts about the core of this PR? I'd like to know how you feel about it before polishing it further

[davidkpiano]

@davidkpiano any thoughts about the core of this PR? I'd like to know how you feel about it before polishing it further

My primary concern is the spreading of isTransient throughout many methods; perhaps we can have a .microTransition(...) separate method instead or something like that?

[Andarist]

My primary concern is the spreading of isTransient throughout many methods; perhaps we can have a .microTransition(...) separate method instead or something like that?

"The first" method that receives this argument is already resolveRaisedTransition:

xstate/packages/core/src/StateNode.ts

Line 1216 in 430362d

private resolveRaisedTransition(

it just calls to some other functions (as the internal machinery is reused, in the same way, it was reused before), and those need to pass it around as the event.type alone is not sufficient for candidate transitions selection. IIRC we "only" need to get that information to the getCandidates call here:

xstate/packages/core/src/StateNode.ts

Line 908 in 430362d

for (const candidate of this.getCandidates(isTransient ? '' : eventName)) {

This is mostly internal and we might figure out a better algorithm in the future so I wouldn't be bothered about this that much.

[davidkpiano]

So this does the same thing as preserveActionOrder?

[Andarist]

It includes that behavior as per the test here but this test is about something different.

At the moment if custom actions are executed within the interpreter so when the machine.transition returns back to it. And builtin actions are resolved within the machine.transition call, so with this:

entry: [
  function first() {},
  assign(function second() {})
]

we might actually observe those functions to be called in the unpredictable order. If we'd put console.logs in them we would see:

second
first

With this PR the order of execution matches more common expectations, resolving builtin actions is immediately followed by executing them, like here:

xstate/packages/core/src/actions.ts

Lines 667 to 672 in 430362d

	const resolved = resolveLog(
	actionObject as LogAction<TContext, TEvent>,
	updatedContext,
	_event
	);
	predictableExec?.(resolved, updatedContext, _event);

And thus we would observe:

first
second

[davidkpiano]

I don't think this will be tricky, especially if we execute actions eagerly in v5, as you suggested?

[Andarist]

The question here is - what exactly the machine.initialState is and what context value does it hold? Does it already have assigns from a top-level entry resolved? remember that machine.initialState.context has to be pure so we cant yet execute actions eagerly at this point in time

[davidkpiano]

That's why currently there's the notion of preInitialState in v5, which represents the initial state and actions without any assignments performed nor actions executed.

In my view, this is what machine.initialState (the pure getter) should be - state without actions executed. Then when the machine starts (is interpreted), the actions should be executed.

So in this test, machine.initialState would have context { count: 0 } since no actions were executed yet, but the read state from interpreter.subscribe(state => ...) would have { count: 2 } since the state after the initial state has those actions executed.

This would also be what the input RFC would solve, since there would be no need for initial assign actions.

Half-baked thoughts, just thinking out loud

[davidkpiano]

What remains left for this to be merged in?

[Andarist]

@davidkpiano still working on adding support for this in the typegen, I've just pushed out an "enabler" PR for this work today: statelyai/xstate-tools#170 . I still need to build on top of that but this refactor allows me to extend the current shape of the code in an easier way

[Andarist]

I've reverted the change related to making a NULL event a pseudo-event with this flag. Because of that, all the entry actions following always transitions would have to be callable with TEvent and that takes away a lot of the typegen appeal (for those cases, but they are not that rare). Since we already have this logic in v5 - we should revisit it and make a decision if it's good or if we should revert it. To postpone this decision I got rid of it from here.

[mellson]

I think this looks great!

[davidkpiano]

Can you add the XState version this applies to?

[davidkpiano]

Suggested change

	* @default false
	* Whether XState calls actions with the event directly responsible for the related transition.
	* @default false

[LekoArts]

Can y'all add more information how to migrate? We had trouble finding a replacement for using this. Thanks!

[Andarist]

Could you open a discussion that would describe your use case for using this property?

[LekoArts]

Sure!

[LekoArts]

@Andarist Done :) #3511