React: [react-hooks/exhaustive-deps] should have special case for props callbacks

[benny-medflyt]

react-hooks/exhaustive-deps attempts to be 100% accurate, and flags the following:

const c = useCallback((value: number) => {
    props.onChange(value);
}, [props.onChange]);         // <-- ERROR: Should be [props]

It helpfully recommends that we should do this:

const {
    onChange
} = props;

const c = useCallback((value: number) => {
    onChange(value);
}, [onChange]);

Unfortunately, I can't bring myself to actually write my code this way. It adds pointless boilerplate code, makes my IDE "find usages" feature on my props send me through another level of indirection i have to follow, and i have this ugly object destructure inside every react component I write only for the callback props that is plain ugly noise.

I've been using this technique lately:

useCallback((value: number) => {
    props.onChange.call(null, value);
}, [props.onChange]);        // <-- now the linter is happy!

Which is a bit better, but still ugly, and sadly the autocomplete experience for the call arguments is much worse.

I understand that if I were to write something like car.drive() then the linter would be correct that I should depend on car since car is being passed as this to drive, and car might change.

But "props" specifically is different. No matter how hard I try, I cannot think of a legitimate case where someone would set a prop callback such that it is expecting the actual props as the this argument. It makes no sense, and all of the official react documentation explain how you should use "bind" or whatever so that you have a locked-down this (if this is even needed at all). And the react team apparently agrees with me here, because this very lint rule suggests to destructure the prop, which would anyway break any such code that expects to have props passed back as this!

Recommendation: the lint rule should treat any function call of the form props.xxx(...) such that the expected dependency is props.xxx (instead of props). Just to be clear: this should only be done specifically for props, any other "method" style function call on an object other than props should continue to be linted in the current way.

Thank you

[gaearon]

Unfortunately, I can't bring myself to actually write my code this way. It adds pointless boilerplate code, makes my IDE "find usages" feature on my props send me through another level of indirection i have to follow, and i have this ugly object destructure inside every react component I write only for the callback props that is plain ugly noise.

The recommended pattern is just this:

function Button({ color, children, onChange }) {
  // ...
}

I.e. destructure them at declaration.

You can think of props as named arguments.

[benny-medflyt]

Thanks for the reply, but that pattern doesn't work well with TypeScript. The only sane way in TypeScript is:

```typescript
function Button(props: ButtonProps) {
// ...
}

[benny-medflyt]

... I mean, you could do

interface ButtonProps {
  color: string;
  children: JSX.Element;
  onChange: () => void;
}

function Button({ color, children, onChange }: ButtonProps) {
  // ...
}

But with it ends up being way too much repetition for my tastes

[gaearon]

It’s a bit repetitive but that’s the convention.

I hear what you’re saying about passing this being unintentional. But it’s meant to guard against a worst possible misunderstanding. I could imagine someone writing this.color instead of this.props.color in a class component method and not realising it. And then if it works accidentally due to this being props, but breaks later due to stale closure, this makes the bug so much more confusing.

[benny-medflyt]

It’s a bit repetitive but that’s the convention.

My understanding is that the React philosophy is not to be over-"opinionated".

This destructured props convention makes complete sense to me when using pure JavaScript: the syntax is concise and most importantly acts as documentation for which props the component accepts (otherwise the consumer has to comb through the internal source of the component to find props usages...).

But when using TypeScript this makes less sense. The types already document the list of props, and the destructing just adds noise, especially when we have lots of props. Also, some codebases have a lint rule that bans shorthand object destructuring(for various reasons, one being that it breaks IDE "rename symbol" functionality).

My goal isn't to convince anyone that destructuring props is a bad idea. Just that we should recognize that different people and projects can have different (reasonable) opinions and approaches, and we shouldn't dictate that all projects must follow a single strict convention.

My original suggestion won't stop anyone from using their own preferred convention, it simply enables straight-forward usage of a legitimate style of writing components.

And btw, this style of declaring an explicit props parameter is the convention that is used in all of the official react docs (including the section about hooks):

• https://reactjs.org/docs/components-and-props.html#function-and-class-components
• https://reactjs.org/docs/hooks-overview.html

[benny-medflyt]

[...] I could imagine someone writing this.color instead of this.props.color in a class component method and not realising it. And then if it works accidentally due to this being props, but breaks later due to stale closure, this makes the bug so much more confusing.

If the goal is to make it easier for people to find the bugs in their own callback code, then there are better approaches that tackle the root of the problem. One idea is that React could (in development mode) automatically iterate through all of the "prop" fields that are functions and scrub the this out of them. This way any improper usage of this in callback code would be caught immediately.

[gaearon]

But when using TypeScript this makes less sense. The types already document the list of props, and the destructing just adds noise, especially when we have lots of props.

You’re kind of buying into some verbosity by using TS in the first place. Declaring props twice is mild compared to other common things types force you to do.

Also, some codebases have a lint rule that bans shorthand object destructuring(for various reasons, one being that it breaks IDE "rename symbol" functionality).

You know what I would say... IDEs should fix this 🙂

Just that we should recognize that different people and projects can have different (reasonable) opinions and approaches, and we shouldn't dictate that all projects must follow a single strict convention.

You can always turn off the lint rule. Or fork it. But it exists to enforce a convention. So it makes sense to me that it relies on some other conventions that are overwhelmingly common.

Destructuring is also going to be the only way to specify default prop values in the future. We’re planning to drop defaultProps for function components. So this argument will come back again when someone is unhappy about it. We might as well start nudging people to adopt this pattern now. A lint rule is a good way to do it.

And btw, this style of declaring an explicit props parameter is the convention that is used in all of the official react docs (including the section about hooks):

That’s a good point. We should address this in a Hooks docs rewrite. It is written that way because when the main docs were written, ES6 was new and people weren’t familiar with its features. And with Hooks we just wanted to be consistent within the docs. I think we can change this now.

One idea is that React could (in development mode) automatically iterate through all of the "prop" fields that are functions and scrub the this out of them.

I’m struggling to imagine how this would work. In JS this depends on the callsite. We wouldn’t be able to do it without binding all functions, which would change all function identities and lead to other problems.

[FiberesimaJoseph]

react-hooks/exhaustive-deps
const c = useCallback((value: number) => {
props.onChange(value);
}, [props.onChange]); // <-- ERROR: Should be [props]

[rachelnabors]

We recommend this convention. Closing for the time being!