react: Bug: is the current `useSyncExternalStore` batching & flushing behaviour intended?

React version: 18

Link to code example:

The expected behavior

Let’s assume we have a increment function that first increments a local value, then a uSES value and then another local value like this:

function increment() {
    setState1((x) => x + 1);
    setUsesState((x) => x + 1);
    setState2((x) => x + 1);
  }

Now, there would be two ways this could behave that would be “intuitive for me”:

everything is batched: [state1, usesState, state2] goes from [0,0,0] to [1,1,1]
it batches until the “sync update” flushes the current batch: [state1, usesState, state2] goes from [0,0,0] to [1,1,0] to [1,1,1]

The current behavior

Now, actual behaviour is different in React 18, depending on the “mode” React is currently in.

in an event handler, everything is batched [0,0,0] to [1,1,1] - no problem here
outside an event handler, the uSES setter is flushed first, then the local state changes are batched. [0,0,0] becomes [0,1,0] becomes [1,1,1] - this is very unintuitive for me.
even inside a manual call wrapped in unstable_batchedUpdates, we go [0,0,0] -> [0,1,0] -> [1,1,1]

Point 3 means that there is actually no way to even manually batch an update by uSES - but looking at point 1, React sometimes does so internally.

It seems that even in the non-batched situations, React does some batching: Calling setUsesState twice before calling setState2 will not lead to a [0,0,0] -> [0,1,0] -> [0,2,0] -> [1,2,1] situation, but to [0,0,0] -> [0,2,0] -> [1,2,1]

Up until now we had assumed that uSES would always behave like in 1., and we were only made aware of this by bug reports on react-redux.

Is this intended behaviour or a bug?

There might be some high priority update thing with a transition that I am missing here though - but either way this feels very breaking from older behaviour to me - and yes, I know that batchedUpdates has the unstable prefix 😉

About this issue

Original URL
State: open
Created 2 years ago
Reactions: 5
Comments: 20 (11 by maintainers)

Most upvoted comments

We discussed with the team and we agreed that it makes sense to change this. Now it’s just a matter of implementing it and when someone can get around to it.

While React treats these as separate lanes, the programming model only has “Sync” and “Transition” so it makes sense to treat these all as Sync and flush them all together at the last possible opportunity but no later than the earliest heuristic.

If something is wrapped in startTransition only the setStates will be delayed, and any uSES will be flushed early. I thought that case was even a warning? Maybe we should add back the warning.

+10

sebmarkbage on Sep 13, 2022

Hm, while priority-based updates are probably a powerful feature - it’s really hard to grasp how this works in edge cases like this. It feels like there should be a way to somehow “join” those updates without resorting to flushSync. In this case, it would be quite convenient to change the priority of the update with the default priority if there is already an ongoing “sync” update. Correct me if I’m wrong but the uSES update is still not exactly synchronous - all updates coming from that store are batched together and more often than not it is desirable to flush other updates with those. Part of the problem is that people usually won’t even interact with uSES directly but rather through a library. In those situations, it’s even harder to notice that one might deal with such a discrepancy in flushed updates.

Andarist on Sep 5, 2022

In React <=17 the default for setState was sync - you could opt-in to batched with batchedUpdates.

In React 18 the default is batched - you can opt-out to sync with flushSync.

useSyncExternalStore is always sync to preserve the consistency with other mutable data.

sebmarkbage on Sep 5, 2022

It’s because useSyncExternalStore always has to be flushed at the highest priority where as other updates can be delayed. It can’t be delayed because of the “Sync” part which is a compromise to using this API since it trades preserving internal consistency with the external store by compromising the ability to delay it. So it can’t be batched but it also can’t be time sliced and deprioritized.

You can regain consistency by flushing other updates together with this one by using flushSync(...) but then you’re compromising by making all other updates not delayed neither.

If it was batched, which is basically what you’d get by manually subscribing without using it, you lose other types of consistency due to using a mutable backing store.

sebmarkbage on Sep 5, 2022

I meant only the in-consistent behavior is bare onClick. I didn’t mean which is correct or expected.

dai-shi on Sep 5, 2022

Andarist on Sep 5, 2022