From 70b40ccb9b9f29e8d74c25575e4d2fe7b3354b33 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Wed, 21 Aug 2019 10:09:48 +0100 Subject: [PATCH] Revert "[ESLint] Forbid top-level use*() calls (#16455)" This reverts commit 96eb703bbff49b7d52ad7d41ea18074dc8e7857a. --- .../__tests__/ESLintRulesOfHooks-test.js | 126 ++---------------- .../src/RulesOfHooks.js | 6 +- 2 files changed, 13 insertions(+), 119 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js index c6fc1746e1351..635196f8db529 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js @@ -19,17 +19,8 @@ ESLintTester.setDefaultConfig({ }, }); -// *************************************************** -// For easier local testing, you can add to any case: -// { -// skip: true, -// --or-- -// only: true, -// ... -// } -// *************************************************** - -const tests = { +const eslintTester = new ESLintTester(); +eslintTester.run('react-hooks', ReactHooksESLintRule, { valid: [ ` // Valid because components can use hooks. @@ -232,20 +223,21 @@ const tests = { (class {i() { useState(); }}); `, ` - // Valid because they're not matching use[A-Z]. - fooState(); + // Currently valid although we *could* consider these invalid. + // It doesn't make a lot of difference because it would crash early. use(); _use(); + useState(); _useState(); + use42(); + useHook(); use_hook(); + React.useState(); `, ` - // 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)({ + // Regression test for the popular "history" library + const {createHistory, useBasename} = require('history-2.1.2'); + const browserHistory = useBasename(createHistory)({ basename: '/', }); `, @@ -677,63 +669,8 @@ const tests = { 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 { @@ -771,42 +708,3 @@ 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 fff4566cfbe61..405a6641a335d 100644 --- a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js +++ b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js @@ -432,13 +432,9 @@ 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