handleTransition's setState batched, actions not called

[lnogol]

I'm using withStatechart and this parallel machine (simplified):

export default {
  key: 'controller',
  parallel: true,
  states: {
    NormalizeItems: {
      initial: 'Normalizing',
      states: {
        Normalizing: {
          on: {
            NORMALIZE_SUCCESS: {
              Normalizing: {
                actions: ['startSessionSocket', 'subscribeItems'],
              },
            },
          },
        },
      },
    },
    SessionSocket: {
      initial: 'Disconnected',
      states: {
        Disconnected: {
          on: {
            START_SESSION_SOCKET: {
              Connected: {
                actions: ['joinSessionSocket'],
              },
            },
          },
        },
        Connected: {
          on: {
            SESSION_SOCKET_SUBSCRIBE: {
              Connected: {
                actions: ['emitSubscribeItems'],
              },
            },
          },
        },
      },
    },
  },
}

At some point, the first "child machine" calls startSessionSocket and subscribeItems, which look like this:

startSessionSocket = () => {
  this.props.transition('START_SESSION_SOCKET')
}

subscribeItems = () => {
  this.props.transition('SESSION_SOCKET_SUBSCRIBE')
}

This triggers the second child machine and should also call joinSessionSocket and emitSubscribeItems. However, only the second action (emitSubscribeItems) is called.

I suspect that's because I call one transition after the other, sync, and setState gets batched by React here. And since runActionMethods is called by componentDidUpdate, the joinSessionSocket action is not called.

I understand you may not see this as a bug, and I could possibly fix this by grouping this to one transition / one action, but this happens in more places in the app and I figured such a fix doesn't always result in maintainable code.

BTW: This component is one of the many "units" I have, and it has no UI. So I don't care much about re-renders.

TL;DR: Action methods are called in componentDidUpdate. When React happens to batch multiple setState() calls in handleTransition, the machine itself is in the correct state (thanks to setState(prevState => ...)), but only actions from the last transition are called.

[oliverhausler]

I consider this a bug. Only rendering the last UI state is an optimization which makes perfect sense, but business logic must always be executed. We may discuss if intermediate transactions are to be executed sync or async, but completely omitting these is wrong IMO.

[MicheleBertoli]

Thank you very much @lnogol for opening this issue. This is a pretty interesting problem, and I'm more than happy to think about a solution.

[lnogol]

I was actually thinking of a solution, but it's tricky.

Calling the actions earlier than componentDidUpdate (perhaps in handleTransition right away) won't work I think, would break the Action component (and maybe more).

Preventing React from batching setState (eg. using setTimeout) - ugly, and could also not maintain order correctly.

I was playing around with setState batching and found a way to tell if prevState is actually rendered, if that helps: https://codesandbox.io/s/mop9rn8j1p

[lnogol]

I know this is probably not an ideal solution, but I tried this, didn't work. xstate was throwing errors

return {
  componentState: { ...prevState.componentState, ...stateChange },
  event,
  // decide if prevState actions were called
  ...(isBatched
    ? {
        machineState: {
          ...nextState,
          actions: prevState.machineState.actions.concat(
            nextState.actions
          ),
        },
      }
    : {
        machineState: nextState,
      }),
}

[MicheleBertoli]

Thank you very much @lnogol for investigating the issue.

• calling runActionMethods earlier has a problem related to the component state: if one of the actions fires another transition with an updater, the prevState is out of sync.

• setTimeout seems to be the easiest patch, but I wouldn't consider it.

• your experiment around batching is interesting. However, it generates an inconsistency between the current machine state and the actions (i.e. the machine state is the latest, but the actions are accumulated from the last X transitions). Also, if any of the actions relies on the prevState it receives the final one, which is not correct.

I'll keep on thinking about a solution, and I'll update you here if I find something - please do the same.

[lnogol]

Thanks! Would forceUpdate() be of any help here? Also ugly, I guess.

[MicheleBertoli]

