Repository: reduxjs/react-redux
Pull request #1536 (2020-03-05 10:05:42)

fix useSelector race condition with memoized selector when dispatching in child components useLayoutEffect as well as cDM/cDU
This is the continuation of #1532.
It can be related with #1508 too.

This only happened if a parent component uses a selector with useCallback, and a child component dispatch an action in useLayouteffect (or componentDidMount / componentDidUpdate).

You can try how it works or fails with the test case by changing one of conditions: Removing useCallback, or changing useLayoutEffect to useEffect, with reverting the useSelector patch.

netlify[bot]: Deploy preview for *react-redux-docs* ready!

Built with commit 58a964cabe022f41ddb873f6f2c3520ed18d1a1b

https://deploy-preview-1536--react-redux-docs.netlify.com
dai-shi: codesandbox reproduction by @laxels : 
https://codesandbox.io/s/react-redux-useselector-bug-igp9l
laxels: @dai-shi There is actually still a tiny edge case bug left here, though it's much harder to trigger:

Check out this test case:
```
        store = createStore(c => c + 1, -1)
        const selectorA = c => Math.min(c, 1)
        const selectorB = c => c

        const Comp = () => {
          const selectorRef = useRef(selectorA)
          useEffect(() => {
            selectorRef.current = selectorB
          }, [])

          const count = useSelector(selectorRef.current)
          renderedItems.push(count)
          return <Child parentCount={count} />
        }

        const Child = ({ parentCount }) => {
          useLayoutEffect(() => {
            if (parentCount === 1) {
              store.dispatch({ type: '' })
            }
          }, [parentCount])
          return <div>{parentCount}</div>
        }

        rtl.render(
          <ProviderMock store={store}>
            <Comp />
          </ProviderMock>
        )

        // The first render doesn't trigger dispatch
        expect(renderedItems).toEqual([0])

        // This dispatch triggers another dispatch in useLayoutEffect
        rtl.act(() => {
          store.dispatch({ type: '' })
        })

        expect(renderedItems).toEqual([0, 1, 2])
```

The only difference between this and your test case is that the selector switches from `selectorA` to `selectorB` after the first render. Put another way, `selectorA` is only ever passed into `useSelector` the first time `Comp` is rendered.

Since the dispatch in `Child`'s `useLayoutEffect` actually fires before `lastSelector` is updated, it uses `selectorA` instead of `selectorB` to decide whether or not the new selected state is equal to the old one. Since `selectorA` caps at 1, it's equal to the old value and **does not trigger a re-render even though the store state has incremented from 1 to 2**.

It's caused by essentially the same timing issue. Another edge case which may also happen due to the same timing issue is that the selector errors in `checkForUpdates`, which causes the error to be recorded in `latestSubscriptionCallbackError`, only to be overwritten as `undefined` before the next render happens. The error is never surfaced as a result.

While I haven't figured out how to pass in a fresh `latestSelector` and `latestSelectedState` to `checkForUpdates` (this would fix the test case I posted), IMO the updates to the cache refs in the `useIsomorphicLayoutEffect` should not automatically assume that the values are guaranteed to be fresh. To build further on your solution, maybe we can do something like
```
useIsomorphicLayoutEffect(() => {
  if (store.getState() === storeState) {
    // update cache refs
  }
})
```
With this^, `checkForUpdates` would also need to ensure that the cache refs are all up-to-date to handle the case where updates are skipped in `useIsomorphicLayoutEffect`. I think the only thing we would need to add there is
```
latestStoreState.current = store.getState()
```
laxels: > Although I understand the edge case, we wouldn't need to take care of mutating selector outside render. We could simply claim it as a wrong usage.

It also applies to in-render mutations, though. For example, the super common usage of `useCallback`:
```
const Comp = ({maxVal}) => {
  const selector = useCallback(c => Math.min(c, maxVal), [maxVal]);
  const selected = useSelector(selector);
  ...
};
```
^ The above will also miss required re-renders. The problem is that `checkForUpdates` is using a stale `latestSelector` value even though **a render with a new selector has already happened**.


> Can you elaborate this please? This sounds like a realistic case, but I'm not certain.

Yet another contrived example, but it's easy to see how this one could happen randomly in the wild:
```
const Comp = () => {
  const selected = useSelector(() => {
    // Simulate an operation with a chance to error out
    if (Math.random() < 0.9) {
      return {};
    }
    throw new Error('Something went wrong');
  });
  ...
};
```

