Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

chrstnv
Copy link

@chrstnv chrstnv commented Sep 20, 2020

Summary

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
Copy link

codesandbox-ci bot commented Sep 20, 2020

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 3ae490e:

Sandbox Source
React Configuration

@chrstnv chrstnv closed this Sep 20, 2020
@chrstnv chrstnv deleted the issue-18051-jsx-support branch September 20, 2020 09:52
@chrstnv chrstnv restored the issue-18051-jsx-support branch September 20, 2020 09:53
@chrstnv chrstnv deleted the issue-18051-jsx-support branch September 20, 2020 09:53
@chrstnv chrstnv restored the issue-18051-jsx-support branch September 20, 2020 09:54
@chrstnv chrstnv reopened this Sep 20, 2020
@chrstnv chrstnv closed this Sep 20, 2020
@chrstnv chrstnv deleted the issue-18051-jsx-support branch September 20, 2020 09:54
@chrstnv chrstnv restored the issue-18051-jsx-support branch September 20, 2020 09:55
@sizebot
Copy link

sizebot commented Sep 20, 2020

Warnings
⚠️ Could not find build artifacts for base commit: 6291255

Size changes (stable)

Generated by 🚫 dangerJS against 38a8d3c

@chrstnv chrstnv reopened this Sep 20, 2020
@sizebot
Copy link

sizebot commented Sep 20, 2020

Warnings
⚠️ Could not find build artifacts for base commit: 6291255

Size changes (experimental)

Generated by 🚫 dangerJS against 38a8d3c

@chrstnv chrstnv force-pushed the issue-18051-jsx-support branch 2 times, most recently from 7a458e2 to 0deec67 Compare September 20, 2020 10:28
@chrstnv chrstnv closed this Sep 20, 2020
@chrstnv chrstnv deleted the issue-18051-jsx-support branch September 20, 2020 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants