Nested `connect` updates are not batched

[OliverJAsh]

Do you want to request a feature or report a bug?

(If this is a usage question, please do not post it here—post it on Stack Overflow instead. If this is not a “feature” or a “bug”, or the phrase “How do I...?” applies, then it's probably a usage question.)

Bug

What is the current behavior?

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to a CodeSandbox (https://codesandbox.io/s/new) or RN Snack (https://snack.expo.io/) example below:

https://stackblitz.com/edit/react-redux-batching-xkhtq8?file=index.tsx

What is the expected behavior?

1. Open https://react-redux-batching-xkhtq8.stackblitz.io/
2. Open dev tools -> React profiler
3. Click "reload and start profiling"
4. After reload, end profiling

Expected: 2 commits:

1. First render
2. Counter and ReduxCounter update, triggered via dispatch inside Counter#componentDidMount

Actual: 3 commits:

1. First render
2. Counter update, triggered via dispatch inside Counter#componentDidMount
3. ReduxCounter update, triggered via dispatch inside Counter#componentDidMount

IIUC, there is no reason why 2 and 3 can not be batched into a single React re-render?

Which versions of React, ReactDOM/React Native, Redux, and React Redux are you using? Which browser and OS are affected by this issue? Did this work in previous versions of React Redux?

Update: note this issue does not occur when using useSelector. https://stackblitz.com/edit/react-redux-batching-zb1wxv?file=index.tsx

[markerikson]

My first guess is that it's specifically because you're dispatching in componentDidMount. When you queue a React state update in cDM, React has to synchronously re-render, immediately, before the browser can re-paint.

What happens if you move the call into a single button click, instead?

[OliverJAsh]

Unfortunately the same happens inside a click handler: https://stackblitz.com/edit/react-redux-batching-nodjcz?file=index.tsx

[markerikson]

Hmm. Somewhat surprised by that.

That said, all the interactions between React's rendering, React's batching, and how React-Redux is implemented make this a very complex topic.

Is there a particular issue you're trying to solve, or was this more of a "why does it work this way?" kind of question?

[OliverJAsh]

I'm profiling React re-renders on unsplash.com, to try to improve our Time to Interactive, and I noticed that there are many more React re-renders than I had expected. I reduced the issue down to this.

My hope is that I can batch those into a single re-render, to improve our TTI.

[markerikson]

So the second thought here is that it has to do with how React-Redux is implemented internally.

We have to enforce top-down updates, and use an internal Subscription class to make that work. Updates get cascaded down the tree - a connected component will re-render, then notify the next level of subscribers below itself to try running mapState themselves. Each Subscription instance does use unstable_batchedUpdates() internally to batch updates queued by those subscribers. However, all that does run synchronously as part of a useLayoutEffect, and React has to immediately re-render as a result.

If I add a second instance of <ReduxCounterEnhanced> to the app and click "run", I still see two commits, with the second commit featuring both ReduxCounterEnhanced components updated together.

I'm guessing that if you add a third level of nesting, you'd see a third commit. In other words, there will be N commits per dispatch, roughly equating to the depth of the component tree, as the nested connected components cascade the subscription notifications downwards.

Just for kicks, you might want to try setting up an example app with 4 or 5 levels of nesting, then try doing profile comparisons between React-Redux v5, v6, and v7, to see how they behave.

[OliverJAsh]

@markerikson Thanks for the explanation. That makes sense.

However, all that does run synchronously as part of a useLayoutEffect, and React has to immediately re-render as a result.

This was the part I was missing.

I tried patching React Redux to switch useLayoutEffect to useEffect and then there was only 1 commit.

I'm now wondering why we need to use useLayoutEffect instead of useEffect given that it prevents React re-renders from properly batching all the way down the tree. I noticed this comment in the code:

react-redux/src/utils/useIsomorphicLayoutEffect.js

Lines 5 to 10 in 77a2044

	// useLayoutEffect in the browser. We need useLayoutEffect to ensure the store
	// subscription callback always has the selector from the latest render commit
	// available, otherwise a store update may happen between render and the effect,
	// which may cause missed updates; we also must ensure the store subscription
	// is created synchronously, otherwise a store update may occur before the
	// subscription is created and an inconsistent state may be observed

I'll have to chew on that for awhile 🤔

[markerikson]

Yes, exactly as that comment points out. In fact, we recently had a bug where we failed to correctly call useLayoutEffect in ReactNative and fell back to useEffect instead, and this led to multiple folks reporting inconsistent behavior. See #1444 , #1313 , and #1437 .

Note that React-Redux v6 was "better" in this regard because everything was done as part of React's normal rendering process, and thus we got top-down behavior for free without any of this subscription work. But, because all that work was being done while rendering, and state propagation was happening via context, the overall work of rendering was much slower, and thus we had to rewrite things for v7 (per #1177 ).

[salvoravida]

@markerikson Thanks for the explanation. That makes sense.

However, all that does run synchronously as part of a useLayoutEffect, and React has to immediately re-render as a result.

This was the part I was missing.

I tried patching React Redux to switch useLayoutEffect to useEffect and then there was only 1 commit.

I'm now wondering why we need to use useLayoutEffect instead of useEffect given that it prevents React re-renders from properly batching all the way down the tree. I noticed this comment in the code:

react-redux/src/utils/useIsomorphicLayoutEffect.js

Lines 5 to 10 in 77a2044

	// useLayoutEffect in the browser. We need useLayoutEffect to ensure the store
	// subscription callback always has the selector from the latest render commit
	// available, otherwise a store update may happen between render and the effect,
	// which may cause missed updates; we also must ensure the store subscription
	// is created synchronously, otherwise a store update may occur before the
	// subscription is created and an inconsistent state may be observed

I'll have to chew on that for awhile 🤔

are you sure? it seems very strange, because useEffect has the same commit phases but with async beetween commits.

[salvoravida]

@markerikson after some debug, it seem that batchUpdates as no effect when "setting states on" didUpdate/layouyEffect because the flow is
first Component re-render -> commitWork -> execute didUpdate/layoutEffect -> notifyNestedSub and so on.

while this flow resolve the selector stale props issue, it cannot de-facto "batch-updates!"

useSelector should not have this problem, as there is no nested-subscription. (if all React elements are using only useSelector (no connect))

i'm quite sure that nested-subscription invalidate "batch-updates".

hum, maybe there is no way to resolve BOTH stale props and batched updates using connect.

my opinion, is that stale props issue should be resolved on next-render, while
batching all store updates should have priority.

but anyway connect is de facto deprecated with useSelector, so this is legacy issue.

:)

[salvoravida]

@OliverJAsh @markerikson
https://codesandbox.io/s/batchedupdates-uselayouteffect-evj8s

i made a sample without redux and react-redux.

it seems a issue or expected behavior of unstable_batchedUpdates

[salvoravida]

facebook/react#17970

[markerikson]

are you sure? it seems very strange, because useEffect has the same commit phases but with async beetween commits.

Yes, because it's that "async" aspect that is the issue.

Imagine this sequence:

1. Component mounts
2. Action is dispatched and subscribers are synchronously notified
3. useEffect runs in a timeout shortly thereafter, and the component now subscribes

In that case, the newly-mounted component has missed the notification that an action was dispatched, and won't re-render correctly. That's exactly the bug that I was talking about that we just had with RN due to mis-configured feature detection.

but anyway connect is de facto deprecated with useSelector, so this is legacy issue.

No, connect is absolutely not deprecated in any way. We will continue to support it indefinitely.

[markerikson]

@salvoravida : replied over in the React issue, but pasting here for context:

The sandbox isn't actually testing the behavior you think it is.

First, React already wraps all event handlers in unstable_batchedUpdates(). That's why multiple queued state updates in a single handler result in one re-render. So, the manual call to unstable_batchedUpdates() does nothing there.

Second, either way, that button click event handler is only queueing one state update, not multiple, so there's no change in behavior.

Third, each of those child components is adding an additional state update in the commit phase, but that's after the batchedUpdates() wrapper has already completed, so it's not like there's any way that could get batched.

On the other hand, what React-Redux does is call unstable_batchedUpdates(), in the commit phase, as multiple nested child components are notified and queue state updates. Very different logic.

[salvoravida]

Sorry I do not think so. React redux call notifynestedsub under uselayouteffecct that is exact the problem of this issue. Open profile of this original nested redux update with connect. I have debuged them.

[markerikson]

What you're missing is the last part of my previous comment. Subscription wraps the additional nested state calls in the commit phase in unstable_batchedUpdates(). Your sandbox doesn't do that, and doesn't replicate the actual structure of connect.