To avoid confusion, and make the current behaviour more explicit, I was thinking about adding an invariant while we find a solution.

[oliverhausler]

ln118 !this.isTransitioning should be -> this.isTransitioning or am I mistaken?

Haven't been digging into the code, but the invariant should possibly throw if another transition is already happening.

[MicheleBertoli]

Thank you very much @oliverhausler for your comment. invariant throws an exception when the provided value is "falsey". In this case, we want to make sure !this.isTransitioning is true when we run a new transition.

[lnogol]

How about not actually transitioning in handleTransition but rather add the requested transition to a queue that is processed in componentDidUpdate? Not sure if that would work, just an idea.

[oliverhausler]

@MicheleBertoli said:

calling runActionMethods earlier has a problem related to the component state: if one of the actions fires another transition with an updater, the prevState is out of sync.

@lnogol said:

How about not actually transitioning in handleTransition but rather add the requested transition to a queue that is processed in componentDidUpdate?

I am not sure if Michele was talking about below problem in his comment [sorry my ignorance about the internal workings of React batching], but in the above example there are potentially 2 orders in which actions may be performed:

a. startSessionSocket -> joinSessionSocket -> subscribeItems -> emitSubscribeItems (stack) b. startSessionSocket -> subscribeItems -> joinSessionSocket -> emitSubscribeItems (queue)

IMO they should be performed as in a., so that triggered actions are run immediately after the triggering action, as in a computer program where a sub-routine is executed BEFORE the consecutive step in the main program is executed. So we may rather want to use a stack [which would not give Lukas the expected result, though].

This leads me to the question if stacked actions should be allowed at all. They make sense in xstate, where we have pure state, but they may simply not work with React, where state is batched.

@davidkpiano In which order would xstate execute actions? Is this defined?

[davidkpiano]

In which order would xstate execute actions? Is this defined?

Order shouldn't matter (actions should be treated as atomic, and not dependent on other actions, according to Harel's paper), but in general, it's always:

1. onExit actions
2. transition actions (which should be preferred)
3. onEntry actions

[oliverhausler]

@davidkpiano I can't grab Mr. Harel here on GitHub I guess, but if actions would be atomic, they couldn't ever trigger a transition, as this would immediately introduce a dependency. There couldn't be shared state if they were atomic. And exactly this is the problem that we have here:

What would happen if I had multiple actions in 2.? These are executed in the order they are contained in the array, so far so good. But what happens if one of these actions runs a transition which triggers another (sub-)action? Is the sub-action executed between the 2 actions in the array, or are all sub-actions queued and executed at the end?

[lnogol]

Should this be closed? The bug is not fixed

[oliverhausler]

To resume:

As nice the approach of marrying React state with automata, IMHO it cannot be fixed. React state is UI state and optimized for that purpose. Using it for business logic like automata simply is an approach that doesn't work, except we disallow starting a second action before the first has been processed [essentially what @MicheleBertoli did].

Michele, can you express your thoughts, please.

[MicheleBertoli]

Thank you very much for your comments @lnogol @oliverhausler.

I closed this issue with a commit that prevents to fire consecutive transitions and enforces the intended behaviour.

For example, the following handler:

handleClick = () => {
  this.props.transition('FOO')
  this.props.transition('BAR')
}

throws this error:

I'm not making any assumption but I couldn't find a use-case where firing multiple tranisitions in the same "render" was the only option.

Joao, on Twitter, expressed a similar opinion:

Well, if someone's doing consecutive transitions they need to step back and stop for a bit to think how state machines work.

Also, I disagree that "Using it for business logic like automata simply is an approach that doesn't work", I just think we need to find the correct approach to get the best of both worlds. For example, when I started using Flux I hated the "cannot dispatch in the middle of a dispatch" error until I learned how to use it properly.

In any case, feel free to reopen this issue and most importantly to propose solutions and/or submit PRs, I'll be more than happy to discuss and review the code.

