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

Jump to bottom

Fix useSelector race condition when dispatch happens in child's useLayoutEffect or componentDidUpdate #1532

Closed
wants to merge 1 commit into from
Closed

Fix useSelector race condition when dispatch happens in child's useLayoutEffect or componentDidUpdate #1532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/hooks/useSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,9 @@ function useSelectorWithStoreAndSubscription(
throw err
}

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

@dai-shi dai-shi Mar 2, 2020 •
edited

Choose a reason for hiding this comment

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

Updating refs in the effect is intentional for concurrent mode.
Do you think this case can be solved by setting the initial ref values?

  const latestSubscriptionCallbackError = useRef()
  const latestSelector = useRef(selector)
  const latestSelectedState = useRef(null)
  if (latestSelectedState.current === null) {
    // lazy ref initialization
    latestSelectedState.current = selector(store.getState())
  }

(But, this doesn't work in the case that selectedState can be null. Should we use a special value?)

Copy link
Author

Choose a reason for hiding this comment

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

Updating refs in the effect is intentional for concurrent mode.
Do you think this case can be solved by setting the initial ref values?

  const latestSelector = useRef(selector)
  const latestSelectedState = useRef(selectedState)

Ah, is that so? I'm not yet aware of the interactions with concurrent mode. I'll try to provide a solution which keeps it in the useIsomorphicLayoutEffect then.

Unfortunately, I don't think setting initial ref values would do anything in this context. The problem is that this block, which is logically updating the selectedState cache to its newest value, is using a potentially stale variable. This can happen long after the initial render phase.

Copy link
Contributor

Choose a reason for hiding this comment

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

This can happen long after the initial render phase.

I see. I misunderstood the issue then. 😥


useIsomorphicLayoutEffect(() => {
function checkForUpdates() {
Expand Down
43 changes: 43 additions & 0 deletions test/hooks/useSelector.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,49 @@ describe('React', () => {

expect(renderedItems).toEqual([0, 1])
})

it('works properly with dispatch from componentDidUpdate', () => {
store = createStore(c => c + 1, -1)

const Comp = () => {
const selector = useCallback(c => c, [])
const value = useSelector(selector)
renderedItems.push(value)
return <CompClass val={value} />
}

class CompClass extends React.PureComponent {
constructor(props) {
super(props)
this.dispatched = false
}
componentDidUpdate() {
if (this.dispatched) {
return
}
store.dispatch({ type: '' })
this.dispatched = true
}
render() {
return <div />
}
}

rtl.render(
<ProviderMock store={store}>
<Comp />
</ProviderMock>
)

// The first render doesn't trigger componentDidUpdate
expect(renderedItems).toEqual([0])

// This dispatch forces Comp and CompClass to re-render,
// triggering componentDidUpdate and dispatching another action
store.dispatch({ type: '' })

expect(renderedItems).toEqual([0, 1, 2])
})
})

describe('performance optimizations and bail-outs', () => {
Expand Down