setState() mutations and redux mutations are not batched into a single re-render

[mgummelt]

What is the current behavior?

See the codesandbox link below for a running example of the following description.

I have a redux store containing a single array of integers, elems, initialized to [0]. I have both an Outer component and an Inner component which are parameterized by elems via connect(). Inner stores an index into the array in this.state.index, initialized to 0. It also renders a button which, when clicked, will insert a new element into the redux and update this.state.index to be the index of the new elem.

When the button is pressed, the component is re-rendered twice. During the first re-render, this.state.index has changed, but not this.props.elems, so when I index the array, I receive an undefined result. The subsequent re-render works fine, however.

Note that if I remove the Outer component, and instead directly render Inner, there is no issue. Something about having both a parent and a child component subscribed to the same redux data is causing this issue.

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://codesandbox.io/s/eager-kepler-coo8g

What is the expected behavior?

I would expect render() to be called only once, with both this.state and this.props updated to their most recent values.

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?

See the codesandbox link above.

[timdorr]

The Codesandbox link you provided doesn't appear to have any of the code you mentioned in it. Can you update the link?

[mgummelt]

@timdorr: updated. didn't realize I needed to explicitly save.

[timdorr]

These updates aren't batched together by default. You'll need to do this via the batch API: https://react-redux.js.org/api/batch

[markerikson]

I was talking with him about this one in Reactiflux last night. I'd like to leave it open for the moment so I can remember to glance at it in the next few days, just to make sure I know what's happening here.

[mgummelt]

@markerikson Thanks!

@timdorr Even if they aren't batched, it seems that the ordering of the two re-renders aren't respecting React's dataflow from higher level to lower level components. The changes to elems should update InnerConnected, whereas the changes to this.state.index should update Inner, so shouldn't the former occur first? Just as Redux mutations must first update higher level components so that lower level components aren't rendered with partially stale data.

[timdorr]

Sorry, I was totally wrong about the batching.

This has something to do with React.memo. If you set pure: false on Inner, the problem goes away. There's still a double-render (which makes sense with the state and wrapper component updates), but the first render gives it up-to-date info from the store. Is this a React bug, maybe?

[mgummelt]

Huh, so any connected component that uses internal state should set pure: false? That's quite common. It seems like it should be documented more prominently, no?

[mgummelt]

I guess I should have started with a more basic question. Is my expectation of my provided event handler's behavior correct? Or should I account for the possibility that the setState mutation may race in front of dispatch?

[markerikson]

@mgummelt : per our chat discussion, I would have expected that the setState()-triggered update would get batched in with the Redux-triggered update. The fact that it doesn't seem to be is concerning to me.

[stkao05]

Very glad that I was not the only one experiencing this issue 😛I experienced this issue when I upgrade the react-redux from v5 to v7.

I tried to debug see why the redux store update only get passed down to the component in the later render call, and from the call stack it seems like the connected component was notified of store update only in the useIsomorphicLayoutEffect lifecycle (ref). I have not yet dug deep into the source code but I suspect that it could be related to the issue.

[markerikson]

@stkao05 : yeah, we cascade notifications down the tree to enforce top-down update behavior.

I don't have time to dig into this one myself atm, but if someone else can do so and report their findings, I'd appreciate it.

[afreix]

We are also experiencing the same issue where a set state call and redux dispatch are not batched into one re-render despite being called within an event handler

[ckedar]

I have spent some time digging into this issue and I will share my findings here:

The root cause of the issue is making the ConnectFunction a PureComponent by wrapping in React.memo().

Based on what I have read in blog, this step of making it pure was taken as part of performance optimization.

But this optimzation is too aggresive, rather improper because ConnectFunction consumes the state inside the store, which is 'external' to this component in the sense that it does not reflect in its props.

A workaround of using {pure:false} is available, but it is un-intuitive as your component that is being connected could be a genuine PureComponent but you will still encounter this bug.

Expectation when you are connecting your component is that your component will always be rendered with latest state from store, so this bug is breaking the basic promise of connect.

In v5.0.7, where Connect was a Class component, it was using shouldComponentUpdate to bail out of render when there is no change in component's props. As Connect is implemented as Function component in v7, there is no mechanism available to bail out a waistful render. I guess making it pure was opted as workaround for this needed bailout.

But this workaround is over-agressive. In v5.0.7's class component, componentWillReceiveProps, which is called in every render cycle, executes selector.run, which will re-evaluate mapStateToProps if either store state or wrapper props have changed. The bailout happens in shouldComponentUpdate only if merged props have not changed, so it is conditional as opposed to unconditional bailout in v7's function component wrapped in React.memo.

However, wrapping the ConnectFunction in React.memo gives a huge performance boost. Removing the React.memo() results in drastic drop in performance, almost to the level of v6. I have verified this with react-redux-benchmarks. So it seems there is no clean solution to this problem.

The main motiviation behind move from v5 to v6 and onwards, I believe, was to move from Legacy Context API to New Context API. Usage of function component is strictly not necessary to achieve this. So we could go back to Class component for Connect while using New Context API and continue to use shouldComponentUpdate bailout.

Of course this will bring back StrictMode warnings, but then question is what is more important - StrictMode compliance or bug free behaviour while keeping performance at acceptable level?

[markerikson]

@ckedar : thanks for digging into this! I appreciate the writeup.

FWIW, we're not going back to a class component at this point. Not only would it be yet another complete rewrite, the complexities with trying to use context values in a class make it infeasible. (I actually did try to put together a class-based approach while working on v7, and gave up on that.)

Part of the issue here is the tiered Subscription handling. A nested connected component will end up getting rendered later with the right store state, but given the current implementation, the setState()-queued update and the top-level connected components would end up being rendered together in the same batch, with the connect-queued render for this component coming shortly afterwards in a layout effect.

I'm not sure if there's a good solution for this issue. Overall, this is somewhat of an edge case. Worst comes to worst, we document as a known issue as we did with "zombie children" and hooks.

[markerikson]

Looks like #1313 is the same issue as this one.

[javascripter]

Not sure if this issue is the root cause, but it seems React Native's FlatList Pull To Refresh functionality is broken because of the slightly delayed propagation of reducer value change compared to component's local setState.

I created a minimal reproducible example below: you can see when using react-redux, Pull To Refresh feels shaky due to a slight delay of refreshing prop change after onRefresh.
https://github.com/javascripter/dispatch-vs-setstate-refreshcontrol-issue-repro

Edit: Link to snack is here (same code) https://snack.expo.io/ByN0nLBhS

I wonder if this is related to the issue discussed here?

[bergkvist]

I believe this might be related: visgl/deck.gl#4550

[bergkvist]

The issue I mentioned above does not appear if any of the following are used:

• class based component with this.setState()
• useState() hook
• React Context API

It does appear when:

• using react-redux
• using setTimeout(() => setState(newState), 0) // from useState hook

So from what I can see, it seems that react-redux might have some issues with Zero Delays allowing other code to run between a dispatch and a rerender.

[n-dragon]

with function component I managed to make it work by triggering a render of Outer and getting redux state with useStore , so that props of Inner are up to date because of the parent render and pulling last redux store data.
you may want to still use useSelector to subcribe to redux store change.

to sum up:

• force render of parent after dispatch so that child can have last props version
• get redux state using useStore
• still use useSelector to keep being notified about redux store change

[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.