From 96eb703bbff49b7d52ad7d41ea18074dc8e7857a Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 19 Aug 2019 19:54:06 +0100 Subject: [PATCH] [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. --- .../__tests__/ESLintRulesOfHooks-test.js | 126 ++++++++++++++++-- .../src/RulesOfHooks.js | 6 +- 2 files changed, 119 insertions(+), 13 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js index 635196f8db..c6fc1746e1 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js @@ -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. @@ -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: '/', }); `, @@ -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 { @@ -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); diff --git a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js index 405a6641a3..fff4566cfb 100644 --- a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js +++ b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js @@ -432,9 +432,13 @@ 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`. + 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