Implement a rule set for react-hooks

[iduuck]

I just saw, that we are in the use of the react-hooks plugin for eslint and not specifying any rules for it. Let's change this.

cc: @visualcookie @amr3ssam @chmtt @LoRaetz

This issue is for discussion for now, until we are on the same page, we may need someone to implement this feature.

[iduuck]

To shoot the first shot, I would love to go with this kind of ruleset (exact same as react is writing about in the documentations):

• "react-hooks/rules-of-hooks" to be of type "error"
• "react-hooks/exhaustive-deps" to be of type "warn"

Why "warn" only, for exhaustive-deps?

Given & Wanted:

function FooBar({ foo }) {
  useEffect(() => {
    if (foo) {
      console.log('bar')
    }
  }, [])

  return null
}

ESLint --fix Output:

function FooBar({ foo }) {
  useEffect(() => {
    if (foo) {
      console.log('bar')
    }
  }, [foo]) // This one here.

  return null
}

---

I would rather prefer the version one ("Given & Wanted") instead of the fixed option. What about you?

[amr3ssam]

I think i will agree with you completely in the first rule.
for the second one, i think we will need a little bit more testing
what is the effect of the fix
is it okay or will lead to a different result than the one you need

[iduuck]

@amr3ssam As far as I know, the effect ist the same. Because when the prop foo is changing, we are also seeing a re-render of the component. It could be error prone for some use cases, but every documentation is stating that we should use the [] dependency for component mounting and unmounting.

The only use case I can think of, where it could be wrong, if there is some case of, when foo is asynchronously fetched, and first null or something, and then holding a value. Then the useEffect will only be called with the null value, instead of the right value. However, we are normally conditionally rendering everything when there is a loading stage happening. So let‘s discuss this. I mean at the end of the day, we still have „warning“ telling the specific developer to have an eye on this line, that it could be the error raising thing.

[amr3ssam]

what you are saying make sense to be honest because the developer still have as you said the "waring" flag
so yea i will agree with you

[visualcookie]

I'm totally fine with the second rule just to be used as a warning to tell the developer "hej, there could be an issue".

[iduuck]

Okay, nice. We have a solution.

Will create a pull request today or tomorrow, and then let you guys confirm on your projects that everything is fine with it.

Thanks 🚀