Event action invoked after transient transition guard evaluation

[czabaj]

Description I created an already closed issue #1199. I investigated the issue further and identified the root cause, which I believe, is a bug.

Given a simplified example (the use case is described in #1199)

const loginMachine = Machine({
  id: "login",
  initial: "pin",
  states: {
    pin: {
      on: {
        "": [
          { cond: "pinNotSet", target: "password" },
          {
            cond: {
              type: "maxPinFailedAttemptsReached",
              maxFailedAttemptsCount: 5
            },
            actions: [
              "notifyMaxFailedPinAttemptsReached",
              "resetPinFailedAttemptsCount"
            ]
          }
        ],
        SUCCESS: "userAuthenticated",
        FAIL: {
          actions: ["incrementPinFailedAttemptsCount"]
        }
      }
    },
    password: {
      on: {
        SUCCESS: "userAuthenticated"
      }
    },
    userAuthenticated: {
      entry: ["resetPinFailedAttemptsCount"],
      type: "final"
    }
  }
});

We are in pin state. Null transition checks, if user failed to enter PIN more than 5 times. When user enters wrong PIN, the form sends FAIL event, which triggers incrementPinFailedAttemptsCount event action. The context.failedAttemptsCount is incremented via assign action. Because the FAIL transition has no target, it is an internal transition, and the pins state null transition reevaluates maxPinFailedAttemptsReached.

Please, see the example in CodeSandbox, open the console, click on the SEND button and observe, that everything works as expected, the ACTION incrementPinFailedAttemptsCount is executed before GUARD maxPinFailedAttemptsReached. Nice.

But in my implementation, I persist the failedAttemptsCount in device storage, so user cannot simply reset failedAttemptsCount with app refresh. Because failedAttemptsCount is not hold in machine's context, I don't use assign action, but I use function expressions

{
  actions: {
    incrementPinFailedAttemptsCount: () => {
      const storedValue = storage.get('failedAttemptsCount');
      storage.set('failedAttemptsCount', storedValue + 1);
    }
  }
}

I find out, that using this function expression instead assign action postpones the action, it is executed after GUARD maxPinFailedAttemptsReached. Guard then works with data not updated by transition's action. I think it is a bug, since approach with assign action executes action and guard in correct order.

See CodeSandbox where I emulate storage with useState hook.

Expected Result The (internal) transition's action written as function expression is executed before state's NULL event.

Actual Result The (internal) transition's action written as function expression is executed after state's NULL event and thus transient transition conds are evaluated with wrong data.

Reproduction Storing data in context and using assign action works as expected, ACTION incrementPinFailedAttemptsCount is executed before GUARD maxPinFailedAttemptsReached. See console and click SEND in https://codesandbox.io/s/stoic-grass-278ke?file=/src/App.js.

Storing data outside context and using ad-hoc function expression as transtition's action does not work. ACTION incrementPinFailedAttemptsCount is executed after GUARD maxPinFailedAttemptsReached. See console and click SEND in https://codesandbox.io/s/reverent-dream-4y7vl?file=/src/App.js

[Andarist]

I'm afraid that currently, this behavior won't change - the API is not supposed to be used like that and I would recommend only ever using structures provided by a machine (context, event, etc) to implement your guards. It should be fairly easy to just store your counter inside the context by using assign and only syncing the current counter to the localStorage when needed.

Keep also in mind that your repro case which uses React is even worse than the localStorage example because React's state is async and there is literally nothing we could do to read its updated value from within that guard as the change won't update until next render and the guard has to be evaluated immediately.

I'm going to close this issue as it's not actionable from our perspective, but feel free to discuss this further if you still have any doubts - I will make sure to get back to you regardless of the issue being closed.

[czabaj]

Hello. Thanks for quick reply. I understand that storing data in context is preferred for many reasons, it was BTW my first instinct when implementing loginMachine, but i realized during implementation that almost all of my state needs to be persisted, and since my storage is synchronous, I can use it directly and omit bloated transport logic from storage to context and vice versa.

One of the greatest things about state-charts it its independence on underlying implementation. One can describe the intended logic very clearly and precisely with state-chart and then implement the actions, guard and other missing pieces in any way in virtually any programming language.

Formally, during transition

the SCXML Processor must first exit all the states in the transitions' exit set in exit order. It must then execute the executable content contained in the transitions in document order. It must then enter the states in the transitions' entry set in entry order. — https://www.w3.org/TR/scxml/#SelectingTransitions

I feel that xstate shall preserve the correct order even if action is function expression, otherwise it looses some of the great flexibility of state-charts.

[davidkpiano]

~@czabaj There is a good chance that your behavior will "just work" in V5, with the action resolution work we're doing.~ Never mind, this isn't the case.

If you're reading something async, or out-of-scope from the machine, that should be modeled as an explicit state with invoke to do the reading.

[Andarist]

@czabaj we realize this is not strictly compatible with SCXML right now, but it really rarely matters - even if this would work like in SCXML then I still would believe that this could be better modeled explicitly (by keeping your data within machine's context).

We might revisit this in the future, but we need to maintain two sets of APIs - Interpreter and Machine, where the latter is pure, so it is somewhat limiting us here (or rather - it enforces a particular design right now). If we figure out how to reimplement stuff in a way that would cover both APIs without implementing both completely separately then we might consider changing this, but there is no concrete plan to pursue this right now.

[czabaj]

@davidkpiano @Andarist guys, thank you both for feedback. I understand that using data from context is the primary API and I can live with it. It would probably deserve some bold notice in the docs, for other people not step in the same puddle, although it might be already in the docs and I just forget it. I'm still new to state-charts and XState and I might sometimes demand too much from it given its super powers.