-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Conversation
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! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Related: reactjs/react.dev#2350 |
@airjp73 Thks for contributing this change. I hope this gets merged. This PR can be marked a fixing reactjs/react.dev#2350 |
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. |
This small PR would really be useful. Thank you for this @airjp73. |
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. |
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
Any ideas? |
FWIW I'd take a PR that enables the check for any Hook ending with Effect. That seems legit to me. |
That sounds like a reasonable solution to me. I'd love to take a crack at that some time this week / weekend. |
@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. |
Since we're using Create React App, I had to set the |
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. |
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.