Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A possible idea towards fixing useSelector tearing in Concurrent Mode #1509

Closed
wants to merge 3 commits into from
Closed

A possible idea towards fixing useSelector tearing in Concurrent Mode #1509

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
76a1d08
Fix useSelector tearing in Concurrent Mode
dai-shi Jan 27, 2020
32481c9
combine the idea used in #1505
dai-shi Jan 27, 2020
0768b45
read fresh store state on dispatch
dai-shi Jan 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 30 additions & 50 deletions src/hooks/useSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { useReducer, useRef, useMemo, useContext } from 'react'
import { useReducer, useEffect, useMemo, useContext } from 'react'
import { useReduxContext as useDefaultReduxContext } from './useReduxContext'
import Subscription from '../utils/Subscription'
import { useIsomorphicLayoutEffect } from '../utils/useIsomorphicLayoutEffect'
import { ReactReduxContext } from '../components/Context'

const refEquality = (a, b) => a === b
Expand All @@ -12,63 +11,44 @@ function useSelectorWithStoreAndSubscription(
store,
contextSub
) {
const [, forceRender] = useReducer(s => s + 1, 0)
const [state, dispatch] = useReducer(
(prevState, storeState) => {
const nextSelectedState = selector(storeState)
if (equalityFn(nextSelectedState, prevState.selectedState)) {
// bail out
return prevState
}
return {
selector,
storeState,
selectedState: nextSelectedState
}
},
store.getState(),
storeState => ({
selector,
storeState,
selectedState: selector(storeState)
})
)

const subscription = useMemo(() => new Subscription(store, contextSub), [
store,
contextSub
])

const latestSubscriptionCallbackError = useRef()
const latestSelector = useRef()
const latestSelectedState = useRef()

let selectedState

try {
if (
selector !== latestSelector.current ||
latestSubscriptionCallbackError.current
) {
selectedState = selector(store.getState())
} else {
selectedState = latestSelectedState.current
}
} catch (err) {
if (latestSubscriptionCallbackError.current) {
err.message += `\nThe error may be correlated with this previous error:\n${latestSubscriptionCallbackError.current.stack}\n\n`
let selectedState = state.selectedState
if (state.selector !== selector) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this whole if even required anymore? the reducer above always uses the latest selector during rendering, which means that state.selectedState should already have been computed with the latest selector, no? tbh, I am not sure I fully understand the "cheat mode" yet and when exactly the reducer is called with what values, so I may be wrong here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I happened to notice a bug, and fixed. 0768b45
It probably better explains why this if is required.
The rule of thumb is never read store state in render. Update it through React state.

Honestly, I don't fully understand it either and it behaves a bit differently from what I expected: The reducer seems to be called before render with various values in CM. That's probably why this if is required.

This actually fixes the tearing issue as far as my tool goes. I'd admit that there might still be edge cases not covered. Hope someone could help it here.

const nextSelectedState = selector(state.storeState)
if (!equalityFn(nextSelectedState, state.selectedState)) {
selectedState = nextSelectedState
// schedule another update
dispatch(state.storeState)
}

throw err
}

useIsomorphicLayoutEffect(() => {
latestSelector.current = selector
latestSelectedState.current = selectedState
latestSubscriptionCallbackError.current = undefined
})

useIsomorphicLayoutEffect(() => {
function checkForUpdates() {
try {
const newSelectedState = latestSelector.current(store.getState())

if (equalityFn(newSelectedState, latestSelectedState.current)) {
return
}

latestSelectedState.current = newSelectedState
} catch (err) {
// we ignore all errors here, since when the component
// is re-rendered, the selectors are called again, and
// will throw again, if neither props nor store state
// changed
latestSubscriptionCallbackError.current = err
}

forceRender({})
}

useEffect(() => {
const checkForUpdates = () => dispatch(store.getState())
subscription.onStateChange = checkForUpdates
subscription.trySubscribe()

Expand Down