5 ms·

> Adding non-primitive props you get passed into your component to internal dependency arrays is rarely right, because this component has no control over the re

> Adding non-primitive props you get passed into your component to internal dependency arrays is rarely right, because this component has no control over the referential stability of those props. I think this is wrong? If you memoize a callback with useCallback and that callback uses something from props without putting it in the dependency array, and then the props change & the callback runs, the callback will use the original/stale value from the props and that's almost certainly a bug. They might be trying to say you just shouldn't use useCallback in that situation, but at best it's very confusingly written there because it sure sounds like it's saying using useCallback but omitting a dependency is acceptable. IMHO useCallback is still a good idea in those situations presuming you care about the potential needless re-renders (which for a lot of smaller apps probably aren't really a perf issue, so maybe you don't). If component A renders component B and does not memoize its own callback that it passes to B as a prop, that is A's problem, not B's. Memoization is easy to get wrong by forgetting to memoize one spot which cascades downwards like the screenshots example, but that doesn't mean memoization is never helpful or components shouldn't do their best to optimize for performance.

p1necone 1y ago
In my experience you're correct yeah - 95% of the time if you use it it should be included in the dependency array, and any edge cases where that's not true you should endeavour to understand and document. There's a 'react-hooks/exhaustive-deps' eslint rule for exactly this purpose.
Izkata 1y ago
> > Adding non-primitive props you get passed into your component to internal dependency arrays is rarely right, because this component has no control over the referential stability of those props. > I think this is wrong? If you memoize a callback with useCallback and that callback uses something from props without putting it in the dependency array, and then the props change & the callback runs, the callback will use the original/stale value from the props and that's almost certainly a bug. The problem is up a level in the tree, not in the component being defined. See the following code block, where they show how the OhNo component is used - the prop is always different.
- skydhash 1y ago
  The parent is the source of truth for the children nodes. Which means don't send anything down that you don't intend to be stable. As the children will (and should) treat it as gospel. That also means that the root component is god for the whole view. Most source of bugs happens when people ignore this simple fact. Input start from the root and get transformed along the way. Each node can bring things from external, but it should be treated careful as the children down won't care. It's all props to them. Which also means don't create new objects out of the blue to pass down. When you need to do so, memoize. As the only reason you need to do so is because you want: - to join two or more props or local state variables - or to transform the value of a prop or a local state variable (in an immutable fashion, as it should be). - or both.
  - bastawhiz 1y ago
    You're making the same argument as the author. As they said, the only two solutions are: 1. Enforce that you're always memoizing everywhere always 2. Memoize every new function and object (and array) and pray everyone else does too #1 is pretty miserable, #2 is almost always guaranteed to fail. The mismatch is that you need stable props from the bottom up, and you enforce stable props from the top down. Any oversight in between (even indirectly, like in a hook or external module) creates a bug that appears silently and regresses performance.
    - skydhash 1y ago
      The only solution is to have everyone have discipline and respect the convention of React. > you need stable props from the bottom up, and you enforce stable props from the top down. You don't need stable props from the bottom up. Component will always react to props change and will returns a new Tree. The function for that are going to be run multiple times and you're not supposed to control when it does. So the answer is to treat everything that comes from the parent as the gospel truth. And any local transformations should be done outside of function calls. #1 is not needed. Memoization concerns is always local to the component, as long as you ensure that the props you're sending down is referentially stable, you're golden. #2 is not needed. Again your only concern is the current component. Whatever everyone does elsewhere is their bad practices. Maybe they will cast your carefully constructed type into any or try/catch your thougthful designed errors into oblivion along the way too. The only thing you can do is correct them in PR reviews or fix the code when you happen upon it.
tkdodo 1y ago
author here , sorry for the confusion. I did not mean to imply that leaving out the prop from useCallback is better - you _always_ have to adhere to the exhaustive-deps lint rule to avoid stale closures (I have a separate blog post on this topic). What I meant is that the API design of B, where the hook only works as intended if the consumer A memoizes the input props, is not ideal because A has no way of knowing that they have to memoize it unless they look at the implementation or it’s clearly documented (which it rarely is). It’s not A’s problem because B could’ve used the latest ref pattern (or, in the future, useEffectEvent) to work without passing that burden onto its consumers.

p1necone 1y ago

In my experience you're correct yeah - 95% of the time if you use it it should be included in the dependency array, and any edge cases where that's not true you should endeavour to understand and document. There's a 'react-hooks/exhaustive-deps' eslint rule for exactly this purpose.

Izkata 1y ago

> > Adding non-primitive props you get passed into your component to internal dependency arrays is rarely right, because this component has no control over the referential stability of those props. > I think this is wrong? If you memoize a callback with useCallback and that callback uses something from props without putting it in the dependency array, and then the props change & the callback runs, the callback will use the original/stale value from the props and that's almost certainly a bug. The problem is up a level in the tree, not in the component being defined. See the following code block, where they show how the OhNo component is used - the prop is always different.

skydhash 1y ago
The parent is the source of truth for the children nodes. Which means don't send anything down that you don't intend to be stable. As the children will (and should) treat it as gospel. That also means that the root component is god for the whole view. Most source of bugs happens when people ignore this simple fact. Input start from the root and get transformed along the way. Each node can bring things from external, but it should be treated careful as the children down won't care. It's all props to them. Which also means don't create new objects out of the blue to pass down. When you need to do so, memoize. As the only reason you need to do so is because you want: - to join two or more props or local state variables - or to transform the value of a prop or a local state variable (in an immutable fashion, as it should be). - or both.
- bastawhiz 1y ago
  You're making the same argument as the author. As they said, the only two solutions are: 1. Enforce that you're always memoizing everywhere always 2. Memoize every new function and object (and array) and pray everyone else does too #1 is pretty miserable, #2 is almost always guaranteed to fail. The mismatch is that you need stable props from the bottom up, and you enforce stable props from the top down. Any oversight in between (even indirectly, like in a hook or external module) creates a bug that appears silently and regresses performance.
  - skydhash 1y ago
    The only solution is to have everyone have discipline and respect the convention of React. > you need stable props from the bottom up, and you enforce stable props from the top down. You don't need stable props from the bottom up. Component will always react to props change and will returns a new Tree. The function for that are going to be run multiple times and you're not supposed to control when it does. So the answer is to treat everything that comes from the parent as the gospel truth. And any local transformations should be done outside of function calls. #1 is not needed. Memoization concerns is always local to the component, as long as you ensure that the props you're sending down is referentially stable, you're golden. #2 is not needed. Again your only concern is the current component. Whatever everyone does elsewhere is their bad practices. Maybe they will cast your carefully constructed type into any or try/catch your thougthful designed errors into oblivion along the way too. The only thing you can do is correct them in PR reviews or fix the code when you happen upon it.

tkdodo 1y ago

author here , sorry for the confusion. I did not mean to imply that leaving out the prop from useCallback is better - you _always_ have to adhere to the exhaustive-deps lint rule to avoid stale closures (I have a separate blog post on this topic). What I meant is that the API design of B, where the hook only works as intended if the consumer A memoizes the input props, is not ideal because A has no way of knowing that they have to memoize it unless they look at the implementation or it’s clearly documented (which it rarely is). It’s not A’s problem because B could’ve used the latest ref pattern (or, in the future, useEffectEvent) to work without passing that burden onto its consumers.