eslint-plugin-react-hooks: JSX support for react-hooks/exhaustive-deps (#18051)

[chrstnv]

Summary

#18051

As mentioned in the discussion, ESLint does not implement React-specific logic, and therefore JSX Identifiers will not be treated by ESLint as regular variables with references to them in scopes.

Because of this circumstance, when we collect hook callback dependencies, JSX variables don't end up in the list of dependencies and JSX-component variables declared in the hook dependencies array are considered exhaustive, but they are not.

So I implemented a prototype of a possible solution to this problem — handling JSX-expressions in the dependency gathering logic of the plugin.

In a nutshell, if JSX is found in the callback hook, then I traverse the JSX tree of elements and collect a list of React components. In the process, I filter out all elements that are not React components. Then I add the resulting array to the list of collected dependencies, which will be checked for exhaustiveness or lack of dependencies.

My prototype solution is imperfect at the moment, it doesn't find JSX-expressions in if/else constructions, it does not fully support JSXMemberExpressions, I have not yet written Typescript tests, etc. But I will do my best if I know that this is the right way.

Thanks!

Test Plan

✅ All test, test-prod, lint, flow checks are passed.
Added some new tests for positive and negative cases of JSX support.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit bedabf6:

Sandbox	Source
React	Configuration

[gaearon]

Thanks for the PR! My concern with this approach is that the traversal is manual and it’s both difficult to maintain and difficult to actually handle all the nodes correctly. Is there any built-in traversal helper that we can instead to find all JSX nodes?

[gaearon]

Maybe this can help?

https://github.com/eslint/eslint-scope

Note the estraverse helper in the README example.

[chrstnv]

Thanks for the great idea with estraverse! It seems like this helper solves all the problems I wrote above. I've refactored the logic and added a set of positive and negative tests for the main JSX use cases.
I would be grateful if you could take a look at the updated solution. Thanks!

[gaearon]

It doesn't seem right that we need a special case here. I think we want to rely on the same logic that we use for identifiers in general. If they're coming from a "pure scope" — e.g. from within the component function — then we want them to be deps. But if they're coming from outside — e.g. from the top-level import — then they shouldn't be dependencies. We already have this logic for regular identifiers, so we need to be able to reuse that somehow.

[chrstnv]

So, I have implemented a solution to this problem, and the common logic of the solution is as follows:

• for the reason that our dependencies must came from pure scopes, I went through the set of pure scopes and picked up all the variables that are declared in them. This list will include function arguments, variables declarations, functions declarations etc.
• I have added the isPureScopeDependency predicate, which checks if a JSX component is in the pure scope variable list. If this is true, then the component is included in the dependency list, if not, then it does not.
• I've removed the special conditions that checked if the JSXIdentifier is equal to React, Fragment, etc. Now these JSXIdentifiers will not be included in the list of dependencies automatically, as well as all other JSXIdentifiers that are not among the pure scopes variable list.

I couldn't directly reuse the logic with regular identifiers, because it is based on references and their resolved variables. JSXIdentifiers have no variable references in its interface and I didn't find a way to add it to them.
Therefore, I tried to implement conceptually similar logic, but based on variables from pure scopes.

[stale]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.