[ESLint] Forbid top-level use*() calls (#16455)
* Add a way to skip/only tests to RulesOfHooks test * [ESLint] Forbid top-level use*() calls * Add a regression test for logical expressions This is not a change. Just adding more coverage.
This commit is contained in:
parent
56f93a7f38
commit
96eb703bbf
|
@ -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: [
|
||||||
`
|
`
|
||||||
// Valid because components can use hooks.
|
// Valid because components can use hooks.
|
||||||
|
@ -223,21 +232,20 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, {
|
||||||
(class {i() { useState(); }});
|
(class {i() { useState(); }});
|
||||||
`,
|
`,
|
||||||
`
|
`
|
||||||
// Currently valid although we *could* consider these invalid.
|
// Valid because they're not matching use[A-Z].
|
||||||
// It doesn't make a lot of difference because it would crash early.
|
fooState();
|
||||||
use();
|
use();
|
||||||
_use();
|
_use();
|
||||||
useState();
|
|
||||||
_useState();
|
_useState();
|
||||||
use42();
|
|
||||||
useHook();
|
|
||||||
use_hook();
|
use_hook();
|
||||||
React.useState();
|
|
||||||
`,
|
`,
|
||||||
`
|
`
|
||||||
// Regression test for the popular "history" library
|
// This is grey area.
|
||||||
const {createHistory, useBasename} = require('history-2.1.2');
|
// Currently it's valid (although React.useCallback would fail here).
|
||||||
const browserHistory = useBasename(createHistory)({
|
// 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: '/',
|
basename: '/',
|
||||||
});
|
});
|
||||||
`,
|
`,
|
||||||
|
@ -669,8 +677,63 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, {
|
||||||
conditionalError('useState'),
|
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) {
|
function conditionalError(hook, hasPreviousFinalizer = false) {
|
||||||
return {
|
return {
|
||||||
|
@ -708,3 +771,42 @@ function genericError(hook) {
|
||||||
'Hook function.',
|
'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);
|
||||||
|
|
|
@ -432,9 +432,13 @@ export default {
|
||||||
'React Hook function.';
|
'React Hook function.';
|
||||||
context.report({node: hook, message});
|
context.report({node: hook, message});
|
||||||
} else if (codePathNode.type === 'Program') {
|
} 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
|
// We could warn here but there are false positives related
|
||||||
// configuring libraries like `history`.
|
// configuring libraries like `history`.
|
||||||
|
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 {
|
} else {
|
||||||
// Assume in all other cases the user called a hook in some
|
// Assume in all other cases the user called a hook in some
|
||||||
// random function callback. This should usually be true for
|
// random function callback. This should usually be true for
|
||||||
|
|
Loading…
Reference in New Issue