[ESLint] Suggest to destructure props when they are only used as members #14993
Conversation
Details of bundled changes.Comparing: 59ef284...9e795c8 eslint-plugin-react-hooks
Generated by 🚫 dangerJS |
|
I don't think it's worth warning if it's called something other than |
| @@ -2224,7 +2224,189 @@ const tests = { | |||
| } | |||
| `, | |||
| errors: [ | |||
| // TODO: make this message clearer since it's not obvious why. | |||
| "React Hook useEffect has a missing dependency: 'props'. " + | |||
There was a problem hiding this comment.
This error message kinda surprises me when taking into account our recent discussion ( #14876 ) around props referential equality etc.
If props have "unstable" reference then IMHO you shouldnt ever suggest using it as hook's dependency 🤔 The latter advice sounds way more appropriate (destructuring)
There was a problem hiding this comment.
props is OK as an escape hatch when you just want the rule to shut up. It's still stable if you only set state, for example. Better than nothing.
There was a problem hiding this comment.
Shouldnt the advice get reordered then? First propose destructuring and only latter propose using props directly.
You are calling it an escape hatch to shut up the rule - so it sounds to me that this is actually not something which you'd like to recommend.
There was a problem hiding this comment.
You're welcome to send a PR. :-) There are more worrying things I'm currently focused on than this particular message.
There was a problem hiding this comment.
Sure, might prepare one - was just validating my thinking so far :)
This addresses some confusion we've seen around
props.foo()asking forpropsto be included. It's also best practice anyway. But only show the warning ifpropsisn't used as an object. (And only if it's actually missing — so that it doesn't complain for valid code.)