Remove deps of useSelector #1272
Conversation
|
Deploy preview for react-redux-docs ready! Built with commit a8098b8 |
test/hooks/useSelector.spec.js
Outdated
| @@ -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', () => { | |||
There was a problem hiding this comment.
Actually, perhaps we should get rid of this test now...
There was a problem hiding this comment.
yeah, this test was specifically for deps, so I would remove it
docs/api/hooks.md
Outdated
| 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) |
There was a problem hiding this comment.
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
docs/api/hooks.md
Outdated
| (todos) => todos.filter(todo => todo.completed !== true) | ||
| ) | ||
|
|
||
| export const selectNPendingTodos = createSelector( |
There was a problem hiding this comment.
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
docs/api/hooks.md
Outdated
| (todos) => todos.filter(todo => todo.completed !== true) | ||
| ) | ||
|
|
||
| export const selectNPendingTodos = createSelector( |
There was a problem hiding this comment.
I am a big fan of proper naming, so I would make sure the examples do it too, i.e. call this selectNrOfPendingTodos
docs/api/hooks.md
Outdated
|
|
||
| return <div>{todo.text}</div> | ||
| } | ||
| ``` | ||
|
|
||
| With Reselect: |
There was a problem hiding this comment.
I would give much more context about this and the following pattern, i.e. in which situations to use it
josepot
force-pushed
the
feat/remove-useSelector-deps
branch
from
6304f01
to
a8098b8
Compare
|
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 :-) |
|
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! |
|
What's your view on this PR @markerikson @timdorr ? |
|
@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! |
|
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 :) |
|
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. |
|
I say we get this merged in now and rebase #1273 to get that just about the evaluations change. |
|
No big objections? Cool. In it goes... |
As @lukaszfiszer pointed out on #1252 it's better if we get rid of the dependencies of
useSelectorand we let the users to memoize the selectors themselves.