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

Conversation

MrWolfZ
Copy link
Contributor

@MrWolfZ MrWolfZ commented Apr 22, 2019

This PR adds a deps parameter to useSelector as discussed here.

It also fixes an issue with staleness of the selector.

@MrWolfZ MrWolfZ changed the base branch from master to v7-hooks-alpha April 22, 2019 14:18
})

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.

@netlify
Copy link

netlify bot commented Apr 22, 2019

Deploy preview for react-redux-docs ready!

Built with commit 143ca4e

https://deploy-preview-1251--react-redux-docs.netlify.com


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 :)

@Dudeonyx
Copy link

If no dependency array is passed what happens?

@markerikson
Copy link
Contributor

@Dudeonyx : the latest passed-in selector should get used, instead of a previous one.

@markerikson
Copy link
Contributor

LGTM. I'll try to get alpha.1 published shortly.

@markerikson markerikson merged commit 6948056 into reduxjs:v7-hooks-alpha Apr 22, 2019
@MrWolfZ MrWolfZ deleted the use-selector-deps branch April 22, 2019 20:17
timdorr pushed a commit that referenced this pull request May 30, 2019
#1251)

* fix stale selector issue

* add `deps` to `useSelector` to allow controlling stability of selector
timdorr pushed a commit that referenced this pull request Jun 11, 2019
#1251)

* fix stale selector issue

* add `deps` to `useSelector` to allow controlling stability of selector
albertodev7 pushed a commit to albertodev7/react-redux that referenced this pull request Dec 8, 2022
reduxjs#1251)

* fix stale selector issue

* add `deps` to `useSelector` to allow controlling stability of selector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants