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] Allow extraneous effect dependencies #14967

Merged
merged 1 commit into from
Feb 27, 2019

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Feb 27, 2019

This makes cases like

  useEffect(() => {
    window.scrollTo(0, 0);
  }, [activeTab]);

not warn.

However, it still warns for unused useCallback/useMemo deps.

Sebastian didn't disagree with this.

Resolves #14920 (comment).

This makes cases like

  useEffect(() => {
    window.scrollTo(0, 0);
  }, [activeTab]);

not warn.

However, it still warns for unused useCallback/useMemo deps.
@sizebot
Copy link

sizebot commented Feb 27, 2019

Details of bundled changes.

Comparing: 00748c5...a65912d

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.9% +1.0% 49.28 KB 49.71 KB 11.62 KB 11.73 KB NODE_DEV
eslint-plugin-react-hooks.production.min.js 🔺+0.4% 🔺+0.1% 12.31 KB 12.37 KB 4.47 KB 4.47 KB NODE_PROD
ESLintPluginReactHooks-dev.js +0.8% +0.9% 52.81 KB 53.24 KB 11.95 KB 12.06 KB FB_WWW_DEV

Generated by 🚫 dangerJS

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sebastian didn't disagree with this.

😆

I have somewhat mixed feelings about this change, but I trust your intuition that this is the right tradeoff based on the real world use cases you've been looking at 👍

@gaearon gaearon merged commit 3ada82b into facebook:master Feb 27, 2019
@gaearon gaearon deleted the fx-tweaks branch February 27, 2019 16:59
@Janpot
Copy link

Janpot commented Feb 27, 2019

@gaearon FYI: Not sure if this was the intention, but your PR comment automatically closed #14920

@bvaughn
Copy link
Contributor

bvaughn commented Feb 27, 2019

Reopened~

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

5 participants