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

Feature: Add support to exhaustive-deps rule for any hook ending with Effect #18580

Merged
merged 3 commits into from
May 1, 2020

Conversation

airjp73
Copy link
Contributor

@airjp73 airjp73 commented Apr 11, 2020

Summary

Following up on @gaearon 's comment here. My original motivation was to document the additionalHooks option in the exhaustive-deps lint rule. He suggested an alternate solution by linting any hook ending with Effect. This PR adds support for that.

Test Plan

I find that eslint rule tests are pretty thorough so all my testing is just through those test cases. I copied all the tests I could find for the additionalHooks option and reworked them to use a useXEffect naming convention instead of the additionalHooks option. I also tweaked some of the additionalHooks tests that were testing for useCustomEffect and changed it to useCustomHook because useCustomEffect would get caught anyway after these changes.

@airjp73 airjp73 changed the title Feature/exhaustive deps effect Feature: Add support to exhaustive-deps rule for any hook ending with Effect Apr 11, 2020
@codesandbox-ci
Copy link

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 47994e7:

Sandbox Source
billowing-bash-qn7n4 Configuration

@sizebot
Copy link

sizebot commented Apr 11, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 47994e7

@sizebot
Copy link

sizebot commented Apr 11, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 47994e7

@karlvr
Copy link

karlvr commented Apr 15, 2020

Oh my code is so much nicer. Thank you thank you.

@gaearon gaearon merged commit 5ac9ca7 into facebook:master May 1, 2020
@gaearon
Copy link
Collaborator

gaearon commented May 1, 2020

Thanks.

@jquense
Copy link
Contributor

jquense commented May 21, 2020

This heuristic seems very prone to false positives, we have a bunch of custom "Effect" hooks that don't take dep arguments and now require pragma's to avoid warnings about potential infinite updates

@Glinkis
Copy link

Glinkis commented May 25, 2020

I am using hooks.macro, which adds a few hook utilities that doesn't take a dependency array. This now incorrectly shows up as a warning.

@gaearon
Copy link
Collaborator

gaearon commented May 26, 2020

One thing we could do is to only run checks if you have a function followed by an array, in addition to ending with Effect. But I'm leaning towards reverting this altogether. There's indeed been too many false positives being reported.

@gaearon
Copy link
Collaborator

gaearon commented May 26, 2020

I'm going to have to revert this because the heuristic has too many reasonable false positives.
Thanks for trying!
#19004

gaearon added a commit that referenced this pull request May 26, 2020
@gaearon
Copy link
Collaborator

gaearon commented May 26, 2020

This is removed in eslint-plugin-react-hooks@4.0.3. Thanks all for the input.

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.

7 participants