-
Notifications
You must be signed in to change notification settings - Fork 49.6k
[eslint-plugin-react-hooks][RulesOfHooks] handle React.useEffect in addition to useEffect #34076
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][RulesOfHooks] handle React.useEffect in addition to useEffect #34076
Conversation
return false; | ||
} | ||
|
||
function getNodeWithoutReactNamespace( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied over from ExhaustiveDeps.ts
return node; | ||
} | ||
|
||
function isUseEffectIdentifier(node: Node): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could have kept the code inlined, but it makes it a bit more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing @jackpope's feedback. This looks great, thanks for taking this on.
Comparing: c260b38...5526704 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
…ddition to useEffect (#34076) ## Summary This is a fix for #34074 ## How did you test this change? I added tests in the eslint package, and ran `yarn jest`. After adding the new tests, I have this: On main | On this branch -|- <img width="356" height="88" alt="image" src="https://github.com/user-attachments/assets/4ae099a1-0156-4032-b2ca-635ebadcaa3f" /> | <img width="435" height="120" alt="image" src="https://github.com/user-attachments/assets/b06c04b8-6cec-43de-befa-a8b4dd20500e" /> ## Changes - Add tests to check that we are checking both `CallExpression` (`useEffect(`), and `MemberExpression` (`React.useEffect(`). To do that, I copied the `getNodeWithoutReactNamespace(` fn from `ExhaustiveDeps.ts` to `RulesOfHooks.ts` DiffTrain build for [87a45ae](87a45ae)
…ddition to useEffect (#34076) ## Summary This is a fix for #34074 ## How did you test this change? I added tests in the eslint package, and ran `yarn jest`. After adding the new tests, I have this: On main | On this branch -|- <img width="356" height="88" alt="image" src="https://github.com/user-attachments/assets/4ae099a1-0156-4032-b2ca-635ebadcaa3f" /> | <img width="435" height="120" alt="image" src="https://github.com/user-attachments/assets/b06c04b8-6cec-43de-befa-a8b4dd20500e" /> ## Changes - Add tests to check that we are checking both `CallExpression` (`useEffect(`), and `MemberExpression` (`React.useEffect(`). To do that, I copied the `getNodeWithoutReactNamespace(` fn from `ExhaustiveDeps.ts` to `RulesOfHooks.ts` DiffTrain build for [87a45ae](87a45ae)
Summary
This is a fix for #34074
How did you test this change?
I added tests in the eslint package, and ran
yarn jest
. After adding the new tests, I have this:Changes
link:
to theeslint-plugin-react-hooks
pointing tobabel-plugin-react-compiler
otherwise it wouldn't run locallyCallExpression
(useEffect(
), andMemberExpression
(React.useEffect(
). To do that, I copied thegetNodeWithoutReactNamespace(
fn fromExhaustiveDeps.ts
toRulesOfHooks.ts