📎 Break useExhaustiveDependencies into two rules

[crutchcorn]

Description

I was migrating to the newest version of Biome and noticed that it seems to group in two seemingly unrelated rules into one:

• × This hook do not specify all of its dependencies.
• × This hook specifies more dependencies than necessary.

Inside of useExhaustiveDependencies

However, there's a problem with doing so this way.

While does not specify all of its deps is a rule that's ported over from React's common ESLint rules, specifies more deps than necessary isn't a rule, as it doesn't break the React Rules of Hooks. Moreover, specifying additional deps is how you're able to get some functionality to work as-expected at times.

Because of this, we should:

• specifies more deps than necessary should be broken into a new rule
• This new rule should be marked as optional, not required

[ematipico]

How would you call the new rule?

[crutchcorn]

What about useOnlyRequiredDependencies?

[arendjr]

noExcessiveDependencies? :)

[arendjr]

If we implement the suggestion from #2509 , do you think we can close this issue? If I understand correctly, the main reason for splitting is to be able to disable one type of check, while leaving others enabled. But if we can make exceptions per dependency, I think that use case is also covered.

[arendjr]

Closing in favor of #2509.

[flatplate]

I think this suggestion still makes sense even with the special ignore.

A way to disable "more dependencies than necessary" rule at least for useEffect and useLayoutEffect should be provided imo since that is what useEffect is used for.

I am migrating a large codebase right now and having to ignore thousands lines should be unnecessary.

[arendjr]

It’s a fair point, although if it is indeed specific to these hooks (and not others), maybe a more specific configuration option might make sense instead of splitting 🤔

[flatplate]

I agree that an option for this rule, or maybe in the hook options, would be enough.

[emigdio821]

I agree with @flatplate .
I recently migrated a project to Biome. Currently I have a simple React custom hook and this error is showing up. The only way I found to fix it is to use the // biome-ignore comment.

[wlechowicz]

I'd like to highlight that the messaging for the rule doesn't help either. If a useEffect has a dependency on a component prop (an "extra" dependency, that's not directly referenced in the effect code), then the message from Biome incorrectly states that:

× This hook specifies more dependencies than necessary: elementId
i Outer scope values aren't valid dependencies because mutating them doesn't re-render the component.

which is a misleading, false statement - a component prop is not an "outer scope value" and it sure as hell does "re-render the component". It's not until I started tinkering with the code that I understood that Biome is just unhappy that there is a dependency that's not referenced in the code, but the code is perfectly valid as per official React guidelines. While the "This hook specifies more dependencies than necessary" part makes sense (and could be configurable separately, per this discussion), the second piece of info looks like Biome failed to recognized that the referenced dependency is a reactive value.

[simon-paris]

I'd like to implement this (as an option).

And I'd also like to make an option to re-enable this behavior: #608

But how should the config object look? Something like this?

{
  hooks: [...]
  reportMissingDependenciesArray: false,
  reportUnnecessaryDependencies: true,
}

But I noticed that the existing config schema is designed to be shared between multiple rules (but it's not actually shared anymore) and adding new keys would change that design.

[arendjr]

Thanks! I’ll assign the issue to you then, @simon-paris.

I don’t see an issue with the options as you proposed them. I would suggest it’s better to implement them in separate PRs.