I will keep on thinking about this as well - it's just that I couldn't find a simple/clean solution and often, when this happens, it means I'm trying to solve the wrong problem : )

[ooloth]

This seems related to the discussion above, so I'm adding it here. (Please let me know if I should move it to a new issue.)

I'm trying to use an intermediate "revealing" state to allow a GSAP animation to complete before it can reset (to avoid jank). The "reveal" animation is triggered when the component enters the viewport and the "reset" method is triggered when the component leaves the viewport. I'd like the intermediate "revealing" state to prevent the "reset" method from being called partway through the animation.

However, I am getting the following error:

Cannot transition on "REVEALED" in the middle of a transition on "REVEAL"

Chart:

const revealChart = {
  initial: `hidden`,
  states: {
    hidden: {
      on: { REVEAL: { revealing: { actions: [`reveal`] } }  }
    },

    revealing: {
      on: {  REVEALED: `revealed` }
    },

    revealed: {
      on: { RESET: { hidden: { actions: [`reset`] } } }
    }
  }
}

Methods of <Reveal /> class component (simplified for brevity):

handleWaypointEnter = () => this.props.transition(`REVEAL`)
handleWaypointLeave = () => this.props.transition(`RESET`)

reveal = () => {
  this.node.animation = TweenMax.from(this.node, 1, {
    css: { opacity: 0 },
    onComplete: this.props.transition(`REVEALED`)
  })
}

reset = () => TweenMax.set(this.node, { opacity: 0 })

Is this triggering the consecutive transition error because my "revealing" state has no actions associated with it?

My "revealing" state has no actions because it's purpose is simply to prevent unwanted actions (i.e. "reset") until the animation finishes. (It's essentially a pending/loading state.)

It seems like a pending state followed by a success state should be a pattern that works well in a state chart, but perhaps I'm implementing it incorrectly here. Any tips would be appreciated!

[MicheleBertoli]

Thank you very much for your comment, @ooloth. This is interesting and surely related to this issue.

I confirm that a pending state (e.g. fetching) followed by success/error is a perfect use-case for statechart and it normally works.

In your case there might be some "concurrency" problem, do you mind sharing a complete non-working example so that I can play with it?

[ooloth]

Thanks for the reply, @MicheleBertoli!

Here's a working example: https://codesandbox.io/s/mjzmpwr30j.

I'm probably making some kind of rookie mistake here... Any tips would be appreciated.

[davidkpiano]

I think the logic here: https://github.com/MicheleBertoli/react-automata/blob/6f5b5051a4b14c7f21ba0a6aa63cf7e15e2ad2c3/src/withStatechart.js#L111 needs to be loosened a bit (specifically this.isTransitioning) for two reasons:

1. transitions should be considered zero-time (they should happen instantaneously, always)
2. we should defer the queued state updates to how React naturally handles it.

That might solve this issue and simplify the code a bit.

[MicheleBertoli]

Thank you very much @ooloth for providing an example, and @davidkpiano for your comment.

@ooloth the problem is that you are transitioning within an action. A potential solution would be wrapping the animation into a raf.

reveal = () =>
  requestAnimationFrame(() => {
    TweenMax.from(this.node, 1, {
      css: { opacity: 0 },
      onComplete: this.props.transition(`REVEALED`), 
    })
  })

@davidkpiano unfortunately, there are some problems with no. 2 (see previous comments) but I'm planning to rewrite part of the logic and fix the issue. For now, isTransitioning is a way to make the current behaviour explicit.

[MicheleBertoli]

To make the "problem" clearer, I created a couple of examples of the most common approaches we could take.

Consider the following statechart:

const statechart = {
  initial: 'a',
  states: {
    a: {
      on: {
        EVENT: {
          b: {
            actions: ['myAction1'],
          },
        },
      },
    },
    b: {
      on: {
        EVENT: {
          b: {
            actions: ['myAction2'],
          },
        },
      },
    },
  },
}

Approach 1: actions in componentDidUpdate

