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.

[salvoravida]

Yes I know. But here the problem is that if you have 100 nested connect component, you will see 100 commit in profile. Batched updates get only the flat connected component on each level.

[markerikson]

Right, again, that's my point.

With how the Subscription batching works, there will be multiple commits in a real component tree, but they should get batched into roughly 1 commit per level of nesting.

With React-Redux v5, which used the same Subscription class but no batching, each individual component ends up as a separate commit. The render passes for those individual components are smaller, but React has to do more of them.

[salvoravida]

Right the problem is that. Batching is only per level. Not for all tree interested of updates. So if we have only a tree of connected component there will be no batch. Is that an expected behavior of unstable batched updates? Hum.

[markerikson]

I'm not sure what you're saying there.

Specific example. Let's say we have a tree of connected components that looks like this:

• A
• B
  • C
  • D
    • E
      • F
    • G

Let's also assume, for this example, that all components are connected and reading a single counter value out of the store, but not passing any props to their children.

<Provider> has subscribed to the store. On dispatch:

• Provider's Subscription notifies A and B. Their re-render is batched.
• After B renders, its Subscription notifies C and D in the commit phase. Their re-render is batched, but executed synchronously.
• After D renders, its Subscription notifies E and F. Their re-render is batched. We're still technically in the commit phase for B, so it runs synchronously.
• After E renders, F is notified, and it also renders synchronously.

So, I would expect 4 total commits from that tree.

[salvoravida]

right. that's exact what @OliverJAsh means in his "Nested connect updates are not batched"

so 1 store update 4 commit. neste batched due to nested subscription are not batched into 1 commit.
that's is done for stale props issue, but slow down normal behviour.

for example in that Tree A-G using useSelector there will be only 1 commit, and ONLY if in that tree there is some parent steal props there will be a re-commit.

this is the point.

[markerikson]

And I'm saying there's two different aspects here:

• Whether nested updates at the same level are batched
• Whether nested updates at different levels are batched

We currently do the first, but not the second.

[markerikson]

I'm going to close this as a WONTFIX for now, largely because we're encouraging folks to stop using connect (even though it'll be fully supported in v8). This is a real bit of behavior that I can understand is annoying, but I don't think there's a meaningful change we can make at this point, and I don't intend to spend time fixing it myself.

If someone still feels strongly about this please comment, or even better file a PR to improve behavior here.