Dispatch more than once in non-batched update cause incorrect result with useSelector

[malash]

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

Report a bug.

What is the current behavior?

I'v created a reproducing repo here https://github.com/malash/react-redux-issue-ref/blob/master/reduxBug.js

This repo use both react-redux and react-native.

<Button
  title="[BUG] Click Me (setTimeout)"
  onPress={() => {
    setTimeout(() => {
      dispatch({type: 'NOOP'});
      dispatch({type: 'TOGGLE'});
    }, 0);
  }}
/>

When using setTimeout's callback to dispatch multi times, the useSelector return incorrect value. The reason why I use setTimeout is to ensure these dispatch calling are non-batched.

const selector = state => ({
	bool: state.bool,
});

const ReduxBugParent = () => {
  const {bool} = useSelector(selector);
  // ...
  <Text>bool from useSelector is {JSON.stringify(bool)}</Text>
  <Text>bool from store.getState is {JSON.stringify(boolFromStore)}</Text>
}

After click the button different selector result are shown. the bool are always different with boolFromStore.

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:

1. Clone the repo https://github.com/malash/react-redux-issue-ref
2. Install React Native CLI
3. Run react-native run-ios
4. Click buttons and see rendered values.

What is the expected behavior?

This bug can only reproduce on non-web environment, maybe it is related to #1436

I believe it should works on React Native as well as React DOM.

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?

react@16.9.0
react-native@0.61.2
react-redux@7.1.1
redux@4.0.4

cc

@byunicorn

[malash]

I'v created a PR #1438 and it seems fixed the issue.

[markerikson]

Per my comment in the PR, that change is not correct.

Can you clarify what "incorrect value" you're seeing here?

[markerikson]

Looking at your code example, I'm not convinced that you're seeing buggy behavior. There are multiple complex aspects for how batching behaves.

If you can provide some specific details on what you are expecting to see, and what is happening, it would help me understand what's going on.

[malash]

I implement the same demo in web. https://codesandbox.io/s/silly-browser-h5z4z

As you can see after button clicked, useSelector(selector).bool should always equals to store.getState().bool. But it doesn't equal in React Native demo.

[timdorr]

So, yes, useSelector provides state from the store at different times than when it exists within the store. This is unavoidable, because React behaves in a semi-asynchronous manner right now (i.e., setState is not synchronous) and will be even more async-y with Concurrent Mode.

However, I don't classify this as a bug. It's just a behavior of the library. We conform to React's update process so that we can achieve reasonable performance. This may create point-in-time inconsistencies between different APIs to access state, but I would posit that you are also using those APIs inconsistently by mixing and matching them in the same context (a React component).

If you're wanting to use useSelector, you should only use that, so the results are consistent. Going around it and accessing state directly isn't expected and advised, so this isn't something we're going to fix or change.

[markerikson]

I am curious what's going on if there are truly differences in behavior between native and web.

[malash]

@timdorr I think you closed the issue too early. I advice you to try the React Native demo even it may cost you some minutes to make environment ready.

I don not intentionally use store.getState() in any production code. I use it only to show that the result of useSelector was not consistent with what in store. Even you can remove that line the bug still exists.
The real problem is once you click the button, which contain "[BUG] Click Me (setTimeout)", the bool should toggle.

In react-dom demo https://codesandbox.io/s/silly-browser-h5z4z
false => ( click button ) => true => ( click button ) => false => ...

But in react-natove demo https://github.com/malash/react-redux-issue-ref
false => ( click button ) => false => ( click button ) => true => ( click button ) => false => ...

I believe the GIF does show how the boolean changed.

@markerikson I agree with your comment in #1438 .

I guess if react-redux still use useLayoutEffect in non-web environment could fix this issue. Meanwhile we should find another way to solve the SSR warning #1402

[malash]

@timdorr @markerikson Could you please revisit this issue ? There must be some bugs in react-redux.

[markerikson]

@malash : I'm not going to say that React-Redux is perfect, but no, I don't see evidence here that things are buggy with React-Redux itself.

If you can dig into this further and provide more evidence that something may be going wrong, we can take another look. In particular, I would suggest trying to add some kind of logging to RN's batching implementation and see if it's being hit properly.

[MingruiZhang]

Hey @markerikson @timdorr We believe this is still an issue with react-redux. And it is actually noted in your own code.
https://github.com/reduxjs/react-redux/blob/master/src/hooks/useSelector.js#L14-L27 Here in the comment your code explicitly said that

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

You use useEffect on the server because React currently throws a warning and it's a no-op. However checking if window exists is problematic because other react platforms (like react-native) does not have window on it's client as well!

We think this is a still an issue, and I would propose we find a better gating for node environment than using window.

cc @MrWolfZ who implemented #1248 originally.

[markerikson]

@MingruiZhang : I'm not sure what problem you're trying to point to here, or how the referenced lines relate. Can you clarify?

While I haven't used RN, this post seems to suggest that window does exist in an RN environment:

https://stackoverflow.com/questions/49911424/what-does-the-variable-window-represent-in-react-native

[MingruiZhang]

While I haven't used RN, this post seems to suggest that window does exist in an RN environment:

https://stackoverflow.com/questions/49911424/what-does-the-variable-window-represent-in-react-native

It's possible that RN has window, but I don't think it has window.document and window.document.createElement which your code checks on. The other way I can reproduce this issue is by running it in a web environment and manually go inside node_modules to update useIsomorphicLayoutEffect from useLayoutEffect to useEffect

