Skip to content

Commit

Permalink
Revert "Revert "[ESLint] Forbid top-level use*() calls (#16455)"" (#1…
Browse files Browse the repository at this point in the history
…6525)

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

This reverts commit 507f0fb.

* Update RulesOfHooks.js
  • Loading branch information
gaearon authored Aug 21, 2019
1 parent 507f0fb commit c433fbb
Show file tree
Hide file tree
Showing 2 changed files with 120 additions and 15 deletions.
126 changes: 114 additions & 12 deletions packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,17 @@ ESLintTester.setDefaultConfig({
},
});

const eslintTester = new ESLintTester();
eslintTester.run('react-hooks', ReactHooksESLintRule, {
// ***************************************************
// For easier local testing, you can add to any case:
// {
// skip: true,
// --or--
// only: true,
// ...
// }
// ***************************************************

const tests = {
valid: [
`
// Valid because components can use hooks.
Expand Down Expand Up @@ -223,21 +232,20 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, {
(class {i() { useState(); }});
`,
`
// Currently valid although we *could* consider these invalid.
// It doesn't make a lot of difference because it would crash early.
// Valid because they're not matching use[A-Z].
fooState();
use();
_use();
useState();
_useState();
use42();
useHook();
use_hook();
React.useState();
`,
`
// Regression test for the popular "history" library
const {createHistory, useBasename} = require('history-2.1.2');
const browserHistory = useBasename(createHistory)({
// This is grey area.
// Currently it's valid (although React.useCallback would fail here).
// We could also get stricter and disallow it, just like we did
// with non-namespace use*() top-level calls.
const History = require('history-2.1.2');
const browserHistory = History.useBasename(History.createHistory)({
basename: '/',
});
`,
Expand Down Expand Up @@ -669,8 +677,63 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, {
conditionalError('useState'),
],
},
{
code: `
// Invalid because it's dangerous and might not warn otherwise.
// This *must* be invalid.
function useHook({ bar }) {
let foo1 = bar && useState();
let foo2 = bar || useState();
let foo3 = bar ?? useState();
}
`,
errors: [
conditionalError('useState'),
conditionalError('useState'),
// TODO: ideally this *should* warn, but ESLint
// doesn't plan full support for ?? until it advances.
// conditionalError('useState'),
],
},
{
code: `
// Invalid because it's dangerous.
// Normally, this would crash, but not if you use inline requires.
// This *must* be invalid.
// It's expected to have some false positives, but arguably
// they are confusing anyway due to the use*() convention
// already being associated with Hooks.
useState();
if (foo) {
const foo = React.useCallback(() => {});
}
useCustomHook();
`,
errors: [
topLevelError('useState'),
topLevelError('React.useCallback'),
topLevelError('useCustomHook'),
],
},
{
code: `
// Technically this is a false positive.
// We *could* make it valid (and it used to be).
//
// However, top-level Hook-like calls can be very dangerous
// in environments with inline requires because they can mask
// the runtime error by accident.
// So we prefer to disallow it despite the false positive.
const {createHistory, useBasename} = require('history-2.1.2');
const browserHistory = useBasename(createHistory)({
basename: '/',
});
`,
errors: [topLevelError('useBasename')],
},
],
});
};

function conditionalError(hook, hasPreviousFinalizer = false) {
return {
Expand Down Expand Up @@ -708,3 +771,42 @@ function genericError(hook) {
'Hook function.',
};
}

function topLevelError(hook) {
return {
message:
`React Hook "${hook}" cannot be called at the top level. React Hooks ` +
'must be called in a React function component or a custom React ' +
'Hook function.',
};
}

// For easier local testing
if (!process.env.CI) {
let only = [];
let skipped = [];
[...tests.valid, ...tests.invalid].forEach(t => {
if (t.skip) {
delete t.skip;
skipped.push(t);
}
if (t.only) {
delete t.only;
only.push(t);
}
});
const predicate = t => {
if (only.length > 0) {
return only.indexOf(t) !== -1;
}
if (skipped.length > 0) {
return skipped.indexOf(t) === -1;
}
return true;
};
tests.valid = tests.valid.filter(predicate);
tests.invalid = tests.invalid.filter(predicate);
}

const eslintTester = new ESLintTester();
eslintTester.run('react-hooks', ReactHooksESLintRule, tests);
9 changes: 6 additions & 3 deletions packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -432,9 +432,12 @@ export default {
'React Hook function.';
context.report({node: hook, message});
} else if (codePathNode.type === 'Program') {
// For now, ignore if it's in top level scope.
// We could warn here but there are false positives related
// configuring libraries like `history`.
// These are dangerous if you have inline requires enabled.
const message =
`React Hook "${context.getSource(hook)}" cannot be called ` +
'at the top level. React Hooks must be called in a ' +
'React function component or a custom React Hook function.';
context.report({node: hook, message});
} else {
// Assume in all other cases the user called a hook in some
// random function callback. This should usually be true for
Expand Down

0 comments on commit c433fbb

Please sign in to comment.