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

[ESLint] Warn about dependencies outside of render scope #14990

Merged
merged 1 commit into from
Mar 1, 2019

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Mar 1, 2019

Adds a better warning for these cases:

// Case 1: watching a global (this is broken)
useEffect(() => {
  console.log(window.innerWidth)
}, [window.innerWidth])
useEffect(() => {
  console.log(GlobalStore.thing)
}, [GlobalStore.thing])

// Case 2: just unnecessary dep
useEffect(() => {
  console.log('hello')
}, [console])

We used to warn before, too, but the message wasn't very specific (it just said it's unused). Later #14967 disabled that message for effects, but that wasn't intentional. This particular case should still warn.

This PR re-adds the warnings along with a bunch of new tests covering it. It detects if we used an identifier that's outside the component scope, and warns that it's essentially a mutable value.

In the docs, we'll explain that the fix is either to omit it, or (if you really want to track it), put it into state and update appropriately.

@sizebot
Copy link

sizebot commented Mar 1, 2019

Details of bundled changes.

Comparing: df7b476...350fafe

eslint-plugin-react-hooks

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
eslint-plugin-react-hooks.development.js +1.7% +1.4% 55.61 KB 56.57 KB 13 KB 13.19 KB NODE_DEV
eslint-plugin-react-hooks.production.min.js 🔺+2.5% 🔺+2.6% 13.86 KB 14.21 KB 4.82 KB 4.94 KB NODE_PROD
ESLintPluginReactHooks-dev.js +1.8% +1.4% 59.16 KB 60.22 KB 13.34 KB 13.53 KB FB_WWW_DEV

Generated by 🚫 dangerJS

@gaearon gaearon merged commit 59ef284 into facebook:master Mar 1, 2019
@gaearon gaearon deleted the rule-moar branch March 1, 2019 18:17
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.

4 participants