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.

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

add deps to useSelector to allow controlling stability of selector #1251

Merged
merged 2 commits into from
Apr 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 12 additions & 4 deletions src/hooks/useSelector.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,14 @@ const useIsomorphicLayoutEffect =
/**
* A hook to access the redux store's state. This hook takes a selector function
* as an argument. The selector is called with the store state.
*
* This hook takes a dependencies array as an optional second argument,
* which when passed ensures referential stability of the selector (this is primarily
* useful if you provide a selector that memoizes values).
*
* @param {Function} selector the selector function
* @param {any[]} deps (optional) dependencies array to control referential stability
* of the selector
*
* @returns {any} the selected state
*
Expand All @@ -35,7 +41,7 @@ export const CounterComponent = () => {
}
```
*/
export function useSelector(selector) {
export function useSelector(selector, deps) {
invariant(selector, `You must pass a selector to useSelectors`)

const { store, subscription: contextSub } = useReduxContext()
Expand All @@ -46,13 +52,15 @@ export function useSelector(selector) {
contextSub
])

const memoizedSelector = useMemo(() => selector, deps)

const latestSubscriptionCallbackError = useRef()
const latestSelector = useRef(selector)
const latestSelector = useRef(memoizedSelector)

let selectedState = undefined

try {
selectedState = latestSelector.current(store.getState())
selectedState = memoizedSelector(store.getState())
Copy link
Contributor Author

@MrWolfZ MrWolfZ Apr 22, 2019

Choose a reason for hiding this comment

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

using latestSelector.current was wrong, since it was a stale selector. This change fixes that issue, which makes the test for stale props succeed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hah, nice catch :)

} catch (err) {
let errorMessage = `An error occured while selecting the store state: ${
err.message
Expand All @@ -70,7 +78,7 @@ export function useSelector(selector) {
const latestSelectedState = useRef(selectedState)

useIsomorphicLayoutEffect(() => {
latestSelector.current = selector
latestSelector.current = memoizedSelector
latestSelectedState.current = selectedState
latestSubscriptionCallbackError.current = undefined
})
Expand Down
39 changes: 31 additions & 8 deletions test/hooks/useSelector.spec.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*eslint-disable react/prop-types*/

import React from 'react'
import React, { useReducer } from 'react'
import { createStore } from 'redux'
import * as rtl from 'react-testing-library'
import { Provider as ProviderMock, useSelector } from '../../src/index.js'
Expand Down Expand Up @@ -165,15 +165,40 @@ describe('React', () => {

expect(renderedItems.length).toBe(1)
})

it('re-uses the selector if deps do not change', () => {
let selectorId = 0
let forceRender

const Comp = () => {
const [, f] = useReducer(c => c + 1, 0)
forceRender = f
const renderedSelectorId = selectorId++
const value = useSelector(() => renderedSelectorId, [])
renderedItems.push(value)
return <div />
}

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

rtl.act(forceRender)

// this line verifies the susbcription callback uses the same memoized selector and therefore
// does not cause a re-render
store.dispatch({ type: '' })

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

describe('edge cases', () => {
it('ignores transient errors in selector (e.g. due to stale props)', () => {
// TODO Not sure this test is really testing what we want.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was correct in what it tested. In fact, it failing showed an error in the implementation.

// TODO The parent re-renders, which causes the child to re-run the selector anyway and throw the error.
// TODO Had to flip the assertion for now. Probably needs to be rethought.

const spy = jest.spyOn(console, 'error').mockImplementation(() => {})

const Parent = () => {
const count = useSelector(s => s.count)
return <Child parentCount={count} />
Expand All @@ -197,9 +222,7 @@ describe('React', () => {
</ProviderMock>
)

expect(() => store.dispatch({ type: '' })).toThrowError(
/while selecting the store state/
)
expect(() => store.dispatch({ type: '' })).not.toThrowError()

spy.mockRestore()
})
Expand Down