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] Forbid top-level use*() calls #16455

Merged
merged 3 commits into from
Aug 19, 2019
Merged

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 19, 2019

In the initial release of eslint-plugin-react-hooks/rules-of-hooks, we forbid this:

function foo() {
  useState(); // BAD: calling a Hook from a non-Hook
}

But we didn't forbid this:

// top-level code

useState(); // ALSO BAD: calling a Hook from top level

This PR forbids it in the lint rule.

Why did it work before?

We didn't ban it at the time for three reasons:

  1. We didn't know how Hooks would be received by the community. Taking the whole use* prefix seemed dicey and we didn't want to push the convention harder than absolutely necessary.
  2. It is a runtime crash anyway, so you find out about this immediately.
  3. There's a history library whose 2.x versions had an API like this:
const {createHistory, useBasename} = require('history-2.1.2');
const browserHistory = useBasename(createHistory)({
  basename: '/',
});

So banning this pattern would create a false positive for it.

Why ban it now?

If your environment uses "inline requires" (opt-in on React Native), you might not get a crash at all. Instead, the top level initialization of a module may run as a result of some component's render, and so Hooks would accidentally "belong" to the parent. That's confusing. While "inline requires" aren't very common on the web, they're a powerful optimization, so it's plausible they will get used more often with time as build systems add support for them.

Even regardless of that risk, we can confidently say Hooks have been a successful rollout now. So whether the bugs themselves are a problem or not, the use convention effectively already strongly implies it's a Hook. From that perspective, use*() at top level is just confusing to look at.

But I'm not using a Hook!

There are some older versions of history and react-router that had use-prefixed APIs for things that aren't Hooks. If you see a false positive, please feel free to suppress it:

// eslint-disable-next-line react-hooks/rules-of-hooks
const browserHistory = useBasename(createHistory)({
  basename: '/foo'
});

This is not ideal, but this isn't an issue in recent versions of these libraries — and also most apps only have at most a single file with this issue.

If you for some reason can't add suppressions, you may alternatively:

  • Rename it during import like import { useBasename as withBasename } from 'history'
  • Or use import * as History from 'history' and History.useBasename()

Note this isn't just to "appease the linter". People expect use prefix to only be used by Hooks now, so it's good to push the ecosystem towards consistent naming.

What about MyLibrary.useFoo()?

We warn about invalid use of use*() and React.use*(), but not MyLibrary.use*(). This is because initially we saw too many false positives from modules like MyStore.useNewAnalytics().

The new top-level error respects the same rule. For example, it would warn for useBasename(), but not for History.useBasename(). So that's another escape hatch if you want to suppress it.

// Doesn't warn
const browserHistory = History.useBasename(History.createHistory)({
  basename: '/foo'
});

In the future, we might consider making it stricter, and banning invalid calls to MyLibrary.use*() too. But it's less common in general because the convention is to import Hooks directly without a namespace. It's also much more commonly encountered in open source, like app.use() from Express, or jest.useFakeTimers(). So maybe it's not worth it.

@sizebot
Copy link

sizebot commented Aug 19, 2019

Details of bundled changes.

Comparing: e89c19d...dfd0407

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.4% -0.0% 75.01 KB 75.31 KB 17.32 KB 17.32 KB NODE_DEV
eslint-plugin-react-hooks.production.min.js 🔺+1.0% -0.2% 19.95 KB 20.14 KB 6.91 KB 6.9 KB NODE_PROD
ESLintPluginReactHooks-dev.js +0.5% +0.1% 80.5 KB 80.87 KB 17.83 KB 17.84 KB FB_WWW_DEV

Generated by 🚫 dangerJS

This is not a change. Just adding more coverage.
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.

Thanks for the helpful context in the PR description.

};
tests.valid = tests.valid.filter(predicate);
tests.invalid = tests.invalid.filter(predicate);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 19, 2019

Note this is a breaking change for the plugin since it would become more aggressive.

@gaearon gaearon merged commit 96eb703 into facebook:master Aug 19, 2019
@gaearon gaearon deleted the lintstrict branch August 19, 2019 18:56
threepointone added a commit to threepointone/react that referenced this pull request Aug 21, 2019
threepointone pushed a commit that referenced this pull request Aug 21, 2019
gaearon added a commit that referenced this pull request Aug 21, 2019
gaearon added a commit that referenced this pull request Aug 21, 2019
gaearon added a commit that referenced this pull request Aug 21, 2019
…6525)

* Revert "Revert "[ESLint] Forbid top-level use*() calls (#16455)" (#16522)"

This reverts commit 507f0fb.

* Update RulesOfHooks.js
arcticicestudio added a commit to svengreb/styleguide-javascript that referenced this pull request Mar 29, 2021
As of version `^2.0.0` of `eslint-plugin-react-hooks` [1] the plugin now
forbids top-level `use*()` calls more aggressively [2].
See the official changelog and corresponding commits [3] for more
details.

To adapt to this change the peer & dev dependency version has been
updated to the latest minor/patch version [4].

[1]: https://github.com/facebook/react/tree/master/packages/eslint-plugin-react-hooks
[2]: facebook/react#16455
[3]: https://github.com/facebook/react/pull/16528/files
[4]: https://www.npmjs.com/package/eslint-plugin-react-hooks

Co-authored-by: Sven Greb <development@svengreb.de>

GH-23
arcticicestudio added a commit to svengreb/styleguide-javascript that referenced this pull request Mar 29, 2021
As of version `^2.0.0` of `eslint-plugin-react-hooks` [1] the plugin now
forbids top-level `use*()` calls more aggressively [2].
See the official changelog and corresponding commits [3] for more
details.

To adapt to this change the peer & dev dependency version has been
updated to the latest minor/patch version [4].

[1]: https://github.com/facebook/react/tree/master/packages/eslint-plugin-react-hooks
[2]: facebook/react#16455
[3]: https://github.com/facebook/react/pull/16528/files
[4]: https://www.npmjs.com/package/eslint-plugin-react-hooks

Co-authored-by: Sven Greb <development@svengreb.de>

GH-23
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