Skip to content

Commit

Permalink
fix useSelector race condition with memoized selector when dispatchin…
Browse files Browse the repository at this point in the history
…g in child components useLayoutEffect as well as cDM/cDU (#1536)

Co-authored-by: Tim Dorr <timdorr@users.noreply.github.com>
  • Loading branch information
dai-shi and timdorr committed Apr 19, 2020
1 parent 79f3d13 commit 2a5b12b
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 2 deletions.
6 changes: 5 additions & 1 deletion src/hooks/useSelector.js
Expand Up @@ -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
}
Expand All @@ -44,6 +47,7 @@ function useSelectorWithStoreAndSubscription(

useIsomorphicLayoutEffect(() => {
latestSelector.current = selector
latestStoreState.current = storeState
latestSelectedState.current = selectedState
latestSubscriptionCallbackError.current = undefined
})
Expand Down
38 changes: 37 additions & 1 deletion 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'
Expand Down Expand Up @@ -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 = {}
Expand Down

0 comments on commit 2a5b12b

Please sign in to comment.