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

Add option to ignore stable hooks from eslint-exhaustive-deps rule #20513

Closed

Conversation

masad-frost
Copy link

@masad-frost masad-frost commented Dec 24, 2020

Summary

I find myself using a bunch of custom ref-esque dependencies inside of useEffect. I'd like to ignore them from exhaustive deps errors just like the rule currently ignores useRef. My useEffect deps are usually decorated with eslint-ignores due to this, and non of my deps are linted anymore, including ones that are not coming from stable hooks.

It would be nice to be able to ignore some of these globally, per file, or on a line basis. One way to go about it is to either add a pattern for stable hooks.

Example:

/*eslint react-hooks/exhaustive-deps: ["error", { stableHooksPattern: "use(.*)Ref" }]*/

function MyComponent(props) {
  const myCustomRef = useMyCustomRef();
  const isMounted  = useIsMounted();

  useEffect(() => {
    console.log(myCustomRef.current);
    console.log(isMounted.current);
  }, []);
}

resolves #16873

Test Plan

I wrote a couple of tests for valid and invalid cases.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 24, 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 178bdeb:

Sandbox Source
React Configuration

@masad-frost
Copy link
Author

Another idea is to have a pattern for dependencies instead of their hook (i.e. stableDependenciesPattern). This allows us to also ignore things that don't come from hooks/function calls, such as props.

@sizebot
Copy link

sizebot commented Dec 24, 2020

Details of bundled changes.

Comparing: 50393dc...178bdeb

eslint-plugin-react-hooks

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
eslint-plugin-react-hooks.development.js +0.5% +0.2% 84.66 KB 85.05 KB 20.17 KB 20.22 KB NODE_DEV
eslint-plugin-react-hooks.production.min.js 🔺+0.9% 🔺+0.4% 24.75 KB 24.96 KB 8.51 KB 8.54 KB NODE_PROD

Size changes (experimental)

Generated by 🚫 dangerJS against 178bdeb

@sizebot
Copy link

sizebot commented Dec 24, 2020

Details of bundled changes.

Comparing: 50393dc...178bdeb

eslint-plugin-react-hooks

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
eslint-plugin-react-hooks.development.js +0.5% +0.2% 84.65 KB 85.04 KB 20.16 KB 20.21 KB NODE_DEV
eslint-plugin-react-hooks.production.min.js 🔺+0.9% 🔺+0.4% 24.74 KB 24.95 KB 8.5 KB 8.53 KB NODE_PROD

Size changes (stable)

Generated by 🚫 dangerJS against 178bdeb

@steveoh
Copy link

steveoh commented Jul 19, 2021

Is this going to be the path forward or what other options are there besides ignoring eslint exhaustive hooks?

@masad-frost
Copy link
Author

I'm happy to resolve conflicts and pick it back up if maintainers want to. I think there might be forks that solve this problem.

@stale
Copy link

stale bot commented Jan 8, 2022

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.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 8, 2022
@hyphenized
Copy link

Any updates on this? I've been using a prefix since it's hard to distinguish between stable and non stable deps, but it has kind of made the code unnecessarily verbose 😕

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jan 25, 2022
@masad-frost
Copy link
Author

No interest from maintainers. You can get the diff from this and create your own eslint rule. Honestly I've just been passing stable refs to my dep list 🤷

@masad-frost
Copy link
Author

Closing, I'll use a fork. Thanks @facebook

@masad-frost masad-frost deleted the fm-stableHooksPattern branch March 22, 2022 20:59
@masad-frost masad-frost restored the fm-stableHooksPattern branch March 22, 2022 20:59
@masad-frost masad-frost deleted the fm-stableHooksPattern branch March 22, 2022 21:00
@masad-frost masad-frost restored the fm-stableHooksPattern branch March 22, 2022 21:00
@gdbd
Copy link

gdbd commented Mar 25, 2022

bump

@gburtini
Copy link

bump x2

@gdbd
Copy link

gdbd commented May 2, 2023

bump x3

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.

[eslint-plugin-react-hooks] allow configuring custom hooks as "static"
7 participants