Bug: v5 `return assign()` from action behavior change from v4

[pckilgore]

XState version

XState version 5

Description

Can no longer return assign({ value }) from action functions.

This isn't called out in https://stately.ai/docs/migration and does not result in a type checking error.

Expected result

I expected a section in the migration docs that this pattern is deprecated or a compiler error from typescript that the return value was not the correct type.

Or, I expected this to work like in v4.

Actual result

No docs, no warning, production bug.

Reproduction

https://codesandbox.io/s/charming-rhodes-mw5dys?file=/src/App.tsx

Additional context

No response

[davidkpiano]

You were never able to do this, including in XState v4:

      addNumBad: ({ context }) => {
        const nums = new Set(Array.from(context.nums));
        nums.add(Math.round(Math.random() * 5000));
        console.log("addNumBad", ...Array.from(context.nums));

        // This used to work.
        return assign({ nums });
      },

Can you share code where this used to work?

[pckilgore]

https://codesandbox.io/s/charming-rhodes-forked-hykf5f?file=/src/App.tsx

Looking at this knowing that, it seems like it might be the behavior of @xstate/react that changed? The mutation of context stands out if the assign is ignored, but the component still updated and so did context so nobody caught that the assign was unless / etc.

[davidkpiano]

Sorry, can you clarify which lines are the lines where the behavior of @xstate/react changed?

[pckilgore]

Here's the same machine running on v4 patterns:

• https://codesandbox.io/s/xstate-v4-hykf5f

Then migrated to v5 patterns:

• https://codesandbox.io/s/xstate-v5-vnxh9y

I'm not sure I can point to "lines" here. There's a button that demonstrates the v4 machine would fire the event, which would fire the action, then react would re-render. In the v5 example, the button fires the event, which fires the action, but react does not re-render the component.

[Andarist]

In v5 we avoid creating redundant snapshot values when there is no change in the snapshot's value as a whole. A side-effect action fired in response to an event doesn't change the snapshot's value anyhow so it doesn't rerender.

[pckilgore]

For what it's worth, I think that makes sense and was a good decision for v5.

Realistically, do you mind dropping a note in the migration guide that xstate became more efficient in terms of triggering React re-renders and so if you were previously relying on sending an event to trigger a necessary re-render, that code will no longer work (unless that event updates the state / context).

I cannot imagine we were the only team on v4 that was accidentally mutating context and relying on the re-render-for-all-events behavior to result in a "working" machine, we're just early on the upgrade adoption curve and didn't know to audit for that.

Happy to make the PR myself if ya'll want some copy to re-write.

On Mon, Jan 15, 2024, at 4:13 AM, Mateusz Burzyński wrote:

In v5 we avoid creating redundant snapshot values when there is no change in the snapshot's value as a whole. A side-effect action fired in response to an event doesn't change the snapshot's value anyhow so it doesn't rerender.

— Reply to this email directly, view it on GitHub https://github.com/statelyai/xstate/issues/4673#issuecomment-1891797519, or unsubscribe https://github.com/notifications/unsubscribe-auth/AATQYQ3IQPKN56XELOYDJNLYOT6NVAVCNFSM6AAAAABBYXHEFWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJRG44TONJRHE. You are receiving this because you authored the thread.Message ID: @.***>

[davidkpiano]

For what it's worth, I think that makes sense and was a good decision for v5. Realistically, do you mind dropping a note in the migration guide that xstate became more efficient in terms of triggering React re-renders and so if you were previously relying on sending an event to trigger a necessary re-render, that code will no longer work (unless that event updates the state / context). I cannot imagine we were the only team on v4 that was accidentally mutating context and relying on the re-render-for-all-events behavior to result in a "working" machine, we're just early on the upgrade adoption curve and didn't know to audit for that. Happy to make the PR myself if ya'll want some copy to re-write. … On Mon, Jan 15, 2024, at 4:13 AM, Mateusz Burzyński wrote: In v5 we avoid creating redundant snapshot values when there is no change in the snapshot's value as a whole. A side-effect action fired in response to an event doesn't change the snapshot's value anyhow so it doesn't rerender. — Reply to this email directly, view it on GitHub <#4673 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AATQYQ3IQPKN56XELOYDJNLYOT6NVAVCNFSM6AAAAABBYXHEFWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJRG44TONJRHE. You are receiving this because you authored the thread.Message ID: @.***>

We would greatly appreciate a PR. Here is the relevant file to edit: https://github.com/statelyai/docs/blob/main/docs/migration.mdx

Thank you!