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

Document additionalHooks option #16912

Closed
wants to merge 1 commit into from
Closed

Conversation

airjp73
Copy link
Contributor

@airjp73 airjp73 commented Sep 26, 2019

Hi!

I noticed that there doesn't seem to be any documentation about this option for exhaustive-deps. Was this a deliberate decision or an oversight? Obviously, feel free to decline this if you don't want to do this or this is the wrong place.

I recently used this for a custom hook at work and it's pretty useful but I'm a little uneasy about using undocumented features.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@sizebot
Copy link

sizebot commented Sep 26, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS against acaa56b

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@vkrol
Copy link
Contributor

vkrol commented Oct 2, 2019

Related: reactjs/react.dev#2350

@wcandillon
Copy link

wcandillon commented Dec 19, 2019

@airjp73 Thks for contributing this change. I hope this gets merged. This PR can be marked a fixing reactjs/react.dev#2350

@stale
Copy link

stale bot commented Mar 18, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Mar 18, 2020
@wcandillon
Copy link

This small PR would really be useful. Thank you for this @airjp73.

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Mar 18, 2020
@gaearon
Copy link
Collaborator

gaearon commented Apr 1, 2020

Most people really shouldn't use it. It's more of an escape hatch for tightly controlled cases. I think I'd prefer this stays undocumented. People who really need it will find it, but generally this isn't something you want to enable.

@gaearon gaearon closed this Apr 1, 2020
@dclowd9901
Copy link

dclowd9901 commented Apr 9, 2020

Most people really shouldn't use it. It's more of an escape hatch for tightly controlled cases. I think I'd prefer this stays undocumented. People who really need it will find it, but generally this isn't something you want to enable.

With all due respect, this is silly reasoning. This is hardly a dangerous footgun, and is incredibly useful to devs who write hooks (basically any React developer at this point). If an org finds this isn't an effective way to set standards internally, they can (and will) easily change the way they do things, but to make that decision for them is addressing a problem that doesn't exist.

It should be easy to find. I nearly wrote my own version of this rule just for this problem. Thankfully I had the good sense to check the source code, but not everyone has that kind of time.

@mbest
Copy link

mbest commented Apr 9, 2020

I'm trying to figure out how to use this option. Here is what I've added to package.json:

  "eslintConfig": {
    "extends": "react-app",
    "rules": {
      "react-hooks/exhaustive-deps": [
        "warn",
        {
          "additionalHooks": "^(useDeepCompareLayoutEffect)$"
        }
      ]
    }
  },

But I don't get any warning. To test I replaced useDeepCompareLayoutEffect with useLayoutEffect in the code and do get a warning when running yarn start:

Compiled with warnings.

./src/core/providers/DataProvider.tsx
  Line 54:12:  React Hook useLayoutEffect has missing dependencies: 'otherProps' and 'setValue'.
  Either include them or remove the dependency array  react-hooks/exhaustive-deps

Search for the keywords to learn more about each warning.
To ignore, add // eslint-disable-next-line to the line before.

Any ideas?

@gaearon
Copy link
Collaborator

gaearon commented Apr 9, 2020

FWIW I'd take a PR that enables the check for any Hook ending with Effect. That seems legit to me.

@airjp73
Copy link
Contributor Author

airjp73 commented Apr 9, 2020

That sounds like a reasonable solution to me. I'd love to take a crack at that some time this week / weekend.

@dclowd9901
Copy link

@mbest have you tried not matching start and end of line? I don't know if it matters, but I do not have that distinction on mine, and it works fine.

@mbest
Copy link

mbest commented Apr 12, 2020

Since we're using Create React App, I had to set the EXTEND_ESLINT environment variable to enable customization of the eslint rules.

@gaearon
Copy link
Collaborator

gaearon commented May 26, 2020

We've tried the heuristic, but I had to revert it because too many false positives are being reported (#19004). So I'm happy to take this PR as an alternative, if you'd like to resubmit it.

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.

None yet

8 participants