That's the current approach, and it presents the problem described at the beginning of the issue: since multiple setState calls are batched and we fire the actions in componentDidUpdate, only the actions of the last transition are actually executed. This is now mitigated by the isTransitioning flag, which makes the intended behaviour more explicit. In general, although transitions should be considered zero-time, I like this solution because it's more inline with the way React works.

class Automata1 extends React.Component {
  machine = Machine(statechart)

  state = {
    machineState: this.machine.initialState,
  }

  transtion = event =>
    this.setState(prevState => ({
      machineState: this.machine.transition(prevState.machineState, event),
    }))

  componentDidUpdate() {
    this.state.machineState.actions.forEach(action => this[action]())
  }

  // this is never fired, because state changes are batched
  myAction1 = () => console.log('myAction1', this.state.machineState)

  myAction2 = () => console.log('myAction2', this.state.machineState)

  handleClick = () => {
    this.transtion('EVENT')
    this.transtion('EVENT')
  }

  render() {
    return <button onClick={this.handleClick}>Approach 1</button>
  }
}

Approach 2: actions in setState

Another potential solution would be to fire the actions right after the transition (and before the update). This adheres to the statechart principles, but there's a downside: when the actions are fired, the new machine state hasn't been added to the state yet (because setState is async) and therefore its value is not correct.

class Automata2 extends React.Component {
  machine = Machine(statechart)

  state = {
    machineState: this.machine.initialState,
  }

  transtion = event =>
    this.setState(prevState => {
      const machineState = this.machine.transition(
        prevState.machineState,
        event
      )
      machineState.actions.forEach(action => this[action]())

      return { machineState }
    })

  // machineState is wrong, because setState is async
  myAction1 = () => console.log('myAction1', this.state.machineState)

  // machineState is wrong, because setState is async
  myAction2 = () => console.log('myAction2', this.state.machineState)

  handleClick = () => {
    this.transtion('EVENT')
    this.transtion('EVENT')
  }

  render() {
    return <button onClick={this.handleClick}>Approach 2</button>
  }
}

Sandbox: https://codesandbox.io/s/53lzpkww0l

I'm still not 100% convinced that we need to support multiple transitions, but I'm experimenting with various solutions from forceUpdate, to queues. I'll keep you posted, and feel free to add your ideas to this thread or send a PR. Thank you very much!

[davidkpiano]

What about as a callback to this.setState()?

E.g.,

this.setState(
  () => {}, // set the finite state
  () => {}  // execute all actions that may also change state
);

[MicheleBertoli]

Thank you very much for suggesting to use setState callbacks, @davidkpiano.

They work similarly to componentDidUpdate (approach 1):

The second parameter to setState() is an optional callback function that will be executed once setState is completed and the component is re-rendered. Generally we recommend using componentDidUpdate() for such logic instead.

But their behaviour is counter-intuitive sometimes (see my poll).

In fact, they would make the problem a bit worse: consecutive setState calls would still be batched (the issue described in the original comment) and all the callbacks would run only once the state is updated.

In the specific case, with the following implementation:

transtion = event =>
  this.setState(
    prevState => ({
      machineState: this.machine.transition(prevState.machineState, event),
    }),
    () => this.state.machineState.actions.forEach(action => this[action]())
  )

The component would be re-rendered once, and both callbacks would fire myAction2 (the final value of the machine state).

[larsbuch]

I am hitting this as well with concurrency as i have two updaters (user and server). Have you found a way to solve it? When i hit a problem like this I think statemachine. I see it like we have three interacting here: React statemachine, xstate statemachine (mine) and react-automata statemachine. Because state transitions are not instant (except perhaps theoretically) it needs to handle both we can change. I am thinking of wrapping my statemachine in a "react-automata" statemachine so mine runs a a substate of the react-automata state.

[MicheleBertoli]

Thanks for your comment, @larsbuch. Would you be able to provide more information about your "concurrency" problem?