@MingruiZhang : I'm not sure what problem you're trying to point to here, or how the referenced lines relate. Can you clarify?

The main issue here is that we have 2 useIsomorphicLayoutEffects in the useSelector implementation.
The first one https://github.com/reduxjs/react-redux/blob/master/src/hooks/useSelector.js#L69-L73 - useIsomorphicLayoutEffect#1
The second one https://github.com/reduxjs/react-redux/blob/master/src/hooks/useSelector.js#L75-L102 - useIsomorphicLayoutEffect#2.

Now the second one listens to store and subscription update, so whenever store updates, useIsomorphicLayoutEffect#2 happens first, then it triggers a component forceRender, render happens next, lastly useIsomorphicLayoutEffect#1 happens.

In the given scenario, 2 sequential dispatch takes place (we used setTimeout to prevent it getting batched).

1. When useIsomorphicLayoutEffect=useLayoutEffect, everything happens in the order we expected it. Because useLayoutEffect is sync.

Dispatch1 useIsomorphicLayoutEffect#2 -> Dispatch1 render -> Dispatch1 useIsomorphicLayoutEffect#1 -> Dispatch2 useIsomorphicLayoutEffect#2 -> Dispatch2 render -> Dispatch2 useIsomorphicLayoutEffect#1.

2. Now when useIsomorphicLayoutEffect=useEffect, . Because useEffect is async., this is where the ordering start to go wrong.

Dispatch1 useIsomorphicLayoutEffect#2 -> Dispatch1 render -> Dispatch2 useIsomorphicLayoutEffect#2 -> Dispatch1 useIsomorphicLayoutEffect#1 -> Dispatch2 render -> Dispatch2 useIsomorphicLayoutEffect#1.

Noticed that because useEffect is async, Dispatch1 useIsomorphicLayoutEffect#1 was delayed and it happened after Dispatch2 useIsomorphicLayoutEffect#2, causing the state correctly updated from Dispatch2 useIsomorphicLayoutEffect#2 to be turned back wrong by Dispatch1 useIsomorphicLayoutEffect#1.

@markerikson does this makes sense to you? Any sequential updates from non-web platforms will suffer from this issue, and it would be really helpful if we can get a fix here.
cc @malash @byunicorn @MrWolfZ @timdorr

[markerikson]

Hmm. Okay, yeah, if that's the case, I can see how this would be a problem.

So... is there a better solution for detecting Node vs RN vs web?

This SO answer suggests checking navigator.product:

https://stackoverflow.com/questions/39468022/how-do-i-know-if-my-code-is-running-as-react-native

[dai-shi]

@MingruiZhang
Per your comment, the original implementation

react-redux/src/hooks/useSelector.js

Lines 15 to 16 in 91116ac

	const useIsomorphicLayoutEffect =
	typeof window !== 'undefined' ? useLayoutEffect : useEffect

seems to solve the issue. Correct?

[markerikson]

The additional checks were added as part of #1283 , to keep the SSR warning from happening if window has been polyfilled. So no, I don't think that's sufficient.

Ironically, someone actually warned about the RN issues here:

#1283 (comment)

I would like us to make a few changes as part of fixing this:

• Extract the definition of useIsomorphicLayoutEffect to a separate util file
• Make sure we check for an RN environment as part of the decision
• Figure out how to run our unit tests in an RN environment as well (which somehow has to include pulling in batchedUpdates.native.js when used with Jest)

[dai-shi]

Thanks for the clarification!

facebook/react#14927 (comment)

And yes, all these shenanigans wouldn't be necessary if this warning just wasn't printed in the first place.

Totally agreed with this.

A crazy idea: Can we simply tolerate the warning? Mitigation with docs?

[MingruiZhang]

@markerikson Appreciate reopening this issue!

I want to point out that we found this issue while we work on building WeChat Miniapp, with our own react renderer, we fall into the same situation as RN not having window.document.createElement. So we would really appreciate if the solution can consider other custom react renderers as well here. Maybe we can come up with a unified global variable namespace we we can manually set?

I totally agree that maybe React shouldn't throw these warnings in the first place, but I guess that's not to our hands to decide. Few other thoughts I have here

1. To what @dai-shi suggested, can we tolerate the warning?
2. I'm guessing React implements this warning to it's renderToString method to justify ssr, is there any flags we can read here?
3. If none of these works, can we provide an api for user to decider what effect hook they can use? Maybe as a third param to the useSelector api.

[markerikson]

The problem is we also rely on this same useIsomorphicLayoutEffect() snippet in connect as well, so it's not just useSelector, and we're not going to add an option to it just to configure that. The correct behavior here is useLayoutEffect.

And I'd really rather not have the SSR warnings be a thing again either.

[MingruiZhang]

@markerikson I totally agree with you. I think it's not ideal as well.

I'm also a bit confused on the whole logic inside useSelectorWithStoreAndSubscription, why do we need an latestSelectedState instance instead of just always use store.getState()? The store is always ensured to be correct, additional state management makes it vulnerable.

[markerikson]

latestSelectedState.current is the most recently derived value, not the entire store state.

I think I've got a fix in place for this, and some unit tests to match. I'm going to publish a test version shortly and will let you know when it's up.

[markerikson]

Just published v7.1.2-alpha.0 as react-redux@fix-test. Can folks try this out and let me know if it fixes the issue?

[bausmeier]

We ran into this issue today and the v7.1.2-alpha.0 release fixes it for us. Perfect timing 💯