When the child dispatches in `useLayoutEffect` / `cDU`, `checkForUpdates` may encounter an error when running the selector and store it:
```
latestSubscriptionCallbackError.current = err
```
It then relies on the next render to throw the error for it. However, the `useIsomorphicLayoutEffect` code runs after `checkForUpdates` but before the next render, with the line
```
latestSubscriptionCallbackError.current = undefined
```
So the error is silently ignored.
dai-shi: > const selector = useCallback(c => Math.min(c, maxVal), [maxVal]);
> ^ The above will also miss required re-renders.

OK, I get it. Thanks for your patience.
We could run `checkForUpdates` in useIsomorphicLayoutEffect to cover such edge cases.

As I understand, this only happens when child components trigger dispatch in useLayoutEffect instead of useEffect. Class components are troublesome in this case, because there's no useEffect equivalent.

> It then relies on the next render to throw the error for it. However, the useIsomorphicLayoutEffect code runs after checkForUpdates but before the next render, with the line

Yeah, similar for this. We could keep the error in a scoped variable in render and check if it's changed in useIsomorphicLayoutEffect (and forceRender if changed.)
dai-shi: @laxels If you are interested, please check out #1509. The implementation doesn't use `useLayoutEffect`, so there's probably no race condition. I'm not pushing to merge it because we are waiting for `useMutableSource`.
dai-shi: @markerikson Can you take a look at this? I think the spec is also useful to evaluate `useMutableSource` based implementation.
timdorr: I think it looks good. I don't have any objections.
daiky00: @timdorr why haven't this been merge I need it :(
timdorr: I'd just like another input here and then we can merge this in.
markerikson: Seems reasonable.
timdorr: Thanks;

Patch:
diff --git a/src/hooks/useSelector.js b/src/hooks/useSelector.js
index 0195df2..ee4168a 100644
--- a/src/hooks/useSelector.js
+++ b/src/hooks/useSelector.js
@@ -21,16 +21,19 @@ function useSelectorWithStoreAndSubscription(
 
   const latestSubscriptionCallbackError = useRef()
   const latestSelector = useRef()
+  const latestStoreState = useRef()
   const latestSelectedState = useRef()
 
+  const storeState = store.getState()
   let selectedState
 
   try {
     if (
       selector !== latestSelector.current ||
+      storeState !== latestStoreState.current ||
       latestSubscriptionCallbackError.current
     ) {
-      selectedState = selector(store.getState())
+      selectedState = selector(storeState)
     } else {
       selectedState = latestSelectedState.current
     }
@@ -44,6 +47,7 @@ function useSelectorWithStoreAndSubscription(
 
   useIsomorphicLayoutEffect(() => {
     latestSelector.current = selector
+    latestStoreState.current = storeState
     latestSelectedState.current = selectedState
     latestSubscriptionCallbackError.current = undefined
   })


Test patch:
diff --git a/test/hooks/useSelector.spec.js b/test/hooks/useSelector.spec.js
index b0c8692..269a9f2 100644
--- a/test/hooks/useSelector.spec.js
+++ b/test/hooks/useSelector.spec.js
@@ -1,6 +1,6 @@
 /*eslint-disable react/prop-types*/
 
-import React, { useCallback, useReducer } from 'react'
+import React, { useCallback, useReducer, useLayoutEffect } from 'react'
 import { createStore } from 'redux'
 import { renderHook, act } from '@testing-library/react-hooks'
 import * as rtl from '@testing-library/react'
@@ -156,6 +156,42 @@ describe('React', () => {
         })
       })
 
+      it('works properly with memoized selector with dispatch in Child useLayoutEffect', () => {
+        store = createStore(c => c + 1, -1)
+
+        const Comp = () => {
+          const selector = useCallback(c => c, [])
+          const count = useSelector(selector)
+          renderedItems.push(count)
+          return <Child parentCount={count} />
+        }
+
+        const Child = ({ parentCount }) => {
+          useLayoutEffect(() => {
+            if (parentCount === 1) {
+              store.dispatch({ type: '' })
+            }
+          }, [parentCount])
+          return <div>{parentCount}</div>
+        }
+
+        rtl.render(
+          <ProviderMock store={store}>
+            <Comp />
+          </ProviderMock>
+        )
+
+        // The first render doesn't trigger dispatch
+        expect(renderedItems).toEqual([0])
+
+        // This dispatch triggers another dispatch in useLayoutEffect
+        rtl.act(() => {
+          store.dispatch({ type: '' })
+        })
+
+        expect(renderedItems).toEqual([0, 1, 2])
+      })
+
       describe('performance optimizations and bail-outs', () => {
         it('defaults to ref-equality to prevent unnecessary updates', () => {
           const state = {}