Skip to content
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

Implement predictableActionArguments feature flag #3289

Merged
merged 10 commits into from
Aug 2, 2022

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented May 12, 2022

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).

@Andarist Andarist requested a review from davidkpiano May 12, 2022 15:29
@linear
Copy link

linear bot commented May 12, 2022

STA-1446

@changeset-bot
Copy link

changeset-bot bot commented May 12, 2022

🦋 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
Copy link

ghost commented May 12, 2022

👇 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!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 12, 2022

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

Comment on lines +314 to +316
// this is currently required to execute initial actions as the `initialState` gets cached
// we can't just recompute it (and execute actions while doing so) because we try to preserve identity of actors created within initial assigns
_event === initEvent) &&
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

Copy link
Member Author

@Andarist Andarist Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

actionObject as StopAction<TContext, TEvent>,
updatedContext,
_event
);
predictableExec?.(resolved, updatedContext, _event);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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

@davidkpiano
Copy link
Member

@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
Copy link
Member Author

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:

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:

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.

import { raise, stop } from '../src/actions';

describe('predictableExec', () => {
it('should call mixed custom and builtin actions in the definitions order', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this does the same thing as preserveActionOrder?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

And thus we would observe:

first
second

Co-authored-by: David Khourshid <davidkpiano@gmail.com>
@@ -584,4 +585,25 @@ describe('predictableExec', () => {

expect(actual).toEqual(['stop 1', 'start 2']);
});

// TODO: this might be tricky in v5 because this currently relies on `.exec` property being mutated to capture the context values in a closure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

What remains left for this to be merged in?

@Andarist
Copy link
Member Author

@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
Copy link
Member Author

Andarist commented Jul 14, 2022

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.

Copy link
Contributor

@mellson mellson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks great!

@@ -1,5 +1,25 @@
# Actions

::: warning

It is advised to configure `predictableActionArguments: true` at the top-level of your machine config, like this:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the XState version this applies to?

@@ -662,6 +662,10 @@ export interface StateNodeConfig<
* @default false
*/
preserveActionOrder?: boolean;
/**
* @default false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Andarist Andarist merged commit c0a147e into main Aug 2, 2022
@Andarist Andarist deleted the andarist/sta-1446-predictable-action-arguments-feature branch August 2, 2022 21:17
@github-actions github-actions bot mentioned this pull request Aug 2, 2022
- XState will always call an action with the event directly responsible for the related transition,
- you also automatically opt-into [`preserveActionOrder`](https://xstate.js.org/docs/guides/context.html#action-order).

Please be aware that you might not able to use `state` from the `meta` argument when using this flag.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Andarist Done :) #3511

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants