Skip to content

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