Unexpected change in behaviour beween React 17 and 18

[obweger]

Unexpected change in behaviour beween React 17 and 18

[theKashey]

A classic "Diamond" problem. Only a few state managers solved it so far.

Vanilla context API is free from it, because "propagation" is synchronised with "rendering", however subscription based useSyncExternalStore is a little bit more "instant", so update of Alpha will reach Beta and Delta at the same time, and only then it will reach Gamma and then trigger secondary render of Delta

This problem was raised to React a couple of times, and the "official" answer (link pending) is that you should not worry how much time you component got rendered, you only need to worry on how much time result of it's work was "commited" because that is expensive operation, and the rest is not.

This still raises a question on how to synchronise some useEffects and run them only when rendering is "stable". I've even written an article about this 4 years ago (DejaVu, state synchronisation part)

---

However, let's try to do a leap of fate - here is a fix for batching, and batching is the moment expected to change with R18, so multiple events would be executed together. Before the first one could be delayed, and that might be a reason for double rendering.

It's just a few lines of code you can try to copy-paste into your local version of sweet-state. Please let me know if it helps

sorry, no R18 around yet to test

[albertogasparin]

Indeed, the tricky part is that the Beta update is triggered during render. At that point RSS will schedule a new update, so it will be:

Alpha update -> Beta + Delta re-render
Beta update -> Gamma + Delta re-render

In the past, RSS had its own implementation of useSyncExternalStore made in a way to allow children to get fresh values asap, without waiting for the next re-render. With React providing useSyncExternalStore, seems like such optimisation is gone once we consume the "native" version (with R18) 😞

