Skip to content

Memoize useDispatch? #1468

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

Closed
bdwain opened this issue Nov 21, 2019 · 9 comments · Fixed by #1598
Closed

Memoize useDispatch? #1468

bdwain opened this issue Nov 21, 2019 · 9 comments · Fixed by #1598

Comments

@bdwain
Copy link

bdwain commented Nov 21, 2019

The docs say

When passing a callback using dispatch to a child component, it is recommended to memoize it with useCallback, since otherwise child components may render unnecessarily due to the changed reference.

with the example code being

  const dispatch = useDispatch()
  const incrementCounter = useCallback(
    () => dispatch({ type: 'increment-counter' }),
    [dispatch]
  )

I think this could use some clarification. I am understanding it to not be specific to useDispatch, and to just be the classic react thing of don't pass anonymous functions to children since they are different every render.

If that is the case, it would probably be worth highlighting that it's not specific to useDispatch. If I am misunderstanding, then is there any reason that react-redux can't export some helper that calls useCallback for you? I imagine most people would not realize they need to do this.

@markerikson
Copy link
Contributor

We decided not to ship any kind of a useActions helper during the alpha period. And yes, it really is a plain React question.

@josepot
Copy link
Contributor

josepot commented Nov 21, 2019

FWIW I also find that recommendation and example pretty misleading.

Using useCallback and React.memo comes at a cost. Is it worth it paying that cost for avoiding the re-render of a simple button component? I honestly doubt it.

I'm pretty sure that the following code, actually performs better than the code of the example, and it's a lot simpler:

import React from 'react'
import { useDispatch } from 'react-redux'

export const CounterComponent = ({ value }) => {
  const dispatch = useDispatch()
  const incrementCounter = () => dispatch({ type: 'increment-counter' })

  return (
    <div>
      <span>{value}</span>
      <button onClick={incrementCounter}>Increment counter</button>
    </div>
  )
}

@markerikson
Copy link
Contributor

I agree that it's not necessary for a <button>, but the point was to illustrate the use of useCallback() with useDispatch(). If you feel that a different example would illustrate the point better, please file a PR to improve the docs.

@papb
Copy link
Contributor

papb commented Jun 29, 2020

The example with useCallback gives [dispatch] as a dependency array, so wouldn't the callback reference still change anyway?

Before:

  1. () => dispatch({ type: 'increment-counter' }) is a new function every time so child component re-renders every time

After:

  1. const dispatch = useDispatch() creates a new dispatch instance every time
  2. const incrementCounter = useCallback(() => dispatch({ type: 'increment-counter' }), [dispatch]) gets the same function reference as long as dispatch did not change (since it is given as a dependency array). But since dispatch is new every single time, this is pointless.

What am I missing? Thank you very much

@bdwain
Copy link
Author

bdwain commented Jun 29, 2020

Dispatch isn’t new every time. It’s the same (I think) as long as you have the same store.

@markerikson
Copy link
Contributor

const dispatch = useDispatch() creates a new dispatch instance every time

This statement is incorrect.

The implementation is just:

const useDispatch = () => {
  const store = useStore();
  return store.dispatch;
}

You typically only have one Redux store instance for the lifetime of the app, so dispatch will be the same function reference the entire time. (In fact, in earlier versions of React-Redux, passing a new store reference was forbidden. We do now support changing it at runtime, but realistically you probably won't do that.)

However, the ESLint rule doesn't know that - it just knows that dispatch isn't a built-in React hook return value, so it might change, and therefore it tells you it should be added to the dependencies array just in case it ever does change.

@papb
Copy link
Contributor

papb commented Jun 30, 2020

Thank you for this clarification! This is great. I think I will submit a PR soon to improve the docs on this matter.

However, before that, I wish to understand it a little better. You (@markerikson) said:

The implementation is just:

const useDispatch = () => {
  const store = useStore();
  return store.dispatch;
}

This alone does not imply that the function is always the same instance because it just shifts the problem into the stability of useStore. I tried navigating the code but it wasn't conclusive...

  • As you've shown useDispatch() is stable (i.e. returns the same instance) if useStore() is stable
  • Checking the code for useStore, I see that it is stable if useReduxContext() is stable
  • Checking the code for useReduxContext(), I see that it is stable if useContext() (from React) is stable

The React docs do not explicitly state this; I guess it is probably obvious for someone minimally acquainted with it, but I have never used it so I don't know.

That said, I will propose a PR.

@markerikson
Copy link
Contributor

markerikson commented Jun 30, 2020

I... just explained under exactly what circumstances the store instance might change :) You didn't need to do any further digging around.

And no, it's not a question of whether useContext is stable. The dispatch reference only changes if you pass a different store instance to <Provider store={store}>.

That's it.

And in 99.98% of Redux apps, that store instance will never change throughout the life of the app.

@papb
Copy link
Contributor

papb commented Jun 30, 2020

I... just explained under exactly what circumstances the store instance might change :) You didn't need to do any further digging around.

Yeah, I know, but I got curious 😬 Thank you.

And no, it's not a question of whether useContext is stable. The dispatch reference only changes if you pass a different store instance to .

Ah, ok! Makes sense! Thank you very much. Could you please review my PR?

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 a pull request may close this issue.

4 participants