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

Bug: preserveActionOrder is not prioritising execution of assign actions #3383

Closed
mieszko4 opened this issue Jun 6, 2022 · 3 comments · Fixed by #3387
Closed

Bug: preserveActionOrder is not prioritising execution of assign actions #3383

mieszko4 opened this issue Jun 6, 2022 · 3 comments · Fixed by #3387

Comments

@mieszko4
Copy link

mieszko4 commented Jun 6, 2022

Description

I've initially created #3382 to show how preserveActionOrder=true is confusing because it only preserve the order of context and not the order of execution of actions.
However, I am not sure anymore what is the intention of preserveActionOrder because it mentions the order of execution as described in (https://github.com/statelyai/xstate/blob/1d1c90c4be57e606975564114b7f23d951ad5ccb/packages/core/CHANGELOG.md#minor-changes-9):

There is a new .preserveActionOrder (default: false) setting in the machine configuration that preserves the order of actions when set to true. Normally, actions are executed in order except for assign(...) actions, which are prioritized and executed first. When .preserveActionOrder is set to true, assign(...) actions will not be prioritized, and will instead run in order. As a result, actions will capture the intermediate context values instead of the resulting context value from all assign(...) actions.

Hence I am creating this issue to show the problem.

// Based on example from docs (https://xstate.js.org/docs/guides/context.html#action-order)
const counterMachine = createMachine<ToggleContext, ToggleEvent>({
  preserveActionOrder: true, // Ensures that assign actions are called in order
  initial: "active",
  // ...
  context: { count: 0 },
  states: {
    active: {
      on: {
        INC_TWICE: {
          actions: [
            (context) => console.log(`Before: ${context.count}`), // "Before: 0"
            assign((context) => {
              console.log(`First assign`);
              return { ...context, count: context.count + 1 };
            }), // count === 1
            assign((context) => {
              console.log(`Second assign`);
              return { ...context, count: context.count + 1 };
            }), // count === 2
            (context) => console.log(`After: ${context.count}`) // "After: 2"
          ]
        }
      }
    }
  }
});

interpret(counterMachine).start().send({ type: "INC_TWICE" });

Expected result

"Before: 0"
"First assign"
"Second assign"
"After: 2"

Actual result

"First assign"
"Second assign"
"Before: 0"
"After: 2"

Reproduction

https://codesandbox.io/s/stupefied-torvalds-mjjhi0

Additional context

xstate@4.31.0

@mieszko4 mieszko4 added the bug label Jun 6, 2022
@davidkpiano
Copy link
Member

Assign actions should be pure. We should mention this in the docs.

@mieszko4
Copy link
Author

mieszko4 commented Jun 7, 2022

Assign actions should be pure. We should mention this in the docs.

Thank you for clarifying that! For me should be pure does not sound strong enough :) I think it would be better to mention must be pure in the docs.

@davidkpiano davidkpiano linked a pull request Jun 7, 2022 that will close this issue
@Andarist
Copy link
Member

Andarist commented Jun 7, 2022

I'm working on another flag that makes this behavior even more predictable (PR) and with that PR you would actually get your expected result:

"Before: 0"
"First assign"
"Second assign"
"After: 2"

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

Successfully merging a pull request may close this issue.

3 participants