One possible workaround is to skip scheduling on some container update actions. Basically implementing a flushSync method (that should also help fixing #178).
Give this a go and see if that helps (it does in the sandbox):

// beta store
const onUpdate =
  () =>
  ({ setState }: StoreAPI, { input }: Props) => {
    console.log('updating beta');
    // ideally this can be implemented inside a nicer facade, like `flushSync(() => ...)`
    defaults.batchUpdates = false;
    setState({ ouput: input });
    defaults.batchUpdates = true;
  };

The downside is that R18 now complains: "Cannot update a component (WiredGammaContainer) while rendering a different component (Container(beta)).", likely because useSyncExternalStore in Gamma also receives the updated value from beta store. Not sure if there is a way to avoid the warning without going back to our previous, custom made subscription.

---

Thinking out loud there is another path that you could take, but requires more refactoring: stores now have a handlers.onUpdate API (different from the container one) that is triggered when their value changes, outside of the react lifecycle. If you provide an action to subscribe then you can notify the other stores.
It kinda duplicates the subscription work, but the you have more power over who gets updated, and outside of react. I've made some changes to the sandbox to implement it:
https://codesandbox.io/p/devbox/sweet-state-batching-issue-forked-q24tvr

[obweger]

Thanks for the quick responses @theKashey @albertogasparin !

@theKashey I tried #218 and it doesn't actually seem to make a difference here. TBH, I don't have a good enough understanding of the internals to say anything more than that.

@albertogasparin Right. I could imagine the flushSync approach for my data processing pipeline, but the reality is that the previous/R17 behaviour is expected pretty much all over my code base. If that's the way to go, I suppose I can just as well set defaults.batchUpdates = false; categorically. The subscriber approach looks interesting, but would indeed be a big refactoring, as I'd have to move away from the props-based approach that I'm currently using.

@theKashey @albertogasparin - do you see any future for sweet-state that would bring back the previous behaviour in a React-18(+)-friendly way? It doesn't have to be fixed now - I'm okay with using defaults.batchUpdates = false for the time being - but if this is somewhat of a dead end, and I'll obviously have to reconsider things.

By the way, just in case it is helpful: If Gamma also has an Alpha hook, even though it doesn't actually use the value, things are back to the previous behaviour again: https://codesandbox.io/p/devbox/sweet-state-batching-issue-forked-ph9gks?file=%2Fsrc%2FApp.tsx

Thanks folks for your help, really appreciated.

[theKashey]

gone once we consume the "native" version (with R18)

So one of the versions is to keep on shim? That could be an option?

If Gamma also has an Alpha hook, things are back to the previous behaviour again

Interesting. Lets look at this black box from aside:

• if React "knowns" that pieces are somehow connected, it handles them in one way
• If React "thinks" pieces are independent, and Beta -> Gamma has nothing with Alpha -> * connection, it handles branches in parallel and this creates a problem

Sounds like an optimisation quite related to the "Concurrent Spirit".

---

A potential solution of the problem, in case if @obweger found a silver bullet, is to use single global store for managing subscriptions. Hello Redux and friends.

• one store to deliver "something updated"
• every subscription to check of "their" store is updated (fast) and get shapshot of data

The end result will trigger more code on any random update, but cause the same number of React component updates and more importantly guarantee (we dont have any proofs yet) correctness. With less following re-renders it might even work faster by doing more things.
And it does not need to be a super global store - we can handle it on per-store basic like container prop, ie one will be able to configure given store to be "isolated" or to "play as a team"

Thoughts?

[theKashey]

Alternate approach - is there any need to do any optimisations on our side with React 18 auto batching?

• autobatching will schedule all updates where React wants them
• it will be one controller to handle the majority of possible updates
• end user will have a capability to call flushSync and flush ALL scheduled events

Meaning:

• less things to be worried for us
• more control for the end user
• proved to be a solution for the problem
• already implemented, just disable batching logic for R18 and call it a day.

[albertogasparin]

@theKashey I think that was my hope with R18, but from my early tests it is not as aggressive as current RSS one. And that is also what batchUpdates = false ends up doing: skipping all RSS logic and let React do its thing.

BTW, I've done some more digging and seems like that the root cause is the conversion of the container from class to functional component (v2.7.0+). As part of that work I had to find a different solution for when triggering the store updates in case of a prop change. Before I was using getDerivedStateFromProps which is triggered before render, but such method is no longer available, so had to resort doing it mid render, where the React team says it might be optimised anyway. I assume however that is after React calculates who needs to be updated, so it does not combine Gamma with Delta.

This is a bit of a pickle... I could convert back to class component, could add a class wrapper (hello HOC) to handle this specific update case, or could say this is a React problem and @obweger has to deal with it 😅
I'm not excited about the first two because of the implications at Jira scale, and not sure if React deopts class components with concurrent. Wishing for another way 🤔

[theKashey]

resort doing it mid render, where the React team says it might be optimised anyway

Officially - the result of current render will be discarded and the new one will start. So "should be fine", but look like it's not.
And look like there are other nuances we need to be worry about:

• link https://codesandbox.io/p/devbox/sweet-state-batching-issue-forked-pydfcc?file=%2Fsrc%2FApp.tsx%3A40%2C6 (with added useEffect)
• output

Beta
Delta 4 3
Delta effect 
Delta effect x 4 3
Beta effect // ok, "effects" are resolved from the bottom up, probably this is why `getDerivedStateFromProps` was playing a better role
Gamma
Gamma effect
Delta 4 4
Delta effect x 4 4 // <--- no Delta Effect that should be called on every render 

[theKashey]

So there is one moment we've missed a little.
Original message states - "In React 17, as well as with defaults.batchUpdates = false, this works "synchronously", or in short - "works"
But there is an error in the console

Warning: Cannot update a component (`WiredGammaContainer`) while rendering a different component (`Container(1d8ny0s)`). To locate the bad setState() call inside `Container(1d8ny0s)`

Callstack leads to this line -

react-sweet-state/src/components/container.js

Line 140 in 0384e28

handlers.onContainerUpdate?.(

- in the FunctionContainer body.

As a result of such update results of a render are getting discarded and rendered "again". This is why you are experiencing the "continuous" flow of data - it just stops on every bump.

---

This creates a possible solution, that I would not be comfortable to implement:

• onUpdate should be scheduled
• and every container with onUpdate functionality
  • should delay all other store notifications going though it
  • in order to synchronise update propagation speed

Ie the goal is to first update container, then values inside it.

Using given primitives, where Container can be given any arguments this is quite challenging task with every Container becoming a gatekeeper for all communications.
#146 proposes a different approach, where relations between stores lives outside of React code, and we can manage them before invoking client-facing hooks (ie notify in two steps). Not sure that is a solution as #146 is quite sweet-state centric, and we might want to wire some variables from user space.

---

In this terms we have two options left:

• embrase multiple renders. This is almost React recomendation, but its really hard to follow it and detect moments of state tearing.
• use different setState implementation for during-render updates:
  • only for onContainerUpdate, we dont have any other case.
  • so it will setState in the current tick, but notify in the next (⬅️ this is a solution)
  • meanwhile update will continue executing Fiber and getSnapshot will read updated value from "current" store -

    react-sweet-state/src/components/hook.js

    Line 36 in 0384e28

    return stateSelector(state, propsArgRef.current);

  • when notify will trigger components will not re-render as snapshots are similar
  • if there is some memo stopped first update propagation, then it will continue in the next tick.

---

Look like this is the correction for R18 we are looking for. I was playing with #218 for quite a while, and pretty sure we dont need batching/schedule at all with React 18 autobatching.
Actually it really works better us interfering with React defaults, except this particular case where onUpdate causes synchronous update and warnings in React.

PS: technically setState in render body is ok, but it does like when update caused by forceStoreRerender. These warnings are just warnings.

[albertogasparin]

pretty sure we dont need batching/schedule at all

That was my initial hope too. Maybe it's still the case for batching, but not for scheduling. RSS currently "holds" updates for longer (almost like startTransition) and so removing that too causes additional re-renders in places where there are close state updates.
Happy to revisit and re-test, but that was the outcome when I tried removing both.

so it will setState in the current tick, but notify in the next (⬅️ this is a solution)

Can you write a POC? As we will be already in render phase, so getSnapshot should return the "old" value once, and then get fresh one on "next" re-render, but might be tricky with useSyncExternalStore

[theKashey]

but not for scheduling. RSS currently "holds" updates for longer

"Right now" RSS schedules only the first update flushing all others synchronously 😢 . And as a fix to that we can remove scheduler at all. Thanks to R18 scheduling any updates for us.

As we will be already in render phase, so getSnapshot should return the "old" value

According to the code it will return value from getShapshot and will execute it on every invocation regardless of receiving or not notification.
See https://github.com/facebook/react/blob/c5b9375767e2c4102d7e5559d383523736f1c902/packages/react-reconciler/src/ReactFiberHooks.js#L1505

• it's return getSnapshot
• setup useEffect to trigger fiber updates
• to state stored anywhere, except user-controlled getSnapshot

So if

if (retrieveStore(Store).storeState !== storeState) forceUpdate({});
const state = storeState.getState();

Will point to the "current state", and it probably will - then all consumers will be in sync

[albertogasparin]

It does not work because getSnapshot is not called as WiredGammaContainer does not re-render.

<AlphaContainer>
  <WiredBetaContainer> <- re-renders
    <WiredGammaContainer> <- no renders
      <Delta /> <- re-renders

React is "smart" about what should re-render, so in case of prop-less children it avoids any work. So WiredGammaContainer never gets the new state unless we notify it. And if we do it too late, it obviously complains that we are updating state during the render of another component

[theKashey]

so in case of prop-less children it avoids any work

Right, so as "children" is not changed, and it is not changed as it's defined a level above, update propagation is stopped.

🤔: Can we change code so WiredBetaContainer explicitly renders WiredGammaContainer? In this case children would be new and this optimisation will be bypassed. This will help us understand is tree composition affecting behavior or not.

Another moment to clarify is a behavior difference between useSyncExternalStoreShim and the one supplied with React 18. While they are quite similar I've found React 18 version to be much simpler that shim version. However I haven't found much difference in the "update" moment:

• React 18: https://github.com/facebook/react/blob/c5b9375767e2c4102d7e5559d383523736f1c902/packages/react-reconciler/src/ReactFiberHooks.js#L1709C4-L1709C4
• Shim: https://github.com/facebook/react/blob/main/packages/use-sync-external-store/src/useSyncExternalStoreShimClient.js#L120

🤔: So it's likely that migration from shim to native solution does not affect behaviour. However....

---

And if we do it too late, it obviously complains that we are updating state during the render of another component

It complains because we notify in during component render, and there is an interesting comment about it

https://github.com/facebook/react/blob/c5b9375767e2c4102d7e5559d383523736f1c902/packages/react-reconciler/src/ReactFiberWorkLoop.js#L764

Look like it's ok to call setState of a local hook (what is "local hook"?), but not ok to notify. However, while it will complain - it will merge lanes and schedule update?

This lures us to https://github.com/facebook/react/blob/c5b9375767e2c4102d7e5559d383523736f1c902/packages/react-reconciler/src/ReactFiberHooks.js#L3157 and enqueueRenderPhaseUpdate (https://github.com/facebook/react/blob/c5b9375767e2c4102d7e5559d383523736f1c902/packages/react-reconciler/src/ReactFiberHooks.js#L3351-L3364)

And it seems like an answer to the question - it will just "mark" affected component to drop it's render result and re-render with new data, aka "restart". In case of external update it will just mark it for further render.

---

So, what if we try to use useState based useSyncExternalStoreShim? There is no real need to import official shim as it can be replicated locally in a few lines of code.

What if....

[albertogasparin]

I've played around with this for another while. Here's an update:

• If we revert the class -> functional container conversion we can restore the atomic re-render. Using getDerivedStateFromProps + useSyncExternalStore and no scheduling we get a single update. 👍
• While looking more into it, I've discovered that useSyncExternalStore is not (and likely never will) concurrent mode compatible (eg: cannot use startTransition). The moment we replace useSyncExternalStore with a useEffect + useState it seems like we go back to Delta re-rendering twice with partial state 😞 👎

The latter limitation is a big bummer in case of large apps, because we are trading off a single blocking render (via useSyncExternalStore) vs smaller resumable re-renders.
We could support both (adding a defaults.updateMode: sync | concurrent) as an option, but wonder if being unable to use concurrent features is really a direction that makes sense.

So I'm currently unsure... My preference would likely be ditch the custom scheduling, ditch useSyncExternalStore and go back to a simpler model where we could use startTransition (explicitly or implicitly). But might have to check what that means from a performance point and what you @obweger could do 🤔

---

More info in the wild:

• Recoil allows consumers to pick, for now
• Jotay ditched uSES entirely
• React-redux is still looking into it

[theKashey]

According to reactwg/react-18#86 this is what was planned

That’s because if updates to stores are synchronous, they are guaranteed to be consistent.
So, in the new design:
Updates triggered by a store change will always be synchronous, even when wrapped in startTransition

This is also the major difference between useSyncExternalStore and useSyncExternalStoreShim -> the latter causes update by calling forceUpdatedriven byuseState`, and that logic is transition-friendly (https://github.com/facebook/react/blob/c5b9375767e2c4102d7e5559d383523736f1c902/packages/react-reconciler/src/ReactFiberHooks.js#L3163)

• returning getDerivedStateFromProps sounds like a good move. I always admired this function for it 🤷‍♂️ placement and stability - not a random imperative hook somewhere, but something in the designated spot
  • just to check - there are no plans/deopts for classes in React 18?
• still giving a shot to uSES shim. That is the biggest difference between R17 and R18

---

PS: Original react issue mentions the absense of getBackgroundState for Redux.

• that remind me one of @threepointone ideas about "freezing" Redux state, cannot find a link to the original
• but it inspired me to create https://github.com/theKashey/restate/tree/master/packages/react-redux-semaphore, used to "protect" / "freeze" UI branches from no longer valid states, and that package has...
• a link to official redux documentation stating

The original Flux pattern describes having multiple “stores” in an app, each one holding a different area of domain data. This can introduce issues such as needing to have one store “waitFor” another store to update. This is not necessary in Redux because the separation between data domains is already achieved by splitting a single reducer into smaller reducers.

So here we are - a bunch of small stored suffering from something Redux solved a whole ago, and look like #146 is more about "Redux" way of solving the problem - moving everything outside.

But still...

---

There are 3 possible ways of working:

• fully outside of React, like Redux. It's always a source of truth, and React has to adjust like rendering everything in sync
• fully inside, like Context, and React can handle it rendering waterfalls from update points
• not here and not there, like almost all state management libraries, mixing different approaches and experiencing hard to explain glitches

Switching to uSES shim seems to move us from 3 to 2. Not yet clear how one should read the value - directly from store as now, or via useState / useDeferred which should be "anything-friendly".
However this moment of "who reads who" can replace one sort of state tearing by another, and we might spent another quarter understanding our glitches.
However, having R18-related glitches to be understood right now, it might be a good timing to enter chaos state.