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

Remove deps of useSelector #1272

Merged

Conversation

josepot
Copy link
Contributor

@josepot josepot commented May 2, 2019

As @lukaszfiszer pointed out on #1252 it's better if we get rid of the dependencies of useSelector and we let the users to memoize the selectors themselves.

@netlify
Copy link

netlify bot commented May 2, 2019

Deploy preview for react-redux-docs ready!

Built with commit a8098b8

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

@@ -155,15 +155,16 @@ describe('React', () => {
expect(renderedItems.length).toBe(1)
})

it('re-uses the selector if deps do not change', () => {
it('re-uses the selector if it does not change', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, perhaps we should get rid of this test now...

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, this test was specifically for deps, so I would remove it

Copy link
Contributor

@MrWolfZ MrWolfZ left a comment

Choose a reason for hiding this comment

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

I would recommend splitting the docs changes and the code changes into two separate PRs, since they are quite different (and also the docs should target master while the code targets the alpha branch)

const todo = useSelector(state => state.todos[props.id], [props.id])
export const TodoListItem = ({id}) => (
const selector = useCallback(state => state.todos[id], [id])
const todo = useSelector(selector)
Copy link
Contributor

Choose a reason for hiding this comment

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

for this example it makes no sense to useCallback and it makes the example more complicated than it needs to be. Just do useSelector(state => state.todos[id]) and leave the whole memoization stuff for other examples

(todos) => todos.filter(todo => todo.completed !== true)
)

export const selectNPendingTodos = createSelector(
Copy link
Contributor

@MrWolfZ MrWolfZ May 2, 2019

Choose a reason for hiding this comment

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

these docs are not about showing how to compose reselect selectors. just use the simplest possible example, i.e. get the length directly in the first selector

(todos) => todos.filter(todo => todo.completed !== true)
)

export const selectNPendingTodos = createSelector(
Copy link
Contributor

@MrWolfZ MrWolfZ May 2, 2019

Choose a reason for hiding this comment

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

I am a big fan of proper naming, so I would make sure the examples do it too, i.e. call this selectNrOfPendingTodos


return <div>{todo.text}</div>
}
```

With Reselect:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would give much more context about this and the following pattern, i.e. in which situations to use it

@josepot josepot force-pushed the feat/remove-useSelector-deps branch from 6304f01 to a8098b8 Compare May 2, 2019 16:15
@lukaszfiszer
Copy link

This is indeed my proposed solution, but I still would like to hear what other maintainers think about it. Let's not rush too much with this change :-)

@MrWolfZ
Copy link
Contributor

MrWolfZ commented May 2, 2019

Regarding the docs changes, since I have another PR open with docs changes, I would like to add those docs changes there, if that's okay with you.

@josepot
Copy link
Contributor Author

josepot commented May 2, 2019

Regarding the docs changes, since I have another PR open with docs changes, I would like to add those docs changes there, if that's okay with you.

Totally! I went to open that PR and then I noticed that there was another PR for docs. That's why I didn't open it. Although, I will probably add some comments into that PR because I don't fully agree with some of the comments that you made to my docs proposal. No worries, nothing important. Thanks @MrWolfZ!

@lukaszfiszer
Copy link

What's your view on this PR @markerikson @timdorr ?

@timdorr
Copy link
Member

timdorr commented May 6, 2019

@lukaszfiszer I'm watching the discussion in #1273, because that encompasses this change plus some additional optimizations with caveats.

@josepot
Copy link
Contributor Author

josepot commented May 6, 2019

@lukaszfiszer I'm watching the discussion in #1273, because that encompasses this change plus some additional optimizations with caveats.

@timdorr ups! That is the opposite of what I intended, sorry! I initially thought that this PR was going to get merged quickly, but I wasn't sure about the other one, that's why the other one also has this commit. Sorry!

If you want I can close #1273 and re-open it once this PR gets merged or declined... (Or just leave it closed if you think that's better). Please, let me know if closing it would make things easier, and if that is the case, whether you would like for it to remain closed. Thanks!

@timdorr
Copy link
Member

timdorr commented May 6, 2019

I am a few days behind on the main feedback issue, so I need to catch up there too. I'll be able to look at this after I've read that novels worth of posts :)

@gaearon
Copy link
Contributor

gaearon commented May 7, 2019

FWIW our conclusion internally has been that custom Hooks should avoid their own dependency argument, and people should just memoize if it's necessary. You have to learn that pattern anyway.

@timdorr
Copy link
Member

timdorr commented May 8, 2019

I say we get this merged in now and rebase #1273 to get that just about the evaluations change.

@timdorr
Copy link
Member

timdorr commented May 13, 2019

No big objections? Cool. In it goes...

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.

5 participants