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