From c0eafd8e6a1cacbb05bf313392c0baecca397998 Mon Sep 17 00:00:00 2001 From: Caleb Meredith Date: Wed, 31 Oct 2018 18:53:33 +0000 Subject: [PATCH 01/33] Add ESLint rule for useEffect/useCallback/useMemo Hook dependencies --- .../ESLintRuleReactiveDependencies-test.js | 539 ++++++++++++++++++ .../src/ReactiveDependencies.js | 322 +++++++++++ .../eslint-plugin-react-hooks/src/index.js | 2 + 3 files changed, 863 insertions(+) create mode 100644 packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js create mode 100644 packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js new file mode 100644 index 000000000000..e5113ae4778e --- /dev/null +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js @@ -0,0 +1,539 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +'use strict'; + +const ESLintTester = require('eslint').RuleTester; +const ReactHooksESLintPlugin = require('eslint-plugin-react-hooks'); +const ReactHooksESLintRule = + ReactHooksESLintPlugin.rules['reactive-dependencies']; + +ESLintTester.setDefaultConfig({ + parser: 'babel-eslint', + parserOptions: { + ecmaVersion: 6, + sourceType: 'module', + }, +}); + +const eslintTester = new ESLintTester(); +eslintTester.run('react-hooks', ReactHooksESLintRule, { + valid: [ + ` + const local = 42; + useEffect(() => { + console.log(local); + }, []); + `, + ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + }); + } + `, + ` + function MyComponent() { + useEffect(() => { + const local = 42; + console.log(local); + }, []); + } + `, + ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + }, [local]); + } + `, + ` + const local1 = 42; + { + const local2 = 42; + useEffect(() => { + console.log(local1); + console.log(local2); + }, []); + } + `, + ` + function MyComponent() { + const local1 = 42; + { + const local2 = 42; + useEffect(() => { + console.log(local1); + console.log(local2); + }); + } + } + `, + ` + function MyComponent() { + const local1 = 42; + { + const local2 = 42; + useEffect(() => { + console.log(local1); + console.log(local2); + }, [local1, local2]); + } + } + `, + ` + function MyComponent() { + const local1 = 42; + function MyNestedComponent() { + const local2 = 42; + useEffect(() => { + console.log(local1); + console.log(local2); + }, [local2]); + } + } + `, + ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + console.log(local); + }, [local]); + } + `, + ` + function MyComponent() { + useEffect(() => { + console.log(unresolved); + }, []); + } + `, + ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + }, [,,,local,,,]); + } + `, + ` + function MyComponent(props) { + useEffect(() => { + console.log(props.foo); + }, [props.foo]); + } + `, + ` + function MyComponent(props) { + useEffect(() => { + console.log(props.foo); + console.log(props.bar); + }, [props.foo, props.bar]); + } + `, + ` + function MyComponent(props) { + const local = 42; + useEffect(() => { + console.log(props.foo); + console.log(props.bar); + console.log(local); + }, [props.foo, props.bar, local]); + } + `, + { + code: ` + function MyComponent(props) { + useCustomEffect(() => { + console.log(props.foo); + }); + } + `, + options: [{additionalHooks: 'useCustomEffect'}], + }, + { + code: ` + function MyComponent(props) { + useCustomEffect(() => { + console.log(props.foo); + }, [props.foo]); + } + `, + options: [{additionalHooks: 'useCustomEffect'}], + }, + ], + invalid: [ + { + code: ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + }, []); + } + `, + errors: [missingError('local')], + }, + { + code: ` + function MyComponent() { + const local1 = 42; + { + const local2 = 42; + useEffect(() => { + console.log(local1); + console.log(local2); + }, []); + } + } + `, + errors: [missingError('local1'), missingError('local2')], + }, + { + code: ` + function MyComponent() { + const local1 = 42; + const local2 = 42; + useEffect(() => { + console.log(local1); + console.log(local2); + }, [local1]); + } + `, + errors: [missingError('local2')], + }, + { + code: ` + function MyComponent() { + const local1 = 42; + const local2 = 42; + useEffect(() => { + console.log(local1); + }, [local1, local2]); + } + `, + errors: [extraError('local2')], + }, + { + code: ` + function MyComponent() { + const local1 = 42; + function MyNestedComponent() { + const local2 = 42; + useEffect(() => { + console.log(local1); + console.log(local2); + }, [local1]); + } + } + `, + errors: [missingError('local2'), extraError('local1')], + }, + { + code: ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + console.log(local); + }, []); + } + `, + errors: [missingError('local'), missingError('local')], + }, + { + code: ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + console.log(local); + }, [local, local]); + } + `, + errors: [duplicateError('local')], + }, + { + code: ` + function MyComponent() { + useEffect(() => {}, [local]); + } + `, + errors: [extraError('local')], + }, + { + code: ` + function MyComponent() { + const dependencies = []; + useEffect(() => {}, dependencies); + } + `, + errors: [ + 'React Hook useEffect has a second argument which is not an array ' + + "literal. This means we can't statically verify whether you've " + + 'passed the correct dependencies.', + ], + }, + { + code: ` + function MyComponent() { + const local = 42; + const dependencies = [local]; + useEffect(() => { + console.log(local); + }, dependencies); + } + `, + errors: [ + missingError('local'), + 'React Hook useEffect has a second argument which is not an array ' + + "literal. This means we can't statically verify whether you've " + + 'passed the correct dependencies.', + ], + }, + { + code: ` + function MyComponent() { + const local = 42; + const dependencies = [local]; + useEffect(() => { + console.log(local); + }, [...dependencies]); + } + `, + errors: [ + missingError('local'), + 'React Hook useEffect has a spread element in its dependency list. ' + + "This means we can't statically verify whether you've passed the " + + 'correct dependencies.', + ], + }, + { + code: ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + }, [local, ...dependencies]); + } + `, + errors: [ + 'React Hook useEffect has a spread element in its dependency list. ' + + "This means we can't statically verify whether you've passed the " + + 'correct dependencies.', + ], + }, + { + code: ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + }, [computeCacheKey(local)]); + } + `, + errors: [ + missingError('local'), + "Unsupported expression in React Hook useEffect's dependency list. " + + 'Currently only simple variables are supported.', + ], + }, + { + code: ` + function MyComponent() { + const local = {id: 42}; + useEffect(() => { + console.log(local); + }, [local.id]); + } + `, + errors: [missingError('local'), extraError('local.id')], + }, + { + code: ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + }, [local, local]); + } + `, + errors: [duplicateError('local')], + }, + { + code: ` + function MyComponent() { + const local1 = 42; + useEffect(() => { + const local1 = 42; + console.log(local1); + }, [local1]); + } + `, + errors: [extraError('local1')], + }, + { + code: ` + function MyComponent() { + const local1 = 42; + useEffect(() => {}, [local1]); + } + `, + errors: [extraError('local1')], + }, + { + code: ` + function MyComponent(props) { + useEffect(() => { + console.log(props.foo); + }, []); + } + `, + errors: [missingError('props.foo')], + }, + { + code: ` + function MyComponent(props) { + useEffect(() => { + console.log(props.foo); + console.log(props.bar); + }, []); + } + `, + errors: [missingError('props.foo'), missingError('props.bar')], + }, + { + code: ` + function MyComponent(props) { + const local = 42; + useEffect(() => { + console.log(props.foo); + console.log(props.bar); + console.log(local); + }, []); + } + `, + errors: [ + missingError('props.foo'), + missingError('props.bar'), + missingError('local'), + ], + }, + { + code: ` + function MyComponent(props) { + useEffect(() => { + console.log(props.foo); + }, []); + useCallback(() => { + console.log(props.foo); + }, []); + useMemo(() => { + console.log(props.foo); + }, []); + React.useEffect(() => { + console.log(props.foo); + }, []); + React.useCallback(() => { + console.log(props.foo); + }, []); + React.useMemo(() => { + console.log(props.foo); + }, []); + React.notReactiveHook(() => { + console.log(props.foo); + }, []); + } + `, + errors: [ + missingError('props.foo', 'useEffect'), + missingError('props.foo', 'useCallback'), + missingError('props.foo', 'useMemo'), + missingError('props.foo', 'React.useEffect'), + missingError('props.foo', 'React.useCallback'), + missingError('props.foo', 'React.useMemo'), + ], + }, + { + code: ` + function MyComponent(props) { + useCustomEffect(() => { + console.log(props.foo); + }, []); + useEffect(() => { + console.log(props.foo); + }, []); + React.useEffect(() => { + console.log(props.foo); + }, []); + React.useCustomEffect(() => { + console.log(props.foo); + }, []); + } + `, + options: [{additionalHooks: 'useCustomEffect'}], + errors: [ + missingError('props.foo', 'useCustomEffect'), + missingError('props.foo', 'useEffect'), + missingError('props.foo', 'React.useEffect'), + ], + }, + { + code: ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + }, [a ? local : b]); + } + `, + errors: [ + missingError('local'), + "Unsupported expression in React Hook useEffect's dependency list. " + + 'Currently only simple variables are supported.', + ], + }, + { + code: ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + }, [a && local]); + } + `, + errors: [ + missingError('local'), + "Unsupported expression in React Hook useEffect's dependency list. " + + 'Currently only simple variables are supported.', + ], + }, + ], +}); + +function missingError(dependency, hook = 'useEffect') { + return ( + `React Hook ${hook} references "${dependency}", but it was not listed in ` + + `the hook dependencies argument. This means if "${dependency}" changes ` + + `then ${hook} won't be able to update.` + ); +} + +function extraError(dependency) { + return ( + `React Hook useEffect has an extra dependency "${dependency}" which is ` + + `not used in its callback. Removing this dependency may mean the hook ` + + `needs to execute fewer times.` + ); +} + +function duplicateError(dependency) { + return `Duplicate value in React Hook useEffect's dependency list for "${dependency}".`; +} diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js new file mode 100644 index 000000000000..5bd791153203 --- /dev/null +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js @@ -0,0 +1,322 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/* eslint-disable no-for-of-loops/no-for-of-loops */ + +'use strict'; + +/** + * Is this node a reactive React Hook? This includes: + * + * - `useEffect()` + * - `useCallback()` + * - `useMemo()` + * + * TODO: handle all useEffect() variants. + * TODO: implement autofix. + * + * Also supports `React` namespacing. e.g. `React.useEffect()`. + * + * NOTE: This is a very naive check. We don't look to make sure these reactive + * hooks are imported correctly. + */ +function isReactiveHook(node, options) { + if ( + node.type === 'MemberExpression' && + node.object.type === 'Identifier' && + node.object.name === 'React' && + node.property.type === 'Identifier' && + !node.computed + ) { + return isReactiveHook(node.property); + } else if ( + node.type === 'Identifier' && + (node.name === 'useEffect' || + node.name === 'useCallback' || + node.name === 'useMemo') + ) { + return true; + } else if (options && options.additionalHooks) { + // Allow the user to provide a regular expression which enables the lint to + // target custom reactive hooks. + let name; + try { + name = getAdditionalHookName(node); + } catch (error) { + if (/Unsupported node type/.test(error.message)) { + return false; + } else { + throw error; + } + } + return options.additionalHooks.test(name); + } else { + return false; + } +} + +/** + * Create a name we will test against our `additionalHooks` regular expression. + */ +function getAdditionalHookName(node) { + if (node.type === 'Identifier') { + return node.name; + } else if (node.type === 'MemberExpression' && !node.computed) { + const object = getAdditionalHookName(node.object); + const property = getAdditionalHookName(node.property); + return `${object}.${property}`; + } else { + throw new Error(`Unsupported node type: ${node.type}`); + } +} + +/** + * Is this node the callback for a reactive hook? It is if the parent is a call + * expression with a reactive hook callee and this node is a function expression + * and the first argument. + */ +function isReactiveHookCallback(node, options) { + return ( + (node.type === 'FunctionExpression' || + node.type === 'ArrowFunctionExpression') && + node.parent.type === 'CallExpression' && + isReactiveHook(node.parent.callee, options) && + node.parent.arguments[0] === node + ); +} + +export default { + meta: { + schema: [ + { + type: 'object', + additionalProperties: false, + properties: { + additionalHooks: { + type: 'string', + }, + }, + }, + ], + }, + create(context) { + // Parse the `additionalHooks` regex. + const additionalHooks = + context.options && + context.options[0] && + context.options[0].additionalHooks + ? new RegExp(context.options[0].additionalHooks) + : undefined; + const options = {additionalHooks}; + + return { + FunctionExpression: visitFunctionExpression, + ArrowFunctionExpression: visitFunctionExpression, + }; + + /** + * Visitor for both function expressions and arrow function expressions. + */ + function visitFunctionExpression(node) { + // We only want to lint nodes which are reactive hook callbacks. + if (!isReactiveHookCallback(node, options)) return; + + // Get the reactive hook node. + const reactiveHook = node.parent.callee; + + // Get the declared dependencies for this reactive hook. If there is no + // second argument then the reactive callback will re-run on every render. + // So no need to check for dependency inclusion. + const declaredDependenciesNode = node.parent.arguments[1]; + if (!declaredDependenciesNode) return; + + // Get the current scope. + const scope = context.getScope(); + + // Find all our "pure scopes". On every re-render of a component these + // pure scopes may have changes to the variables declared within. So all + // variables used in our reactive hook callback but declared in a pure + // scope need to be listed as dependencies of our reactive hook callback. + // + // According to the rules of React you can't read a mutable value in pure + // scope. We can't enforce this in a lint so we trust that all variables + // declared outside of pure scope are indeed frozen. + const pureScopes = new Set(); + { + let currentScope = scope.upper; + while (currentScope) { + pureScopes.add(currentScope); + if (currentScope.type === 'function') break; + currentScope = currentScope.upper; + } + // If there is no parent function scope then there are no pure scopes. + // The ones we've collected so far are incorrect. So don't continue with + // the lint. + if (!currentScope) return; + } + + // Get dependencies from all our resolved references in pure scopes. + const dependencies = new Map(); + for (const reference of scope.references) { + // If this reference is not resolved or it is not declared in a pure + // scope then we don't care about this reference. + if (!reference.resolved) continue; + if (!pureScopes.has(reference.resolved.scope)) continue; + // Narrow the scope of a dependency if it is, say, a member expression. + // Then normalize the narrowed dependency. + const dependencyNode = getDependency(reference.identifier); + const dependency = normalizeDependencyNode(dependencyNode); + // Add the dependency to a map so we can make sure it is referenced + // again in our dependencies array. + let nodes = dependencies.get(dependency); + if (!nodes) dependencies.set(dependency, (nodes = [])); + nodes.push(dependencyNode); + } + + // Get all of the declared dependencies and put them in a set. We will + // compare this to the dependencies we found in the callback later. + const declaredDependencies = new Map(); + if (declaredDependenciesNode.type !== 'ArrayExpression') { + // If the declared dependencies are not an array expression then we + // can't verify that the user provided the correct dependencies. Tell + // the user this in an error. + context.report( + declaredDependenciesNode, + `React Hook ${context.getSource(reactiveHook)} has a second ` + + "argument which is not an array literal. This means we can't " + + "statically verify whether you've passed the correct dependencies.", + ); + } else { + for (const declaredDependencyNode of declaredDependenciesNode.elements) { + // Skip elided elements. + if (declaredDependencyNode === null) continue; + // If we see a spread element then add a special warning. + if (declaredDependencyNode.type === 'SpreadElement') { + context.report( + declaredDependencyNode, + `React Hook ${context.getSource(reactiveHook)} has a spread ` + + "element in its dependency list. This means we can't " + + "statically verify whether you've passed the " + + 'correct dependencies.', + ); + continue; + } + // Try to normalize the declared dependency. If we can't then an error + // will be thrown. We will catch that error and report an error. + let declaredDependency; + try { + declaredDependency = normalizeDependencyNode( + declaredDependencyNode, + ); + } catch (error) { + if (/Unexpected node type/.test(error.message)) { + context.report( + declaredDependencyNode, + 'Unsupported expression in React Hook ' + + `${context.getSource(reactiveHook)}'s dependency list. ` + + 'Currently only simple variables are supported.', + ); + continue; + } else { + throw error; + } + } + // If the programmer already declared this dependency then report a + // duplicate dependency error. + if (declaredDependencies.has(declaredDependency)) { + context.report( + declaredDependencyNode, + 'Duplicate value in React Hook ' + + `${context.getSource(reactiveHook)}'s dependency list for ` + + `"${context.getSource(declaredDependencyNode)}".`, + ); + } else { + // Add the dependency to our declared dependency map. + declaredDependencies.set( + declaredDependency, + declaredDependencyNode, + ); + } + } + } + + // Loop through all our dependencies to make sure they have been declared. + // If the dependency has not been declared we need to report some errors. + for (const [dependency, dependencyNodes] of dependencies) { + if (declaredDependencies.has(dependency)) { + // Yay! Our dependency has been declared. Delete it from + // `declaredDependencies` since we will report all remaining unused + // declared dependencies. + declaredDependencies.delete(dependency); + } else { + // Oh no! Our dependency was not declared. So report an error for all + // of the nodes which we expected to see an error from. + for (const dependencyNode of dependencyNodes) { + context.report( + dependencyNode, + `React Hook ${context.getSource(reactiveHook)} references ` + + `"${context.getSource(dependencyNode)}", but it was not ` + + 'listed in the hook dependencies argument. This means if ' + + `"${context.getSource(dependencyNode)}" changes then ` + + `${context.getSource(reactiveHook)} won't be able to update.`, + ); + } + } + } + + // Loop through all the unused declared dependencies and report a warning + // so the programmer removes the unused dependency from their list. + for (const declaredDependencyNode of declaredDependencies.values()) { + context.report( + declaredDependencyNode, + `React Hook ${context.getSource(reactiveHook)} has an extra ` + + `dependency "${context.getSource(declaredDependencyNode)}" which ` + + 'is not used in its callback. Removing this dependency may mean ' + + 'the hook needs to execute fewer times.', + ); + } + } + }, +}; + +/** + * Gets a dependency for our reactive callback from an identifier. If the + * identifier is the object part of a member expression then we use the entire + * member expression as a dependency. + * + * For instance, if we get `props` in `props.foo` then our dependency should be + * the full member expression. + */ +function getDependency(node) { + if ( + node.parent.type === 'MemberExpression' && + node.parent.object === node && + !node.parent.computed + ) { + return node.parent; + } else { + return node; + } +} + +/** + * Normalizes a dependency into a standard string representation which can + * easily be compared. + * + * Throws an error if the node type is not a valid dependency. + */ +function normalizeDependencyNode(node) { + if (node.type === 'Identifier') { + return node.name; + } else if (node.type === 'MemberExpression' && !node.parent.computed) { + const object = normalizeDependencyNode(node.object); + const property = normalizeDependencyNode(node.property); + return `${object}.${property}`; + } else { + throw new Error(`Unexpected node type: ${node.type}`); + } +} diff --git a/packages/eslint-plugin-react-hooks/src/index.js b/packages/eslint-plugin-react-hooks/src/index.js index a3d6216e097b..e828c8bca70c 100644 --- a/packages/eslint-plugin-react-hooks/src/index.js +++ b/packages/eslint-plugin-react-hooks/src/index.js @@ -8,7 +8,9 @@ 'use strict'; import RuleOfHooks from './RulesOfHooks'; +import ReactiveDependencies from './ReactiveDependencies'; export const rules = { 'rules-of-hooks': RuleOfHooks, + 'reactive-dependencies': ReactiveDependencies, }; From 6e2ba67b2f1e5771871d8914da4904a509e35424 Mon Sep 17 00:00:00 2001 From: Jamie Kyle Date: Wed, 31 Oct 2018 13:59:45 -0700 Subject: [PATCH 02/33] Fix ReactiveDependencies rule --- .../src/ReactiveDependencies.js | 70 ++++++++++++++++++- 1 file changed, 68 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js index 5bd791153203..d04763f67a94 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js @@ -168,7 +168,12 @@ export default { if (!pureScopes.has(reference.resolved.scope)) continue; // Narrow the scope of a dependency if it is, say, a member expression. // Then normalize the narrowed dependency. - const dependencyNode = getDependency(reference.identifier); + + const referenceNode = fastFindReferenceWithParent( + node, + reference.identifier, + ); + const dependencyNode = getDependency(referenceNode); const dependency = normalizeDependencyNode(dependencyNode); // Add the dependency to a map so we can make sure it is referenced // again in our dependencies array. @@ -312,7 +317,7 @@ function getDependency(node) { function normalizeDependencyNode(node) { if (node.type === 'Identifier') { return node.name; - } else if (node.type === 'MemberExpression' && !node.parent.computed) { + } else if (node.type === 'MemberExpression' && !node.computed) { const object = normalizeDependencyNode(node.object); const property = normalizeDependencyNode(node.property); return `${object}.${property}`; @@ -320,3 +325,64 @@ function normalizeDependencyNode(node) { throw new Error(`Unexpected node type: ${node.type}`); } } + +/** + * ESLint won't assign node.parent to references from context.getScope() + * + * So instead we search for the node from an ancestor assigning node.parent + * as we go. This mutates the AST. + * + * This traversal is: + * - optimized by only searching nodes with a range surrounding our target node + * - agnostic to AST node types, it looks for `{ type: string, ... }` + */ +function fastFindReferenceWithParent(start, target) { + let queue = [start]; + let item = null; + + while (queue.length) { + item = queue.shift(); + + if (isSameIdentifier(item, target)) return item; + if (!isAncestorNodeOf(item, target)) continue; + + for (let [key, value] of Object.entries(item)) { + if (key === 'parent') continue; + if (isNodeLike(value)) { + value.parent = item; + queue.push(value); + } else if (Array.isArray(value)) { + value.forEach(val => { + if (isNodeLike(val)) { + val.parent = item; + queue.push(val); + } + }); + } + } + } + + return null; +} + +function isNodeLike(val) { + return ( + typeof val === 'object' && + val !== null && + !Array.isArray(val) && + typeof val.type === 'string' + ); +} + +function isSameIdentifier(a, b) { + return ( + a.type === 'Identifier' && + a.name === b.name && + a.range[0] === b.range[0] && + a.range[1] === b.range[1] + ); +} + +function isAncestorNodeOf(a, b) { + return a.range[0] <= b.range[0] && a.range[1] >= b.range[1]; +} From 4704a98b555655aea9e28e910034d53609a381d2 Mon Sep 17 00:00:00 2001 From: Jamie Kyle Date: Wed, 31 Oct 2018 15:22:14 -0700 Subject: [PATCH 03/33] fix lint errors --- .../src/ReactiveDependencies.js | 45 ++++++++++++++----- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js index d04763f67a94..63493b356f4f 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js @@ -123,7 +123,9 @@ export default { */ function visitFunctionExpression(node) { // We only want to lint nodes which are reactive hook callbacks. - if (!isReactiveHookCallback(node, options)) return; + if (!isReactiveHookCallback(node, options)) { + return; + } // Get the reactive hook node. const reactiveHook = node.parent.callee; @@ -132,7 +134,9 @@ export default { // second argument then the reactive callback will re-run on every render. // So no need to check for dependency inclusion. const declaredDependenciesNode = node.parent.arguments[1]; - if (!declaredDependenciesNode) return; + if (!declaredDependenciesNode) { + return; + } // Get the current scope. const scope = context.getScope(); @@ -150,13 +154,17 @@ export default { let currentScope = scope.upper; while (currentScope) { pureScopes.add(currentScope); - if (currentScope.type === 'function') break; + if (currentScope.type === 'function') { + break; + } currentScope = currentScope.upper; } // If there is no parent function scope then there are no pure scopes. // The ones we've collected so far are incorrect. So don't continue with // the lint. - if (!currentScope) return; + if (!currentScope) { + return; + } } // Get dependencies from all our resolved references in pure scopes. @@ -164,8 +172,12 @@ export default { for (const reference of scope.references) { // If this reference is not resolved or it is not declared in a pure // scope then we don't care about this reference. - if (!reference.resolved) continue; - if (!pureScopes.has(reference.resolved.scope)) continue; + if (!reference.resolved) { + continue; + } + if (!pureScopes.has(reference.resolved.scope)) { + continue; + } // Narrow the scope of a dependency if it is, say, a member expression. // Then normalize the narrowed dependency. @@ -178,7 +190,9 @@ export default { // Add the dependency to a map so we can make sure it is referenced // again in our dependencies array. let nodes = dependencies.get(dependency); - if (!nodes) dependencies.set(dependency, (nodes = [])); + if (!nodes) { + dependencies.set(dependency, (nodes = [])); + } nodes.push(dependencyNode); } @@ -198,7 +212,9 @@ export default { } else { for (const declaredDependencyNode of declaredDependenciesNode.elements) { // Skip elided elements. - if (declaredDependencyNode === null) continue; + if (declaredDependencyNode === null) { + continue; + } // If we see a spread element then add a special warning. if (declaredDependencyNode.type === 'SpreadElement') { context.report( @@ -343,11 +359,18 @@ function fastFindReferenceWithParent(start, target) { while (queue.length) { item = queue.shift(); - if (isSameIdentifier(item, target)) return item; - if (!isAncestorNodeOf(item, target)) continue; + if (isSameIdentifier(item, target)) { + return item; + } + + if (!isAncestorNodeOf(item, target)) { + continue; + } for (let [key, value] of Object.entries(item)) { - if (key === 'parent') continue; + if (key === 'parent') { + continue; + } if (isNodeLike(value)) { value.parent = item; queue.push(value); From 6f67ef155f9cbfa1b6df270c91c32c8e891a787c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 10 Jan 2019 19:47:05 +0000 Subject: [PATCH 04/33] Support useLayoutEffect --- packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js index 63493b356f4f..a3662fca76b8 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js @@ -16,7 +16,6 @@ * - `useCallback()` * - `useMemo()` * - * TODO: handle all useEffect() variants. * TODO: implement autofix. * * Also supports `React` namespacing. e.g. `React.useEffect()`. @@ -36,6 +35,7 @@ function isReactiveHook(node, options) { } else if ( node.type === 'Identifier' && (node.name === 'useEffect' || + node.name === 'useLayoutEffect' || node.name === 'useCallback' || node.name === 'useMemo') ) { From da75f310a141b36f9ed9310a307f098aa6b5b2ad Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 10 Jan 2019 20:04:59 +0000 Subject: [PATCH 05/33] Add some failing tests and comments --- .../ESLintRuleReactiveDependencies-test.js | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js index e5113ae4778e..fb66b55fe99f 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js @@ -24,6 +24,7 @@ const eslintTester = new ESLintTester(); eslintTester.run('react-hooks', ReactHooksESLintRule, { valid: [ ` + // TODO: we don't care about hooks outside of components. const local = 42; useEffect(() => { console.log(local); @@ -54,6 +55,7 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { } `, ` + // TODO: we don't care about hooks outside of components. const local1 = 42; { const local2 = 42; @@ -124,6 +126,23 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { } `, ` + // Regression test + function MyComponent({ foo }) { + useEffect(() => { + console.log(foo.length); + }, [foo]); + } + `, + ` + // Regression test + function MyComponent({ history }) { + useEffect(() => { + return history.listen(); + }, [history]); + } + `, + ` + // TODO: we might want to forbid dot-access in deps. function MyComponent(props) { useEffect(() => { console.log(props.foo); @@ -131,6 +150,7 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { } `, ` + // TODO: we might want to forbid dot-access in deps. function MyComponent(props) { useEffect(() => { console.log(props.foo); @@ -139,6 +159,7 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { } `, ` + // TODO: we might want to forbid dot-access in deps. function MyComponent(props) { const local = 42; useEffect(() => { @@ -160,6 +181,7 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, { code: ` + // TODO: we might want to forbid dot-access in deps. function MyComponent(props) { useCustomEffect(() => { console.log(props.foo); @@ -181,6 +203,49 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { `, errors: [missingError('local')], }, + { + code: ` + // Regression test + function MyComponent() { + const local = 42; + useEffect(() => { + if (true) { + console.log(local); + } + }, []); + } + `, + errors: [missingError('local')], + }, + { + code: ` + // Regression test + function MyComponent() { + const local = 42; + useEffect(() => { + try { + console.log(local); + } finally {} + }, []); + } + `, + errors: [missingError('local')], + }, + { + code: ` + // Regression test + function MyComponent() { + const local = 42; + useEffect(() => { + function inner() { + console.log(local); + } + inner(); + }, []); + } + `, + errors: [missingError('local')], + }, { code: ` function MyComponent() { @@ -347,6 +412,7 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, { code: ` + // TODO: need to think more about this case. function MyComponent() { const local = {id: 42}; useEffect(() => { From 58183fec1a2d9eb4736f5d9652053a3e7808a487 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 11 Jan 2019 14:35:44 +0000 Subject: [PATCH 06/33] Gather dependencies in child scopes too --- .../src/ReactiveDependencies.js | 53 +++++++++++-------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js index a3662fca76b8..728c6fb501f0 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js @@ -169,31 +169,38 @@ export default { // Get dependencies from all our resolved references in pure scopes. const dependencies = new Map(); - for (const reference of scope.references) { - // If this reference is not resolved or it is not declared in a pure - // scope then we don't care about this reference. - if (!reference.resolved) { - continue; - } - if (!pureScopes.has(reference.resolved.scope)) { - continue; - } - // Narrow the scope of a dependency if it is, say, a member expression. - // Then normalize the narrowed dependency. + gatherDependenciesRecursively(scope); - const referenceNode = fastFindReferenceWithParent( - node, - reference.identifier, - ); - const dependencyNode = getDependency(referenceNode); - const dependency = normalizeDependencyNode(dependencyNode); - // Add the dependency to a map so we can make sure it is referenced - // again in our dependencies array. - let nodes = dependencies.get(dependency); - if (!nodes) { - dependencies.set(dependency, (nodes = [])); + function gatherDependenciesRecursively(currentScope) { + for (const reference of currentScope.references) { + // If this reference is not resolved or it is not declared in a pure + // scope then we don't care about this reference. + if (!reference.resolved) { + continue; + } + if (!pureScopes.has(reference.resolved.scope)) { + continue; + } + // Narrow the scope of a dependency if it is, say, a member expression. + // Then normalize the narrowed dependency. + + const referenceNode = fastFindReferenceWithParent( + node, + reference.identifier, + ); + const dependencyNode = getDependency(referenceNode); + const dependency = normalizeDependencyNode(dependencyNode); + // Add the dependency to a map so we can make sure it is referenced + // again in our dependencies array. + let nodes = dependencies.get(dependency); + if (!nodes) { + dependencies.set(dependency, (nodes = [])); + } + nodes.push(dependencyNode); + } + for (const childScope of currentScope.childScopes) { + gatherDependenciesRecursively(childScope); } - nodes.push(dependencyNode); } // Get all of the declared dependencies and put them in a set. We will From d1515f45046f7ed015e0a2f642aa6bef02e04364 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 11 Jan 2019 15:11:25 +0000 Subject: [PATCH 07/33] If we don't find foo.bar.baz in deps, try foo.bar, then foo --- .../src/ReactiveDependencies.js | 30 +++++++++++++++---- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js index 728c6fb501f0..b3c3a220874e 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js @@ -275,12 +275,30 @@ export default { // Loop through all our dependencies to make sure they have been declared. // If the dependency has not been declared we need to report some errors. for (const [dependency, dependencyNodes] of dependencies) { - if (declaredDependencies.has(dependency)) { - // Yay! Our dependency has been declared. Delete it from - // `declaredDependencies` since we will report all remaining unused - // declared dependencies. - declaredDependencies.delete(dependency); - } else { + let isDeclared = false; + + // If we can't find `foo.bar.baz`, search for `foo.bar`, then for `foo`. + let candidate = dependency; + while (candidate) { + if (declaredDependencies.has(candidate)) { + // Yay! Our dependency has been declared. Delete it from + // `declaredDependencies` since we will report all remaining unused + // declared dependencies. + isDeclared = true; + declaredDependencies.delete(candidate); + break; + } + const lastDotAccessIndex = candidate.lastIndexOf('.'); + if (lastDotAccessIndex === -1) { + break; + } + // If we didn't find `foo.bar.baz`, try `foo.bar`. Then try `foo`. + candidate = candidate.substring(0, lastDotAccessIndex); + // This is not super solid but works for simple cases we support. + // Alternatively we could stringify later and use nodes directly. + } + + if (!isDeclared) { // Oh no! Our dependency was not declared. So report an error for all // of the nodes which we expected to see an error from. for (const dependencyNode of dependencyNodes) { From 57ebe56095e91978653c59634a7d10b2506e77f5 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 11 Jan 2019 18:26:57 +0000 Subject: [PATCH 08/33] foo is enough for both foo.bar and foo.baz --- .../ESLintRuleReactiveDependencies-test.js | 35 +++++++++++++++++++ .../src/ReactiveDependencies.js | 19 ++++++---- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js index fb66b55fe99f..7ec8c90c17c6 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js @@ -133,6 +133,15 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [foo]); } `, + ` + // Regression test + function MyComponent({ foo }) { + useEffect(() => { + console.log(foo.length); + console.log(foo.slice(0)); + }, [foo]); + } + `, ` // Regression test function MyComponent({ history }) { @@ -169,6 +178,19 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [props.foo, props.bar, local]); } `, + { + code: ` + // TODO: we might want to warn "props.foo" + // is extraneous because we already have "props". + function MyComponent(props) { + const local = 42; + useEffect(() => { + console.log(props.foo); + console.log(props.bar); + }, [props, props.foo]); + } + `, + }, { code: ` function MyComponent(props) { @@ -492,6 +514,19 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { missingError('local'), ], }, + { + code: ` + function MyComponent(props) { + const local = 42; + useEffect(() => { + console.log(props.foo); + console.log(props.bar); + console.log(local); + }, [props]); + } + `, + errors: [missingError('local')], + }, { code: ` function MyComponent(props) { diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js index b3c3a220874e..6da40cce853c 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js @@ -272,6 +272,8 @@ export default { } } + let usedDependencies = new Set(); + // Loop through all our dependencies to make sure they have been declared. // If the dependency has not been declared we need to report some errors. for (const [dependency, dependencyNodes] of dependencies) { @@ -281,11 +283,10 @@ export default { let candidate = dependency; while (candidate) { if (declaredDependencies.has(candidate)) { - // Yay! Our dependency has been declared. Delete it from - // `declaredDependencies` since we will report all remaining unused - // declared dependencies. + // Yay! Our dependency has been declared. + // Record this so we don't report is unused. isDeclared = true; - declaredDependencies.delete(candidate); + usedDependencies.add(candidate); break; } const lastDotAccessIndex = candidate.lastIndexOf('.'); @@ -316,11 +317,15 @@ export default { // Loop through all the unused declared dependencies and report a warning // so the programmer removes the unused dependency from their list. - for (const declaredDependencyNode of declaredDependencies.values()) { + for (const [dependency, dependencyNode] of declaredDependencies) { + if (usedDependencies.has(dependency)) { + continue; + } + context.report( - declaredDependencyNode, + dependencyNode, `React Hook ${context.getSource(reactiveHook)} has an extra ` + - `dependency "${context.getSource(declaredDependencyNode)}" which ` + + `dependency "${context.getSource(dependencyNode)}" which ` + 'is not used in its callback. Removing this dependency may mean ' + 'the hook needs to execute fewer times.', ); From 369aa61162a5af8d7d6b9bcaa1b2969a40d2b4f0 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 17 Jan 2019 01:22:23 +0000 Subject: [PATCH 09/33] Shorter rule name --- fixtures/eslint/.eslintrc.json | 3 ++- ...iveDependencies-test.js => ESLintRuleReactiveDeps-test.js} | 3 +-- .../src/{ReactiveDependencies.js => ReactiveDeps.js} | 0 packages/eslint-plugin-react-hooks/src/index.js | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) rename packages/eslint-plugin-react-hooks/__tests__/{ESLintRuleReactiveDependencies-test.js => ESLintRuleReactiveDeps-test.js} (99%) rename packages/eslint-plugin-react-hooks/src/{ReactiveDependencies.js => ReactiveDeps.js} (100%) diff --git a/fixtures/eslint/.eslintrc.json b/fixtures/eslint/.eslintrc.json index 982d6c8eb65f..0c97574cfee8 100644 --- a/fixtures/eslint/.eslintrc.json +++ b/fixtures/eslint/.eslintrc.json @@ -9,6 +9,7 @@ }, "plugins": ["react-hooks"], "rules": { - "react-hooks/rules-of-hooks": 2 + "react-hooks/rules-of-hooks": 2, + "react-hooks/reactive-deps": 2 } } diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js similarity index 99% rename from packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js rename to packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index 7ec8c90c17c6..e374059b01d2 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -9,8 +9,7 @@ const ESLintTester = require('eslint').RuleTester; const ReactHooksESLintPlugin = require('eslint-plugin-react-hooks'); -const ReactHooksESLintRule = - ReactHooksESLintPlugin.rules['reactive-dependencies']; +const ReactHooksESLintRule = ReactHooksESLintPlugin.rules['reactive-deps']; ESLintTester.setDefaultConfig({ parser: 'babel-eslint', diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js similarity index 100% rename from packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js rename to packages/eslint-plugin-react-hooks/src/ReactiveDeps.js diff --git a/packages/eslint-plugin-react-hooks/src/index.js b/packages/eslint-plugin-react-hooks/src/index.js index e828c8bca70c..f7e5c6fa2eb6 100644 --- a/packages/eslint-plugin-react-hooks/src/index.js +++ b/packages/eslint-plugin-react-hooks/src/index.js @@ -8,9 +8,9 @@ 'use strict'; import RuleOfHooks from './RulesOfHooks'; -import ReactiveDependencies from './ReactiveDependencies'; +import ReactiveDeps from './ReactiveDeps'; export const rules = { 'rules-of-hooks': RuleOfHooks, - 'reactive-dependencies': ReactiveDependencies, + 'reactive-deps': ReactiveDeps, }; From 2df41d8b88baf03924cce972bf3af8b2f81e9454 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 18 Jan 2019 23:58:51 +0000 Subject: [PATCH 10/33] Add fixable meta --- .../src/ReactiveDeps.js | 65 ++++++++++--------- 1 file changed, 36 insertions(+), 29 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index 6da40cce853c..9450893671b3 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -91,6 +91,7 @@ function isReactiveHookCallback(node, options) { export default { meta: { + fixable: 'code', schema: [ { type: 'object', @@ -210,28 +211,30 @@ export default { // If the declared dependencies are not an array expression then we // can't verify that the user provided the correct dependencies. Tell // the user this in an error. - context.report( - declaredDependenciesNode, - `React Hook ${context.getSource(reactiveHook)} has a second ` + + context.report({ + node: declaredDependenciesNode, + message: + `React Hook ${context.getSource(reactiveHook)} has a second ` + "argument which is not an array literal. This means we can't " + "statically verify whether you've passed the correct dependencies.", - ); + }); } else { - for (const declaredDependencyNode of declaredDependenciesNode.elements) { + declaredDependenciesNode.elements.forEach(declaredDependencyNode => { // Skip elided elements. if (declaredDependencyNode === null) { - continue; + return; } // If we see a spread element then add a special warning. if (declaredDependencyNode.type === 'SpreadElement') { - context.report( - declaredDependencyNode, - `React Hook ${context.getSource(reactiveHook)} has a spread ` + + context.report({ + node: declaredDependencyNode, + message: + `React Hook ${context.getSource(reactiveHook)} has a spread ` + "element in its dependency list. This means we can't " + "statically verify whether you've passed the " + 'correct dependencies.', - ); - continue; + }); + return; } // Try to normalize the declared dependency. If we can't then an error // will be thrown. We will catch that error and report an error. @@ -242,13 +245,14 @@ export default { ); } catch (error) { if (/Unexpected node type/.test(error.message)) { - context.report( - declaredDependencyNode, - 'Unsupported expression in React Hook ' + + context.report({ + node: declaredDependencyNode, + message: + 'Unsupported expression in React Hook ' + `${context.getSource(reactiveHook)}'s dependency list. ` + 'Currently only simple variables are supported.', - ); - continue; + }); + return; } else { throw error; } @@ -256,12 +260,13 @@ export default { // If the programmer already declared this dependency then report a // duplicate dependency error. if (declaredDependencies.has(declaredDependency)) { - context.report( - declaredDependencyNode, - 'Duplicate value in React Hook ' + + context.report({ + node: declaredDependencyNode, + message: + 'Duplicate value in React Hook ' + `${context.getSource(reactiveHook)}'s dependency list for ` + `"${context.getSource(declaredDependencyNode)}".`, - ); + }); } else { // Add the dependency to our declared dependency map. declaredDependencies.set( @@ -269,7 +274,7 @@ export default { declaredDependencyNode, ); } - } + }); } let usedDependencies = new Set(); @@ -303,14 +308,15 @@ export default { // Oh no! Our dependency was not declared. So report an error for all // of the nodes which we expected to see an error from. for (const dependencyNode of dependencyNodes) { - context.report( - dependencyNode, - `React Hook ${context.getSource(reactiveHook)} references ` + + context.report({ + node: dependencyNode, + message: + `React Hook ${context.getSource(reactiveHook)} references ` + `"${context.getSource(dependencyNode)}", but it was not ` + 'listed in the hook dependencies argument. This means if ' + `"${context.getSource(dependencyNode)}" changes then ` + `${context.getSource(reactiveHook)} won't be able to update.`, - ); + }); } } } @@ -322,13 +328,14 @@ export default { continue; } - context.report( - dependencyNode, - `React Hook ${context.getSource(reactiveHook)} has an extra ` + + context.report({ + node: dependencyNode, + message: + `React Hook ${context.getSource(reactiveHook)} has an extra ` + `dependency "${context.getSource(dependencyNode)}" which ` + 'is not used in its callback. Removing this dependency may mean ' + 'the hook needs to execute fewer times.', - ); + }); } } }, From 3b3d7dacd3be218b5d10d31e38aed3ea8a2d62d9 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 19 Jan 2019 00:10:50 +0000 Subject: [PATCH 11/33] Remove a bunch of code and start from scratch --- .../src/ReactiveDeps.js | 83 ++----------------- 1 file changed, 6 insertions(+), 77 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index 9450893671b3..ebd57ffd9b4a 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -105,6 +105,7 @@ export default { ], }, create(context) { + const sourceCode = context.getSourceCode(); // Parse the `additionalHooks` regex. const additionalHooks = context.options && @@ -257,86 +258,14 @@ export default { throw error; } } - // If the programmer already declared this dependency then report a - // duplicate dependency error. - if (declaredDependencies.has(declaredDependency)) { - context.report({ - node: declaredDependencyNode, - message: - 'Duplicate value in React Hook ' + - `${context.getSource(reactiveHook)}'s dependency list for ` + - `"${context.getSource(declaredDependencyNode)}".`, - }); - } else { - // Add the dependency to our declared dependency map. - declaredDependencies.set( - declaredDependency, - declaredDependencyNode, - ); - } + // Add the dependency to our declared dependency map. + declaredDependencies.set( + declaredDependency, + declaredDependencyNode, + ); }); } - let usedDependencies = new Set(); - - // Loop through all our dependencies to make sure they have been declared. - // If the dependency has not been declared we need to report some errors. - for (const [dependency, dependencyNodes] of dependencies) { - let isDeclared = false; - - // If we can't find `foo.bar.baz`, search for `foo.bar`, then for `foo`. - let candidate = dependency; - while (candidate) { - if (declaredDependencies.has(candidate)) { - // Yay! Our dependency has been declared. - // Record this so we don't report is unused. - isDeclared = true; - usedDependencies.add(candidate); - break; - } - const lastDotAccessIndex = candidate.lastIndexOf('.'); - if (lastDotAccessIndex === -1) { - break; - } - // If we didn't find `foo.bar.baz`, try `foo.bar`. Then try `foo`. - candidate = candidate.substring(0, lastDotAccessIndex); - // This is not super solid but works for simple cases we support. - // Alternatively we could stringify later and use nodes directly. - } - - if (!isDeclared) { - // Oh no! Our dependency was not declared. So report an error for all - // of the nodes which we expected to see an error from. - for (const dependencyNode of dependencyNodes) { - context.report({ - node: dependencyNode, - message: - `React Hook ${context.getSource(reactiveHook)} references ` + - `"${context.getSource(dependencyNode)}", but it was not ` + - 'listed in the hook dependencies argument. This means if ' + - `"${context.getSource(dependencyNode)}" changes then ` + - `${context.getSource(reactiveHook)} won't be able to update.`, - }); - } - } - } - - // Loop through all the unused declared dependencies and report a warning - // so the programmer removes the unused dependency from their list. - for (const [dependency, dependencyNode] of declaredDependencies) { - if (usedDependencies.has(dependency)) { - continue; - } - - context.report({ - node: dependencyNode, - message: - `React Hook ${context.getSource(reactiveHook)} has an extra ` + - `dependency "${context.getSource(dependencyNode)}" which ` + - 'is not used in its callback. Removing this dependency may mean ' + - 'the hook needs to execute fewer times.', - }); - } } }, }; From d4d501bce6ffd9da9f081f8d8226a1d10a7ffd48 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 19 Jan 2019 01:26:22 +0000 Subject: [PATCH 12/33] [WIP] Only report errors from dependency array This results in nicer editing experience. Also has autofix. --- fixtures/eslint/index.js | 31 ++++++-- .../src/ReactiveDeps.js | 78 +++++++++++++++++-- 2 files changed, 97 insertions(+), 12 deletions(-) diff --git a/fixtures/eslint/index.js b/fixtures/eslint/index.js index 6fefccd55e67..954e9380bbab 100644 --- a/fixtures/eslint/index.js +++ b/fixtures/eslint/index.js @@ -4,8 +4,29 @@ // 2. "File > Add Folder to Workspace" this specific folder in VSCode with ESLint extension // 3. Changes to the rule source should get picked up without restarting ESLint server -function Foo() { - if (condition) { - useEffect(() => {}); - } -} + + +function Comment({ + comment, + commentSource, +}) { + const currentUserID = comment.viewer.id; + const environment = RelayEnvironment.forUser(currentUserID); + const commentID = nullthrows(comment.id); + useEffect(() => { + const subscription = SubscriptionCounter.subscribeOnce( + `StoreSubscription_${commentID}`, + () => + StoreSubscription.subscribe( + environment, + { + comment_id: commentID, + }, + currentUserID, + commentSource + ) + ); + return () => subscription.dispose(); + }, [commentID, environment, currentUserID, commentSource]); +} + \ No newline at end of file diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index ebd57ffd9b4a..a808b6c4d811 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -115,6 +115,8 @@ export default { : undefined; const options = {additionalHooks}; + console.log('\n\n\n\n\n\n\n\n\n\n\n\n\n'); + return { FunctionExpression: visitFunctionExpression, ArrowFunctionExpression: visitFunctionExpression, @@ -205,9 +207,7 @@ export default { } } - // Get all of the declared dependencies and put them in a set. We will - // compare this to the dependencies we found in the callback later. - const declaredDependencies = new Map(); + const declaredDependencies = []; if (declaredDependenciesNode.type !== 'ArrayExpression') { // If the declared dependencies are not an array expression then we // can't verify that the user provided the correct dependencies. Tell @@ -259,13 +259,77 @@ export default { } } // Add the dependency to our declared dependency map. - declaredDependencies.set( - declaredDependency, - declaredDependencyNode, - ); + declaredDependencies.push({ + key: declaredDependency, + node: declaredDependencyNode, + }); }); } + let suggestedDependencies = []; + + let duplicateDependencies = new Set(); + let unnecessaryDependencies = new Set(); + let missingDependencies = new Set(); + + // First, ensure what user specified makes sense. + for (let {node, key} of declaredDependencies) { + if (dependencies.has(key)) { + // Legit dependency. + if (suggestedDependencies.indexOf(key) === -1) { + suggestedDependencies.push(key); + } else { + // Duplicate. Do nothing. + duplicateDependencies.add(key); + } + } else { + // Unnecessary dependency. Do nothing. + unnecessaryDependencies.add(key); + } + } + // Then fill in the missing ones. + for (let [key, usageNode] of dependencies) { + if (suggestedDependencies.indexOf(key) === -1) { + // Legit missing. + suggestedDependencies.push(key); + missingDependencies.add(key); + } else { + // Already did that. Do nothing. + } + } + + if ( + duplicateDependencies.size > 0 || + missingDependencies.size > 0 || + unnecessaryDependencies.size > 0 + ) { + context.report({ + node: declaredDependenciesNode, + message: + `React Hook ${context.getSource(reactiveHook)} has ` + + [ + (missingDependencies.size > 0 ? + `missing [${Array.from(missingDependencies).join(', ')}]` + : null + ), + (duplicateDependencies.size > 0 ? + `duplicate [${Array.from(duplicateDependencies).join(', ')}]` + : null + ), + (unnecessaryDependencies.size > 0 ? + `duplicate [${Array.from(unnecessaryDependencies).join(', ')}]` : + null + ), + ].filter(Boolean).join(', ') + + ` dependencies. Either fix or remove the dependency array.`, + fix(fixer) { + return fixer.replaceText( + declaredDependenciesNode, + `[${suggestedDependencies.join(', ')}]` + ); + } + }); + } } }, }; From 695693caed75aa650530a18589b84f01db41e54e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 19 Jan 2019 20:12:06 +0000 Subject: [PATCH 13/33] Fix typo --- packages/eslint-plugin-react-hooks/src/ReactiveDeps.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index a808b6c4d811..7bb81ab68f25 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -317,7 +317,7 @@ export default { : null ), (unnecessaryDependencies.size > 0 ? - `duplicate [${Array.from(unnecessaryDependencies).join(', ')}]` : + `unnecessary [${Array.from(unnecessaryDependencies).join(', ')}]` : null ), ].filter(Boolean).join(', ') + From 340bffb1350311702bb774658c05eb2caa903615 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 13 Feb 2019 18:56:21 +0000 Subject: [PATCH 14/33] [Temp] Skip all tests --- .../__tests__/ESLintRuleReactiveDeps-test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index e374059b01d2..77ced25939d0 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -211,7 +211,7 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { `, options: [{additionalHooks: 'useCustomEffect'}], }, - ], + ].filter(x => x.code && x.code.indexOf('/*only*/') > -1), invalid: [ { code: ` @@ -615,7 +615,7 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { 'Currently only simple variables are supported.', ], }, - ], + ].filter(x => x.code && x.code.indexOf('/*only*/') > -1), }); function missingError(dependency, hook = 'useEffect') { From 3832eb319aec1cf4fd8c79b3af26d80b7e13ef56 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 13 Feb 2019 18:58:31 +0000 Subject: [PATCH 15/33] Fix the first test --- .../__tests__/ESLintRuleReactiveDeps-test.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index 77ced25939d0..ead781affac7 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -215,6 +215,7 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { invalid: [ { code: ` + /*only*/ function MyComponent() { const local = 42; useEffect(() => { @@ -222,7 +223,18 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, []); } `, - errors: [missingError('local')], + output: ` + /*only*/ + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + }, [local]); + } + `, + errors: [ + 'React Hook useEffect has missing [local] dependencies. Either fix or remove the dependency array.' + ], }, { code: ` From 244a1ff08a5d43031e785113e0f674400c24f917 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 14 Feb 2019 21:55:28 +0000 Subject: [PATCH 16/33] Revamp the test suite --- fixtures/eslint/index.js | 49 +- .../__tests__/ESLintRuleReactiveDeps-test.js | 591 +++++++++++++++--- .../src/ReactiveDeps.js | 38 +- 3 files changed, 536 insertions(+), 142 deletions(-) diff --git a/fixtures/eslint/index.js b/fixtures/eslint/index.js index 954e9380bbab..8b7db9ff07fb 100644 --- a/fixtures/eslint/index.js +++ b/fixtures/eslint/index.js @@ -4,29 +4,26 @@ // 2. "File > Add Folder to Workspace" this specific folder in VSCode with ESLint extension // 3. Changes to the rule source should get picked up without restarting ESLint server - - -function Comment({ - comment, - commentSource, -}) { - const currentUserID = comment.viewer.id; - const environment = RelayEnvironment.forUser(currentUserID); - const commentID = nullthrows(comment.id); - useEffect(() => { - const subscription = SubscriptionCounter.subscribeOnce( - `StoreSubscription_${commentID}`, - () => - StoreSubscription.subscribe( - environment, - { - comment_id: commentID, - }, - currentUserID, - commentSource - ) - ); - return () => subscription.dispose(); - }, [commentID, environment, currentUserID, commentSource]); -} - \ No newline at end of file +function Comment({comment, commentSource}) { + const currentUserID = comment.viewer.id; + const environment = RelayEnvironment.forUser(currentUserID); + const commentID = nullthrows(comment.id); + useEffect( + () => { + const subscription = SubscriptionCounter.subscribeOnce( + `StoreSubscription_${commentID}`, + () => + StoreSubscription.subscribe( + environment, + { + comment_id: commentID, + }, + currentUserID, + commentSource + ) + ); + return () => subscription.dispose(); + }, + [commentID, environment, currentUserID, commentSource] + ); +} diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index ead781affac7..3bfcf860c6e1 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -19,17 +19,20 @@ 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: [ - ` - // TODO: we don't care about hooks outside of components. - const local = 42; - useEffect(() => { - console.log(local); - }, []); - `, - ` + { + code: ` function MyComponent() { const local = 42; useEffect(() => { @@ -37,7 +40,9 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }); } `, - ` + }, + { + code: ` function MyComponent() { useEffect(() => { const local = 42; @@ -45,7 +50,9 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, []); } `, - ` + }, + { + code: ` function MyComponent() { const local = 42; useEffect(() => { @@ -53,18 +60,9 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [local]); } `, - ` - // TODO: we don't care about hooks outside of components. - const local1 = 42; - { - const local2 = 42; - useEffect(() => { - console.log(local1); - console.log(local2); - }, []); - } - `, - ` + }, + { + code: ` function MyComponent() { const local1 = 42; { @@ -76,7 +74,9 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { } } `, - ` + }, + { + code: ` function MyComponent() { const local1 = 42; { @@ -88,7 +88,9 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { } } `, - ` + }, + { + code: ` function MyComponent() { const local1 = 42; function MyNestedComponent() { @@ -100,7 +102,9 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { } } `, - ` + }, + { + code: ` function MyComponent() { const local = 42; useEffect(() => { @@ -109,14 +113,18 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [local]); } `, - ` + }, + { + code: ` function MyComponent() { useEffect(() => { console.log(unresolved); }, []); } `, - ` + }, + { + code: ` function MyComponent() { const local = 42; useEffect(() => { @@ -124,7 +132,10 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [,,,local,,,]); } `, - ` + }, + { + skip: true, // TODO + code: ` // Regression test function MyComponent({ foo }) { useEffect(() => { @@ -132,7 +143,10 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [foo]); } `, - ` + }, + { + skip: true, // TODO + code: ` // Regression test function MyComponent({ foo }) { useEffect(() => { @@ -141,7 +155,10 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [foo]); } `, - ` + }, + { + skip: true, // TODO + code: ` // Regression test function MyComponent({ history }) { useEffect(() => { @@ -149,16 +166,20 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [history]); } `, - ` + }, + { // TODO: we might want to forbid dot-access in deps. + code: ` function MyComponent(props) { useEffect(() => { console.log(props.foo); }, [props.foo]); } `, - ` + }, + { // TODO: we might want to forbid dot-access in deps. + code: ` function MyComponent(props) { useEffect(() => { console.log(props.foo); @@ -166,8 +187,10 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [props.foo, props.bar]); } `, - ` + }, + { // TODO: we might want to forbid dot-access in deps. + code: ` function MyComponent(props) { const local = 42; useEffect(() => { @@ -177,10 +200,12 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [props.foo, props.bar, local]); } `, + }, { + skip: true, // TODO + // TODO: we might want to warn "props.foo" + // is extraneous because we already have "props". code: ` - // TODO: we might want to warn "props.foo" - // is extraneous because we already have "props". function MyComponent(props) { const local = 42; useEffect(() => { @@ -201,8 +226,8 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { options: [{additionalHooks: 'useCustomEffect'}], }, { + // TODO: we might want to forbid dot-access in deps. code: ` - // TODO: we might want to forbid dot-access in deps. function MyComponent(props) { useCustomEffect(() => { console.log(props.foo); @@ -211,11 +236,32 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { `, options: [{additionalHooks: 'useCustomEffect'}], }, - ].filter(x => x.code && x.code.indexOf('/*only*/') > -1), + { + code: ` + // Valid because we don't care about hooks outside of components. + const local = 42; + useEffect(() => { + console.log(local); + }, []); + `, + }, + { + code: ` + // Valid because we don't care about hooks outside of components. + const local1 = 42; + { + const local2 = 42; + useEffect(() => { + console.log(local1); + console.log(local2); + }, []); + } + `, + }, + ], invalid: [ { code: ` - /*only*/ function MyComponent() { const local = 42; useEffect(() => { @@ -224,7 +270,6 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { } `, output: ` - /*only*/ function MyComponent() { const local = 42; useEffect(() => { @@ -233,7 +278,8 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { } `, errors: [ - 'React Hook useEffect has missing [local] dependencies. Either fix or remove the dependency array.' + 'React Hook useEffect has missing [local] dependencies. ' + + 'Either fix or remove the dependency array.', ], }, { @@ -248,7 +294,21 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, []); } `, - errors: [missingError('local')], + output: ` + // Regression test + function MyComponent() { + const local = 42; + useEffect(() => { + if (true) { + console.log(local); + } + }, [local]); + } + `, + errors: [ + 'React Hook useEffect has missing [local] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -262,7 +322,21 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, []); } `, - errors: [missingError('local')], + output: ` + // Regression test + function MyComponent() { + const local = 42; + useEffect(() => { + try { + console.log(local); + } finally {} + }, [local]); + } + `, + errors: [ + 'React Hook useEffect has missing [local] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -277,7 +351,22 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, []); } `, - errors: [missingError('local')], + output: ` + // Regression test + function MyComponent() { + const local = 42; + useEffect(() => { + function inner() { + console.log(local); + } + inner(); + }, [local]); + } + `, + errors: [ + 'React Hook useEffect has missing [local] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -292,7 +381,22 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { } } `, - errors: [missingError('local1'), missingError('local2')], + output: ` + function MyComponent() { + const local1 = 42; + { + const local2 = 42; + useEffect(() => { + console.log(local1); + console.log(local2); + }, [local1, local2]); + } + } + `, + errors: [ + 'React Hook useEffect has missing [local1, local2] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -305,7 +409,20 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [local1]); } `, - errors: [missingError('local2')], + output: ` + function MyComponent() { + const local1 = 42; + const local2 = 42; + useEffect(() => { + console.log(local1); + console.log(local2); + }, [local1, local2]); + } + `, + errors: [ + 'React Hook useEffect has missing [local2] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -317,9 +434,23 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [local1, local2]); } `, - errors: [extraError('local2')], + output: ` + function MyComponent() { + const local1 = 42; + const local2 = 42; + useEffect(() => { + console.log(local1); + }, [local1]); + } + `, + errors: [ + 'React Hook useEffect has unnecessary [local2] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { + // TODO: this case is weird. + // Maybe it should not consider local1 unused despite component nesting? code: ` function MyComponent() { const local1 = 42; @@ -332,7 +463,22 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { } } `, - errors: [missingError('local2'), extraError('local1')], + output: ` + function MyComponent() { + const local1 = 42; + function MyNestedComponent() { + const local2 = 42; + useEffect(() => { + console.log(local1); + console.log(local2); + }, [local2]); + } + } + `, + errors: [ + 'React Hook useEffect has missing [local2], unnecessary [local1] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -344,7 +490,19 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, []); } `, - errors: [missingError('local'), missingError('local')], + output: ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + console.log(local); + }, [local]); + } + `, + errors: [ + 'React Hook useEffect has missing [local] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -356,7 +514,19 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [local, local]); } `, - errors: [duplicateError('local')], + output: ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + console.log(local); + }, [local]); + } + `, + errors: [ + 'React Hook useEffect has duplicate [local] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -364,7 +534,15 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { useEffect(() => {}, [local]); } `, - errors: [extraError('local')], + output: ` + function MyComponent() { + useEffect(() => {}, []); + } + `, + errors: [ + 'React Hook useEffect has unnecessary [local] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -373,6 +551,12 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { useEffect(() => {}, dependencies); } `, + output: ` + function MyComponent() { + const dependencies = []; + useEffect(() => {}, dependencies); + } + `, errors: [ 'React Hook useEffect has a second argument which is not an array ' + "literal. This means we can't statically verify whether you've " + @@ -389,11 +573,22 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, dependencies); } `, + // TODO: should this autofix or bail out? + output: ` + function MyComponent() { + const local = 42; + const dependencies = [local]; + useEffect(() => { + console.log(local); + }, [local]); + } + `, errors: [ - missingError('local'), 'React Hook useEffect has a second argument which is not an array ' + "literal. This means we can't statically verify whether you've " + 'passed the correct dependencies.', + 'React Hook useEffect has missing [local] dependencies. ' + + 'Either fix or remove the dependency array.', ], }, { @@ -406,8 +601,19 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [...dependencies]); } `, + // TODO: should this autofix or bail out? + output: ` + function MyComponent() { + const local = 42; + const dependencies = [local]; + useEffect(() => { + console.log(local); + }, [local]); + } + `, errors: [ - missingError('local'), + 'React Hook useEffect has missing [local] dependencies. ' + + 'Either fix or remove the dependency array.', 'React Hook useEffect has a spread element in its dependency list. ' + "This means we can't statically verify whether you've passed the " + 'correct dependencies.', @@ -422,6 +628,14 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [local, ...dependencies]); } `, + output: ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + }, [local, ...dependencies]); + } + `, errors: [ 'React Hook useEffect has a spread element in its dependency list. ' + "This means we can't statically verify whether you've passed the " + @@ -437,15 +651,26 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [computeCacheKey(local)]); } `, + // TODO: I'm not sure this is a good idea. + // Maybe bail out? + output: ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + }, [local]); + } + `, errors: [ - missingError('local'), + 'React Hook useEffect has missing [local] dependencies. ' + + 'Either fix or remove the dependency array.', "Unsupported expression in React Hook useEffect's dependency list. " + 'Currently only simple variables are supported.', ], }, { + // TODO: need to think more about this case. code: ` - // TODO: need to think more about this case. function MyComponent() { const local = {id: 42}; useEffect(() => { @@ -453,7 +678,19 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [local.id]); } `, - errors: [missingError('local'), extraError('local.id')], + // TODO: this may not be a good idea. + output: ` + function MyComponent() { + const local = {id: 42}; + useEffect(() => { + console.log(local); + }, [local]); + } + `, + errors: [ + 'React Hook useEffect has missing [local], unnecessary [local.id] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -464,7 +701,18 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [local, local]); } `, - errors: [duplicateError('local')], + output: ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + }, [local]); + } + `, + errors: [ + 'React Hook useEffect has duplicate [local] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -476,7 +724,19 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [local1]); } `, - errors: [extraError('local1')], + output: ` + function MyComponent() { + const local1 = 42; + useEffect(() => { + const local1 = 42; + console.log(local1); + }, []); + } + `, + errors: [ + 'React Hook useEffect has unnecessary [local1] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -485,7 +745,16 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { useEffect(() => {}, [local1]); } `, - errors: [extraError('local1')], + output: ` + function MyComponent() { + const local1 = 42; + useEffect(() => {}, []); + } + `, + errors: [ + 'React Hook useEffect has unnecessary [local1] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -495,7 +764,19 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, []); } `, - errors: [missingError('props.foo')], + // TODO: we need to think about ideal output here. + // Should it capture by default? + output: ` + function MyComponent(props) { + useEffect(() => { + console.log(props.foo); + }, [props.foo]); + } + `, + errors: [ + 'React Hook useEffect has missing [props.foo] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -506,7 +787,20 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, []); } `, - errors: [missingError('props.foo'), missingError('props.bar')], + // TODO: we need to think about ideal output here. + // Should it capture by default? + output: ` + function MyComponent(props) { + useEffect(() => { + console.log(props.foo); + console.log(props.bar); + }, [props.foo, props.bar]); + } + `, + errors: [ + 'React Hook useEffect has missing [props.foo, props.bar] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -519,13 +813,25 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, []); } `, + // TODO: we need to think about ideal output here. + // Should it capture by default? + output: ` + function MyComponent(props) { + const local = 42; + useEffect(() => { + console.log(props.foo); + console.log(props.bar); + console.log(local); + }, [props.foo, props.bar, local]); + } + `, errors: [ - missingError('props.foo'), - missingError('props.bar'), - missingError('local'), + 'React Hook useEffect has missing [props.foo, props.bar, local] dependencies. ' + + 'Either fix or remove the dependency array.', ], }, { + skip: true, // TODO code: ` function MyComponent(props) { const local = 42; @@ -536,7 +842,20 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [props]); } `, - errors: [missingError('local')], + output: ` + function MyComponent(props) { + const local = 42; + useEffect(() => { + console.log(props.foo); + console.log(props.bar); + console.log(local); + }, [props, local]); + } + `, + errors: [ + 'React Hook useEffect has missing [local] dependencies. ' + + 'Either fix or remove the dependency array.', + ], }, { code: ` @@ -564,13 +883,44 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, []); } `, + output: ` + function MyComponent(props) { + useEffect(() => { + console.log(props.foo); + }, [props.foo]); + useCallback(() => { + console.log(props.foo); + }, [props.foo]); + useMemo(() => { + console.log(props.foo); + }, [props.foo]); + React.useEffect(() => { + console.log(props.foo); + }, [props.foo]); + React.useCallback(() => { + console.log(props.foo); + }, [props.foo]); + React.useMemo(() => { + console.log(props.foo); + }, [props.foo]); + React.notReactiveHook(() => { + console.log(props.foo); + }, []); + } + `, errors: [ - missingError('props.foo', 'useEffect'), - missingError('props.foo', 'useCallback'), - missingError('props.foo', 'useMemo'), - missingError('props.foo', 'React.useEffect'), - missingError('props.foo', 'React.useCallback'), - missingError('props.foo', 'React.useMemo'), + 'React Hook useEffect has missing [props.foo] dependencies. ' + + 'Either fix or remove the dependency array.', + 'React Hook useCallback has missing [props.foo] dependencies. ' + + 'Either fix or remove the dependency array.', + 'React Hook useMemo has missing [props.foo] dependencies. ' + + 'Either fix or remove the dependency array.', + 'React Hook React.useEffect has missing [props.foo] dependencies. ' + + 'Either fix or remove the dependency array.', + 'React Hook React.useCallback has missing [props.foo] dependencies. ' + + 'Either fix or remove the dependency array.', + 'React Hook React.useMemo has missing [props.foo] dependencies. ' + + 'Either fix or remove the dependency array.', ], }, { @@ -590,11 +940,30 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, []); } `, + output: ` + function MyComponent(props) { + useCustomEffect(() => { + console.log(props.foo); + }, [props.foo]); + useEffect(() => { + console.log(props.foo); + }, [props.foo]); + React.useEffect(() => { + console.log(props.foo); + }, [props.foo]); + React.useCustomEffect(() => { + console.log(props.foo); + }, []); + } + `, options: [{additionalHooks: 'useCustomEffect'}], errors: [ - missingError('props.foo', 'useCustomEffect'), - missingError('props.foo', 'useEffect'), - missingError('props.foo', 'React.useEffect'), + 'React Hook useCustomEffect has missing [props.foo] dependencies. ' + + 'Either fix or remove the dependency array.', + 'React Hook useEffect has missing [props.foo] dependencies. ' + + 'Either fix or remove the dependency array.', + 'React Hook React.useEffect has missing [props.foo] dependencies. ' + + 'Either fix or remove the dependency array.', ], }, { @@ -606,8 +975,18 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [a ? local : b]); } `, + // TODO: should we bail out instead? + output: ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + }, [local]); + } + `, errors: [ - missingError('local'), + 'React Hook useEffect has missing [local] dependencies. ' + + 'Either fix or remove the dependency array.', "Unsupported expression in React Hook useEffect's dependency list. " + 'Currently only simple variables are supported.', ], @@ -621,31 +1000,51 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [a && local]); } `, + // TODO: should we bail out instead? + output: ` + function MyComponent() { + const local = 42; + useEffect(() => { + console.log(local); + }, [local]); + } + `, errors: [ - missingError('local'), + 'React Hook useEffect has missing [local] dependencies. ' + + 'Either fix or remove the dependency array.', "Unsupported expression in React Hook useEffect's dependency list. " + 'Currently only simple variables are supported.', ], }, - ].filter(x => x.code && x.code.indexOf('/*only*/') > -1), -}); - -function missingError(dependency, hook = 'useEffect') { - return ( - `React Hook ${hook} references "${dependency}", but it was not listed in ` + - `the hook dependencies argument. This means if "${dependency}" changes ` + - `then ${hook} won't be able to update.` - ); -} + ], +}; -function extraError(dependency) { - return ( - `React Hook useEffect has an extra dependency "${dependency}" which is ` + - `not used in its callback. Removing this dependency may mean the hook ` + - `needs to execute fewer times.` - ); +// 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); } -function duplicateError(dependency) { - return `Duplicate value in React Hook useEffect's dependency list for "${dependency}".`; -} +const eslintTester = new ESLintTester(); +eslintTester.run('react-hooks', ReactHooksESLintRule, tests); diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index 7bb81ab68f25..a58bf572a3c4 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -105,7 +105,6 @@ export default { ], }, create(context) { - const sourceCode = context.getSourceCode(); // Parse the `additionalHooks` regex. const additionalHooks = context.options && @@ -115,8 +114,6 @@ export default { : undefined; const options = {additionalHooks}; - console.log('\n\n\n\n\n\n\n\n\n\n\n\n\n'); - return { FunctionExpression: visitFunctionExpression, ArrowFunctionExpression: visitFunctionExpression, @@ -273,7 +270,7 @@ export default { let missingDependencies = new Set(); // First, ensure what user specified makes sense. - for (let {node, key} of declaredDependencies) { + for (let {key} of declaredDependencies) { if (dependencies.has(key)) { // Legit dependency. if (suggestedDependencies.indexOf(key) === -1) { @@ -288,7 +285,7 @@ export default { } } // Then fill in the missing ones. - for (let [key, usageNode] of dependencies) { + for (let [key] of dependencies) { if (suggestedDependencies.indexOf(key) === -1) { // Legit missing. suggestedDependencies.push(key); @@ -308,26 +305,27 @@ export default { message: `React Hook ${context.getSource(reactiveHook)} has ` + [ - (missingDependencies.size > 0 ? - `missing [${Array.from(missingDependencies).join(', ')}]` - : null - ), - (duplicateDependencies.size > 0 ? - `duplicate [${Array.from(duplicateDependencies).join(', ')}]` - : null - ), - (unnecessaryDependencies.size > 0 ? - `unnecessary [${Array.from(unnecessaryDependencies).join(', ')}]` : - null - ), - ].filter(Boolean).join(', ') + + missingDependencies.size > 0 + ? `missing [${Array.from(missingDependencies).join(', ')}]` + : null, + duplicateDependencies.size > 0 + ? `duplicate [${Array.from(duplicateDependencies).join(', ')}]` + : null, + unnecessaryDependencies.size > 0 + ? `unnecessary [${Array.from(unnecessaryDependencies).join( + ', ', + )}]` + : null, + ] + .filter(Boolean) + .join(', ') + ` dependencies. Either fix or remove the dependency array.`, fix(fixer) { return fixer.replaceText( declaredDependenciesNode, - `[${suggestedDependencies.join(', ')}]` + `[${suggestedDependencies.join(', ')}]`, ); - } + }, }); } } From 58e38ca1e2a48bb322a421ecc9617af9ce8421b7 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 15 Feb 2019 16:18:49 +0000 Subject: [PATCH 17/33] Fix [foo] to include foo.bar --- .../__tests__/ESLintRuleReactiveDeps-test.js | 5 ---- .../src/ReactiveDeps.js | 23 ++++++++++++++----- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index 3bfcf860c6e1..72b016507e26 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -134,7 +134,6 @@ const tests = { `, }, { - skip: true, // TODO code: ` // Regression test function MyComponent({ foo }) { @@ -145,7 +144,6 @@ const tests = { `, }, { - skip: true, // TODO code: ` // Regression test function MyComponent({ foo }) { @@ -157,7 +155,6 @@ const tests = { `, }, { - skip: true, // TODO code: ` // Regression test function MyComponent({ history }) { @@ -202,7 +199,6 @@ const tests = { `, }, { - skip: true, // TODO // TODO: we might want to warn "props.foo" // is extraneous because we already have "props". code: ` @@ -831,7 +827,6 @@ const tests = { ], }, { - skip: true, // TODO code: ` function MyComponent(props) { const local = 42; diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index a58bf572a3c4..6b581a34a72c 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -269,9 +269,16 @@ export default { let unnecessaryDependencies = new Set(); let missingDependencies = new Set(); + let actualDependencies = Array.from(dependencies.keys()); + + function satisfies(actualDep, dep) { + return actualDep === dep || actualDep.startsWith(dep + '.'); + } + + // TODO: this could use some refactoring and optimizations. // First, ensure what user specified makes sense. - for (let {key} of declaredDependencies) { - if (dependencies.has(key)) { + declaredDependencies.forEach(({key}) => { + if (actualDependencies.some(actualDep => satisfies(actualDep, key))) { // Legit dependency. if (suggestedDependencies.indexOf(key) === -1) { suggestedDependencies.push(key); @@ -283,17 +290,21 @@ export default { // Unnecessary dependency. Do nothing. unnecessaryDependencies.add(key); } - } + }); // Then fill in the missing ones. - for (let [key] of dependencies) { - if (suggestedDependencies.indexOf(key) === -1) { + Array.from(dependencies.keys()).forEach(key => { + if ( + !suggestedDependencies.some(suggestedDep => + satisfies(key, suggestedDep), + ) + ) { // Legit missing. suggestedDependencies.push(key); missingDependencies.add(key); } else { // Already did that. Do nothing. } - } + }); if ( duplicateDependencies.size > 0 || From 0aea58e01417e5a9422e01e6f4ec0e1e63125504 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 15 Feb 2019 18:06:15 +0000 Subject: [PATCH 18/33] Don't suggest call expressions --- .../__tests__/ESLintRuleReactiveDeps-test.js | 46 +++++++++++++++++++ .../src/ReactiveDeps.js | 7 ++- 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index 72b016507e26..1d18c1bc526f 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -540,6 +540,52 @@ const tests = { 'Either fix or remove the dependency array.', ], }, + { + code: ` + function MyComponent({ history }) { + useEffect(() => { + return history.listen(); + }, []); + } + `, + output: ` + function MyComponent({ history }) { + useEffect(() => { + return history.listen(); + }, [history]); + } + `, + errors: [ + 'React Hook useEffect has missing [history] dependencies. ' + + 'Either fix or remove the dependency array.', + ], + }, + { + code: ` + function MyComponent({ history }) { + useEffect(() => { + return [ + history.foo.bar[2].dobedo.listen(), + history.foo.bar().dobedo.listen[2] + ]; + }, []); + } + `, + output: ` + function MyComponent({ history }) { + useEffect(() => { + return [ + history.foo.bar[2].dobedo.listen(), + history.foo.bar().dobedo.listen[2] + ]; + }, [history.foo]); + } + `, + errors: [ + 'React Hook useEffect has missing [history.foo] dependencies. ' + + 'Either fix or remove the dependency array.', + ], + }, { code: ` function MyComponent() { diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index 6b581a34a72c..10a25cf25f7a 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -355,7 +355,12 @@ function getDependency(node) { if ( node.parent.type === 'MemberExpression' && node.parent.object === node && - !node.parent.computed + !node.parent.computed && + !( + node.parent.parent != null && + node.parent.parent.type === 'CallExpression' && + node.parent.parent.callee === node.parent + ) ) { return node.parent; } else { From 14b222972ede7a9f9dbe0caec1a119e802e40d4c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 15 Feb 2019 18:23:07 +0000 Subject: [PATCH 19/33] Special case 'current' for refs --- .../__tests__/ESLintRuleReactiveDeps-test.js | 79 +++++++++++++++++++ .../src/ReactiveDeps.js | 1 + 2 files changed, 80 insertions(+) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index 1d18c1bc526f..c9e052390d63 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -254,6 +254,16 @@ const tests = { } `, }, + { + code: ` + function MyComponent() { + const ref = useRef(); + useEffect(() => { + console.log(ref.current); + }, [ref]); + } + `, + }, ], invalid: [ { @@ -1057,6 +1067,75 @@ const tests = { 'Currently only simple variables are supported.', ], }, + { + code: ` + function MyComponent() { + const ref = useRef(); + useEffect(() => { + console.log(ref.current); + }, []); + } + `, + output: ` + function MyComponent() { + const ref = useRef(); + useEffect(() => { + console.log(ref.current); + }, [ref]); + } + `, + // TODO: better message for the ref case. + errors: [ + 'React Hook useEffect has missing [ref] dependencies. ' + + 'Either fix or remove the dependency array.', + ], + }, + { + code: ` + function MyComponent(props) { + const ref1 = useRef(); + const ref2 = useRef(); + useEffect(() => { + ref1.current.focus(); + console.log(ref2.current.textContent); + alert(props.someOtherRefs.current.innerHTML); + fetch(props.color); + }, []); + } + `, + output: ` + function MyComponent(props) { + const ref1 = useRef(); + const ref2 = useRef(); + useEffect(() => { + ref1.current.focus(); + console.log(ref2.current.textContent); + alert(props.someOtherRefs.current.innerHTML); + fetch(props.color); + }, [ref1, ref2, props.someOtherRefs, props.color]); + } + `, + // TODO: better message for the ref case. + errors: [ + 'React Hook useEffect has missing [ref1, ref2, props.someOtherRefs, props.color] dependencies. ' + + 'Either fix or remove the dependency array.', + ], + }, + { + code: ` + function MyComponent() { + const ref = useRef(); + useEffect(() => { + console.log(ref.current); + }, [ref.current]); + } + `, + // TODO: better message for the ref case. + errors: [ + 'React Hook useEffect has missing [ref], unnecessary [ref.current] dependencies. ' + + 'Either fix or remove the dependency array.', + ], + }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index 10a25cf25f7a..fd1396d7f006 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -355,6 +355,7 @@ function getDependency(node) { if ( node.parent.type === 'MemberExpression' && node.parent.object === node && + node.parent.property.name !== 'current' && !node.parent.computed && !( node.parent.parent != null && From 366d5df7abb4f814ce209a7eb32609279162a012 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 18 Feb 2019 15:51:09 +0000 Subject: [PATCH 20/33] Don't complain about known static deps --- .../__tests__/ESLintRuleReactiveDeps-test.js | 161 ++++++++++++++++-- .../src/ReactiveDeps.js | 45 ++++- 2 files changed, 186 insertions(+), 20 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index c9e052390d63..8edbe109df22 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -264,6 +264,128 @@ const tests = { } `, }, + { + code: ` + function MyComponent() { + const ref = useRef(); + useEffect(() => { + console.log(ref.current); + }, []); + } + `, + }, + { + code: ` + function MyComponent({ maybeRef2 }) { + const definitelyRef1 = useRef(); + const definitelyRef2 = useRef(); + const maybeRef1 = useSomeOtherRefyThing(); + const [state1, setState1] = useState(); + const [state2, setState2] = useState(); + const [state3, dispatch1] = useReducer(); + const [state4, dispatch2] = useReducer(); + const [state5, maybeSetState] = useFunnyState(); + const [state6, maybeDispatch] = useFunnyReducer(); + function mySetState() {} + function myDispatch() {} + + useEffect(() => { + // Known to be static + console.log(definitelyRef1.current); + console.log(definitelyRef2.current); + console.log(maybeRef1.current); + console.log(maybeRef2.current); + setState1(); + setState2(); + dispatch1(); + dispatch2(); + + // Dynamic + console.log(state1); + console.log(state2); + console.log(state3); + console.log(state4); + console.log(state5); + console.log(state6); + mySetState(); + myDispatch(); + + // Not sure; assume dynamic + maybeSetState(); + maybeDispatch(); + }, [ + // Dynamic + state1, state2, state3, state4, state5, state6, + maybeRef1, maybeRef2, + + // Not sure; assume dynamic + mySetState, myDispatch, + maybeSetState, maybeDispatch + + // In this test, we don't specify static deps. + // That should be okay. + ]); + } + `, + }, + { + code: ` + function MyComponent({ maybeRef2 }) { + const definitelyRef1 = useRef(); + const definitelyRef2 = useRef(); + const maybeRef1 = useSomeOtherRefyThing(); + + const [state1, setState1] = useState(); + const [state2, setState2] = useState(); + const [state3, dispatch1] = useReducer(); + const [state4, dispatch2] = useReducer(); + + const [state5, maybeSetState] = useFunnyState(); + const [state6, maybeDispatch] = useFunnyReducer(); + + function mySetState() {} + function myDispatch() {} + + useEffect(() => { + // Known to be static + console.log(definitelyRef1.current); + console.log(definitelyRef2.current); + console.log(maybeRef1.current); + console.log(maybeRef2.current); + setState1(); + setState2(); + dispatch1(); + dispatch2(); + + // Dynamic + console.log(state1); + console.log(state2); + console.log(state3); + console.log(state4); + console.log(state5); + console.log(state6); + mySetState(); + myDispatch(); + + // Not sure; assume dynamic + maybeSetState(); + maybeDispatch(); + }, [ + // Dynamic + state1, state2, state3, state4, state5, state6, + maybeRef1, maybeRef2, + + // Not sure; assume dynamic + mySetState, myDispatch, + maybeSetState, maybeDispatch, + + // In this test, we specify static deps. + // That should be okay too! + definitelyRef1, definitelyRef2, setState1, setState2, dispatch1, dispatch2 + ]); + } + `, + }, ], invalid: [ { @@ -1071,22 +1193,25 @@ const tests = { code: ` function MyComponent() { const ref = useRef(); + const [state, setState] = useState(); useEffect(() => { - console.log(ref.current); + ref.current = 42; + setState(state + 1); }, []); } `, output: ` function MyComponent() { const ref = useRef(); + const [state, setState] = useState(); useEffect(() => { - console.log(ref.current); - }, [ref]); + ref.current = 42; + setState(state + 1); + }, [ref, setState, state]); } `, - // TODO: better message for the ref case. errors: [ - 'React Hook useEffect has missing [ref] dependencies. ' + + 'React Hook useEffect has missing [state] dependencies. ' + 'Either fix or remove the dependency array.', ], }, @@ -1117,22 +1242,30 @@ const tests = { `, // TODO: better message for the ref case. errors: [ - 'React Hook useEffect has missing [ref1, ref2, props.someOtherRefs, props.color] dependencies. ' + + 'React Hook useEffect has missing [props.someOtherRefs, props.color] dependencies. ' + 'Either fix or remove the dependency array.', ], }, { code: ` - function MyComponent() { - const ref = useRef(); - useEffect(() => { - console.log(ref.current); - }, [ref.current]); - } - `, + function MyComponent() { + const ref = useRef(); + useEffect(() => { + console.log(ref.current); + }, [ref.current]); + } + `, + output: ` + function MyComponent() { + const ref = useRef(); + useEffect(() => { + console.log(ref.current); + }, [ref]); + } + `, // TODO: better message for the ref case. errors: [ - 'React Hook useEffect has missing [ref], unnecessary [ref.current] dependencies. ' + + 'React Hook useEffect has unnecessary [ref.current] dependencies. ' + 'Either fix or remove the dependency array.', ], }, diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index fd1396d7f006..c72def073a9b 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -193,11 +193,42 @@ export default { const dependency = normalizeDependencyNode(dependencyNode); // Add the dependency to a map so we can make sure it is referenced // again in our dependencies array. - let nodes = dependencies.get(dependency); - if (!nodes) { - dependencies.set(dependency, (nodes = [])); + let info = dependencies.get(dependency); + if (!info) { + info = {isKnownToBeStatic: false}; + dependencies.set(dependency, info); + + if ( + reference.resolved != null && + Array.isArray(reference.resolved.defs) + ) { + const def = reference.resolved.defs[0]; + if (def != null && def.node.init != null) { + const init = def.node.init; + if (init.callee != null) { + if ( + init.callee.name === 'useRef' && + def.node.id.type === 'Identifier' + ) { + info.isKnownToBeStatic = true; + } else if ( + init.callee.name === 'useState' || + init.callee.name === 'useReducer' + ) { + if ( + def.node.id.type === 'ArrayPattern' && + def.node.id.elements.length === 2 && + Array.isArray(reference.resolved.identifiers) && + def.node.id.elements[1] === + reference.resolved.identifiers[0] + ) { + info.isKnownToBeStatic = true; + } + } + } + } + } } - nodes.push(dependencyNode); } for (const childScope of currentScope.childScopes) { gatherDependenciesRecursively(childScope); @@ -292,7 +323,7 @@ export default { } }); // Then fill in the missing ones. - Array.from(dependencies.keys()).forEach(key => { + dependencies.forEach((info, key) => { if ( !suggestedDependencies.some(suggestedDep => satisfies(key, suggestedDep), @@ -300,7 +331,9 @@ export default { ) { // Legit missing. suggestedDependencies.push(key); - missingDependencies.add(key); + if (!info.isKnownToBeStatic) { + missingDependencies.add(key); + } } else { // Already did that. Do nothing. } From 3d46a1ea3f9a76eb36e40f28cf6728ae9405fa23 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 18 Feb 2019 18:36:00 +0000 Subject: [PATCH 21/33] Support useImperativeHandle --- .../__tests__/ESLintRuleReactiveDeps-test.js | 46 +++++++++++++ .../src/ReactiveDeps.js | 67 +++++++++---------- 2 files changed, 76 insertions(+), 37 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index 8edbe109df22..0df2baffa9e0 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -386,6 +386,28 @@ const tests = { } `, }, + { + code: ` + const MyComponent = forwardRef((props, ref) => { + useImperativeHandle(ref, () => ({ + focus() { + alert(props.hello); + } + })) + }); + `, + }, + { + code: ` + const MyComponent = forwardRef((props, ref) => { + useImperativeHandle(ref, () => ({ + focus() { + alert(props.hello); + } + }), [props.hello]) + }); + `, + }, ], invalid: [ { @@ -1269,6 +1291,30 @@ const tests = { 'Either fix or remove the dependency array.', ], }, + { + code: ` + const MyComponent = forwardRef((props, ref) => { + useImperativeHandle(ref, () => ({ + focus() { + alert(props.hello); + } + }), []) + }); + `, + output: ` + const MyComponent = forwardRef((props, ref) => { + useImperativeHandle(ref, () => ({ + focus() { + alert(props.hello); + } + }), [props.hello]) + }); + `, + errors: [ + 'React Hook useImperativeHandle has missing [props.hello] dependencies. ' + + 'Either fix or remove the dependency array.', + ], + }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index c72def073a9b..3f472867f167 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -9,21 +9,11 @@ 'use strict'; -/** - * Is this node a reactive React Hook? This includes: - * - * - `useEffect()` - * - `useCallback()` - * - `useMemo()` - * - * TODO: implement autofix. - * - * Also supports `React` namespacing. e.g. `React.useEffect()`. - * - * NOTE: This is a very naive check. We don't look to make sure these reactive - * hooks are imported correctly. - */ -function isReactiveHook(node, options) { +// -1 if not a reactive Hook. +// 0 for useEffect/useMemo/useCallback. +// 1 for useImperativeHandle. +// For additionally configured Hooks, assume 0. +function getReactiveHookCallbackIndex(node, options) { if ( node.type === 'MemberExpression' && node.object.type === 'Identifier' && @@ -31,7 +21,7 @@ function isReactiveHook(node, options) { node.property.type === 'Identifier' && !node.computed ) { - return isReactiveHook(node.property); + return getReactiveHookCallbackIndex(node.property); } else if ( node.type === 'Identifier' && (node.name === 'useEffect' || @@ -39,7 +29,12 @@ function isReactiveHook(node, options) { node.name === 'useCallback' || node.name === 'useMemo') ) { - return true; + return 0; + } else if ( + node.type === 'Identifier' && + node.name === 'useImperativeHandle' + ) { + return 1; } else if (options && options.additionalHooks) { // Allow the user to provide a regular expression which enables the lint to // target custom reactive hooks. @@ -48,14 +43,14 @@ function isReactiveHook(node, options) { name = getAdditionalHookName(node); } catch (error) { if (/Unsupported node type/.test(error.message)) { - return false; + return 0; } else { throw error; } } - return options.additionalHooks.test(name); + return options.additionalHooks.test(name) ? 0 : -1; } else { - return false; + return -1; } } @@ -74,21 +69,6 @@ function getAdditionalHookName(node) { } } -/** - * Is this node the callback for a reactive hook? It is if the parent is a call - * expression with a reactive hook callee and this node is a function expression - * and the first argument. - */ -function isReactiveHookCallback(node, options) { - return ( - (node.type === 'FunctionExpression' || - node.type === 'ArrowFunctionExpression') && - node.parent.type === 'CallExpression' && - isReactiveHook(node.parent.callee, options) && - node.parent.arguments[0] === node - ); -} - export default { meta: { fixable: 'code', @@ -124,7 +104,19 @@ export default { */ function visitFunctionExpression(node) { // We only want to lint nodes which are reactive hook callbacks. - if (!isReactiveHookCallback(node, options)) { + if ( + (node.type !== 'FunctionExpression' && + node.type !== 'ArrowFunctionExpression') || + node.parent.type !== 'CallExpression' + ) { + return; + } + + const callbackIndex = getReactiveHookCallbackIndex( + node.parent.callee, + options, + ); + if (node.parent.arguments[callbackIndex] !== node) { return; } @@ -134,7 +126,8 @@ export default { // Get the declared dependencies for this reactive hook. If there is no // second argument then the reactive callback will re-run on every render. // So no need to check for dependency inclusion. - const declaredDependenciesNode = node.parent.arguments[1]; + const depsIndex = callbackIndex + 1; + const declaredDependenciesNode = node.parent.arguments[depsIndex]; if (!declaredDependenciesNode) { return; } From 367962752f399f627e7a1f68b4b93384a82fe0ac Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 18 Feb 2019 20:21:51 +0000 Subject: [PATCH 22/33] Better punctuation and formatting --- .../__tests__/ESLintRuleReactiveDeps-test.js | 161 +++++++++--------- .../src/ReactiveDeps.js | 87 ++++++---- 2 files changed, 136 insertions(+), 112 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index 0df2baffa9e0..35669b0b89ec 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -428,8 +428,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [local] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'local' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -456,8 +456,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [local] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'local' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -484,8 +484,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [local] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'local' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -514,8 +514,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [local] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'local' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -544,8 +544,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [local1, local2] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has missing 'local1' and 'local2' dependencies. " + + 'Either include them or remove the dependency array.', ], }, { @@ -570,8 +570,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [local2] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'local2' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -594,8 +594,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has unnecessary [local2] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has an unnecessary 'local2' dependency. " + + 'Either exclude it or remove the dependency array.', ], }, { @@ -626,8 +626,9 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [local2], unnecessary [local1] dependencies. ' + - 'Either fix or remove the dependency array.', + // Focus on the more important part first (missing dep) + "React Hook useEffect has a missing 'local2' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -650,8 +651,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [local] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'local' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -674,8 +675,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has duplicate [local] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a duplicate 'local' dependency. " + + 'Either omit it or remove the dependency array.', ], }, { @@ -690,8 +691,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has unnecessary [local] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has an unnecessary 'local' dependency. " + + 'Either exclude it or remove the dependency array.', ], }, { @@ -710,8 +711,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [history] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'history' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -736,8 +737,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [history.foo] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'history.foo' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -783,8 +784,8 @@ const tests = { 'React Hook useEffect has a second argument which is not an array ' + "literal. This means we can't statically verify whether you've " + 'passed the correct dependencies.', - 'React Hook useEffect has missing [local] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'local' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -808,8 +809,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [local] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'local' dependency. " + + 'Either include it or remove the dependency array.', 'React Hook useEffect has a spread element in its dependency list. ' + "This means we can't statically verify whether you've passed the " + 'correct dependencies.', @@ -858,8 +859,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [local] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'local' dependency. " + + 'Either include it or remove the dependency array.', "Unsupported expression in React Hook useEffect's dependency list. " + 'Currently only simple variables are supported.', ], @@ -884,8 +885,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [local], unnecessary [local.id] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'local' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -906,8 +907,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has duplicate [local] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a duplicate 'local' dependency. " + + 'Either omit it or remove the dependency array.', ], }, { @@ -930,8 +931,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has unnecessary [local1] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has an unnecessary 'local1' dependency. " + + 'Either exclude it or remove the dependency array.', ], }, { @@ -948,8 +949,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has unnecessary [local1] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has an unnecessary 'local1' dependency. " + + 'Either exclude it or remove the dependency array.', ], }, { @@ -970,8 +971,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [props.foo] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'props.foo' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -994,8 +995,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [props.foo, props.bar] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has missing 'props.foo' and 'props.bar' dependencies. " + + 'Either include them or remove the dependency array.', ], }, { @@ -1022,8 +1023,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [props.foo, props.bar, local] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has missing 'props.foo', 'props.bar', and 'local' dependencies. " + + 'Either include them or remove the dependency array.', ], }, { @@ -1048,8 +1049,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [local] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'local' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -1104,18 +1105,18 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [props.foo] dependencies. ' + - 'Either fix or remove the dependency array.', - 'React Hook useCallback has missing [props.foo] dependencies. ' + - 'Either fix or remove the dependency array.', - 'React Hook useMemo has missing [props.foo] dependencies. ' + - 'Either fix or remove the dependency array.', - 'React Hook React.useEffect has missing [props.foo] dependencies. ' + - 'Either fix or remove the dependency array.', - 'React Hook React.useCallback has missing [props.foo] dependencies. ' + - 'Either fix or remove the dependency array.', - 'React Hook React.useMemo has missing [props.foo] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'props.foo' dependency. " + + 'Either include it or remove the dependency array.', + "React Hook useCallback has a missing 'props.foo' dependency. " + + 'Either include it or remove the dependency array.', + "React Hook useMemo has a missing 'props.foo' dependency. " + + 'Either include it or remove the dependency array.', + "React Hook React.useEffect has a missing 'props.foo' dependency. " + + 'Either include it or remove the dependency array.', + "React Hook React.useCallback has a missing 'props.foo' dependency. " + + 'Either include it or remove the dependency array.', + "React Hook React.useMemo has a missing 'props.foo' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -1153,12 +1154,12 @@ const tests = { `, options: [{additionalHooks: 'useCustomEffect'}], errors: [ - 'React Hook useCustomEffect has missing [props.foo] dependencies. ' + - 'Either fix or remove the dependency array.', - 'React Hook useEffect has missing [props.foo] dependencies. ' + - 'Either fix or remove the dependency array.', - 'React Hook React.useEffect has missing [props.foo] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useCustomEffect has a missing 'props.foo' dependency. " + + 'Either include it or remove the dependency array.', + "React Hook useEffect has a missing 'props.foo' dependency. " + + 'Either include it or remove the dependency array.', + "React Hook React.useEffect has a missing 'props.foo' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -1180,8 +1181,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [local] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'local' dependency. " + + 'Either include it or remove the dependency array.', "Unsupported expression in React Hook useEffect's dependency list. " + 'Currently only simple variables are supported.', ], @@ -1205,8 +1206,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [local] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'local' dependency. " + + 'Either include it or remove the dependency array.', "Unsupported expression in React Hook useEffect's dependency list. " + 'Currently only simple variables are supported.', ], @@ -1233,8 +1234,8 @@ const tests = { } `, errors: [ - 'React Hook useEffect has missing [state] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has a missing 'state' dependency. " + + 'Either include it or remove the dependency array.', ], }, { @@ -1262,10 +1263,10 @@ const tests = { }, [ref1, ref2, props.someOtherRefs, props.color]); } `, - // TODO: better message for the ref case. + // TODO: special message for the ref case. errors: [ - 'React Hook useEffect has missing [props.someOtherRefs, props.color] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has missing 'props.someOtherRefs' and 'props.color' dependencies. " + + 'Either include them or remove the dependency array.', ], }, { @@ -1285,10 +1286,10 @@ const tests = { }, [ref]); } `, - // TODO: better message for the ref case. + // TODO: special message for the ref case. errors: [ - 'React Hook useEffect has unnecessary [ref.current] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useEffect has an unnecessary 'ref.current' dependency. " + + 'Either exclude it or remove the dependency array.', ], }, { @@ -1311,8 +1312,8 @@ const tests = { }); `, errors: [ - 'React Hook useImperativeHandle has missing [props.hello] dependencies. ' + - 'Either fix or remove the dependency array.', + "React Hook useImperativeHandle has a missing 'props.hello' dependency. " + + 'Either include it or remove the dependency array.', ], }, ], diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index 3f472867f167..bff4426b88d1 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -332,39 +332,62 @@ export default { } }); - if ( - duplicateDependencies.size > 0 || - missingDependencies.size > 0 || - unnecessaryDependencies.size > 0 - ) { - context.report({ - node: declaredDependenciesNode, - message: - `React Hook ${context.getSource(reactiveHook)} has ` + - [ - missingDependencies.size > 0 - ? `missing [${Array.from(missingDependencies).join(', ')}]` - : null, - duplicateDependencies.size > 0 - ? `duplicate [${Array.from(duplicateDependencies).join(', ')}]` - : null, - unnecessaryDependencies.size > 0 - ? `unnecessary [${Array.from(unnecessaryDependencies).join( - ', ', - )}]` - : null, - ] - .filter(Boolean) - .join(', ') + - ` dependencies. Either fix or remove the dependency array.`, - fix(fixer) { - return fixer.replaceText( - declaredDependenciesNode, - `[${suggestedDependencies.join(', ')}]`, - ); - }, - }); + const problemCount = + duplicateDependencies.size + + missingDependencies.size + + unnecessaryDependencies.size; + + if (problemCount === 0) { + return; } + + const quote = name => "'" + name + "'"; + const join = (arr, forceComma) => { + let s = ''; + for (let i = 0; i < arr.length; i++) { + s += arr[i]; + if (i === 0 && arr.length === 2) { + s += ' and '; + } else if (i === arr.length - 2 && arr.length > 2) { + s += ', and '; + } else if (i < arr.length - 1) { + s += ', '; + } + } + return s; + }; + const list = (set, label, singlePrefix, fixVerb) => { + if (set.size === 0) { + return null; + } + return ( + (set.size > 1 ? '' : singlePrefix + ' ') + + label + + ' ' + + join(Array.from(set).map(quote)) + + ' ' + + (set.size > 1 ? 'dependencies' : 'dependency') + + `. Either ${fixVerb} ${ + set.size > 1 ? 'them' : 'it' + } or remove the dependency array.` + ); + }; + context.report({ + node: declaredDependenciesNode, + message: + `React Hook ${context.getSource(reactiveHook)} has ` + + // To avoid a long message, show the next actionable item. + (list(missingDependencies, 'missing', 'a', 'include') || + list(unnecessaryDependencies, 'unnecessary', 'an', 'exclude') || + list(duplicateDependencies, 'duplicate', 'a', 'omit')), + fix(fixer) { + // TODO: consider keeping the comments? + return fixer.replaceText( + declaredDependenciesNode, + `[${suggestedDependencies.join(', ')}]`, + ); + }, + }); } }, }; From 5d2c991c6965785da093a1b3b6acb4ca6c97df4a Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 19 Feb 2019 14:49:53 +0000 Subject: [PATCH 23/33] More uniform message format --- .../__tests__/ESLintRuleReactiveDeps-test.js | 78 +++++++++---------- .../src/ReactiveDeps.js | 12 +-- 2 files changed, 45 insertions(+), 45 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index 35669b0b89ec..a625084eef17 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -428,7 +428,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'local' dependency. " + + "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', ], }, @@ -456,7 +456,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'local' dependency. " + + "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', ], }, @@ -484,7 +484,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'local' dependency. " + + "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', ], }, @@ -514,7 +514,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'local' dependency. " + + "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', ], }, @@ -544,7 +544,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has missing 'local1' and 'local2' dependencies. " + + "React Hook useEffect has missing dependencies: 'local1' and 'local2'. " + 'Either include them or remove the dependency array.', ], }, @@ -570,7 +570,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'local2' dependency. " + + "React Hook useEffect has a missing dependency: 'local2'. " + 'Either include it or remove the dependency array.', ], }, @@ -594,7 +594,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has an unnecessary 'local2' dependency. " + + "React Hook useEffect has an unnecessary dependency: 'local2'. " + 'Either exclude it or remove the dependency array.', ], }, @@ -627,7 +627,7 @@ const tests = { `, errors: [ // Focus on the more important part first (missing dep) - "React Hook useEffect has a missing 'local2' dependency. " + + "React Hook useEffect has a missing dependency: 'local2'. " + 'Either include it or remove the dependency array.', ], }, @@ -651,7 +651,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'local' dependency. " + + "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', ], }, @@ -675,7 +675,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a duplicate 'local' dependency. " + + "React Hook useEffect has a duplicate dependency: 'local'. " + 'Either omit it or remove the dependency array.', ], }, @@ -691,7 +691,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has an unnecessary 'local' dependency. " + + "React Hook useEffect has an unnecessary dependency: 'local'. " + 'Either exclude it or remove the dependency array.', ], }, @@ -711,7 +711,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'history' dependency. " + + "React Hook useEffect has a missing dependency: 'history'. " + 'Either include it or remove the dependency array.', ], }, @@ -737,7 +737,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'history.foo' dependency. " + + "React Hook useEffect has a missing dependency: 'history.foo'. " + 'Either include it or remove the dependency array.', ], }, @@ -784,7 +784,7 @@ const tests = { 'React Hook useEffect has a second argument which is not an array ' + "literal. This means we can't statically verify whether you've " + 'passed the correct dependencies.', - "React Hook useEffect has a missing 'local' dependency. " + + "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', ], }, @@ -809,7 +809,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'local' dependency. " + + "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', 'React Hook useEffect has a spread element in its dependency list. ' + "This means we can't statically verify whether you've passed the " + @@ -859,7 +859,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'local' dependency. " + + "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', "Unsupported expression in React Hook useEffect's dependency list. " + 'Currently only simple variables are supported.', @@ -885,7 +885,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'local' dependency. " + + "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', ], }, @@ -907,7 +907,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a duplicate 'local' dependency. " + + "React Hook useEffect has a duplicate dependency: 'local'. " + 'Either omit it or remove the dependency array.', ], }, @@ -931,7 +931,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has an unnecessary 'local1' dependency. " + + "React Hook useEffect has an unnecessary dependency: 'local1'. " + 'Either exclude it or remove the dependency array.', ], }, @@ -949,7 +949,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has an unnecessary 'local1' dependency. " + + "React Hook useEffect has an unnecessary dependency: 'local1'. " + 'Either exclude it or remove the dependency array.', ], }, @@ -971,7 +971,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'props.foo' dependency. " + + "React Hook useEffect has a missing dependency: 'props.foo'. " + 'Either include it or remove the dependency array.', ], }, @@ -995,7 +995,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has missing 'props.foo' and 'props.bar' dependencies. " + + "React Hook useEffect has missing dependencies: 'props.foo' and 'props.bar'. " + 'Either include them or remove the dependency array.', ], }, @@ -1023,7 +1023,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has missing 'props.foo', 'props.bar', and 'local' dependencies. " + + "React Hook useEffect has missing dependencies: 'props.foo', 'props.bar', and 'local'. " + 'Either include them or remove the dependency array.', ], }, @@ -1049,7 +1049,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'local' dependency. " + + "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', ], }, @@ -1105,17 +1105,17 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'props.foo' dependency. " + + "React Hook useEffect has a missing dependency: 'props.foo'. " + 'Either include it or remove the dependency array.', - "React Hook useCallback has a missing 'props.foo' dependency. " + + "React Hook useCallback has a missing dependency: 'props.foo'. " + 'Either include it or remove the dependency array.', - "React Hook useMemo has a missing 'props.foo' dependency. " + + "React Hook useMemo has a missing dependency: 'props.foo'. " + 'Either include it or remove the dependency array.', - "React Hook React.useEffect has a missing 'props.foo' dependency. " + + "React Hook React.useEffect has a missing dependency: 'props.foo'. " + 'Either include it or remove the dependency array.', - "React Hook React.useCallback has a missing 'props.foo' dependency. " + + "React Hook React.useCallback has a missing dependency: 'props.foo'. " + 'Either include it or remove the dependency array.', - "React Hook React.useMemo has a missing 'props.foo' dependency. " + + "React Hook React.useMemo has a missing dependency: 'props.foo'. " + 'Either include it or remove the dependency array.', ], }, @@ -1154,11 +1154,11 @@ const tests = { `, options: [{additionalHooks: 'useCustomEffect'}], errors: [ - "React Hook useCustomEffect has a missing 'props.foo' dependency. " + + "React Hook useCustomEffect has a missing dependency: 'props.foo'. " + 'Either include it or remove the dependency array.', - "React Hook useEffect has a missing 'props.foo' dependency. " + + "React Hook useEffect has a missing dependency: 'props.foo'. " + 'Either include it or remove the dependency array.', - "React Hook React.useEffect has a missing 'props.foo' dependency. " + + "React Hook React.useEffect has a missing dependency: 'props.foo'. " + 'Either include it or remove the dependency array.', ], }, @@ -1181,7 +1181,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'local' dependency. " + + "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', "Unsupported expression in React Hook useEffect's dependency list. " + 'Currently only simple variables are supported.', @@ -1206,7 +1206,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'local' dependency. " + + "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', "Unsupported expression in React Hook useEffect's dependency list. " + 'Currently only simple variables are supported.', @@ -1234,7 +1234,7 @@ const tests = { } `, errors: [ - "React Hook useEffect has a missing 'state' dependency. " + + "React Hook useEffect has a missing dependency: 'state'. " + 'Either include it or remove the dependency array.', ], }, @@ -1265,7 +1265,7 @@ const tests = { `, // TODO: special message for the ref case. errors: [ - "React Hook useEffect has missing 'props.someOtherRefs' and 'props.color' dependencies. " + + "React Hook useEffect has missing dependencies: 'props.someOtherRefs' and 'props.color'. " + 'Either include them or remove the dependency array.', ], }, @@ -1288,7 +1288,7 @@ const tests = { `, // TODO: special message for the ref case. errors: [ - "React Hook useEffect has an unnecessary 'ref.current' dependency. " + + "React Hook useEffect has an unnecessary dependency: 'ref.current'. " + 'Either exclude it or remove the dependency array.', ], }, @@ -1312,7 +1312,7 @@ const tests = { }); `, errors: [ - "React Hook useImperativeHandle has a missing 'props.hello' dependency. " + + "React Hook useImperativeHandle has a missing dependency: 'props.hello'. " + 'Either include it or remove the dependency array.', ], }, diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index bff4426b88d1..ffb6771800ce 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -356,7 +356,7 @@ export default { } return s; }; - const list = (set, label, singlePrefix, fixVerb) => { + const list = (set, singlePrefix, label, fixVerb) => { if (set.size === 0) { return null; } @@ -364,9 +364,9 @@ export default { (set.size > 1 ? '' : singlePrefix + ' ') + label + ' ' + - join(Array.from(set).map(quote)) + - ' ' + (set.size > 1 ? 'dependencies' : 'dependency') + + ': ' + + join(Array.from(set).map(quote)) + `. Either ${fixVerb} ${ set.size > 1 ? 'them' : 'it' } or remove the dependency array.` @@ -377,9 +377,9 @@ export default { message: `React Hook ${context.getSource(reactiveHook)} has ` + // To avoid a long message, show the next actionable item. - (list(missingDependencies, 'missing', 'a', 'include') || - list(unnecessaryDependencies, 'unnecessary', 'an', 'exclude') || - list(duplicateDependencies, 'duplicate', 'a', 'omit')), + (list(missingDependencies, 'a', 'missing', 'include') || + list(unnecessaryDependencies, 'an', 'unnecessary', 'exclude') || + list(duplicateDependencies, 'a', 'duplicate', 'omit')), fix(fixer) { // TODO: consider keeping the comments? return fixer.replaceText( From e0203cb3b2d2918fae2475a896372b48cdb27f16 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 19 Feb 2019 14:56:26 +0000 Subject: [PATCH 24/33] Treat React.useRef/useState/useReducer as static too --- .../__tests__/ESLintRuleReactiveDeps-test.js | 8 ++--- .../src/ReactiveDeps.js | 34 ++++++++++++------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index a625084eef17..ee96f02ffa1c 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -281,9 +281,9 @@ const tests = { const definitelyRef2 = useRef(); const maybeRef1 = useSomeOtherRefyThing(); const [state1, setState1] = useState(); - const [state2, setState2] = useState(); + const [state2, setState2] = React.useState(); const [state3, dispatch1] = useReducer(); - const [state4, dispatch2] = useReducer(); + const [state4, dispatch2] = React.useReducer(); const [state5, maybeSetState] = useFunnyState(); const [state6, maybeDispatch] = useFunnyReducer(); function mySetState() {} @@ -336,9 +336,9 @@ const tests = { const maybeRef1 = useSomeOtherRefyThing(); const [state1, setState1] = useState(); - const [state2, setState2] = useState(); + const [state2, setState2] = React.useState(); const [state3, dispatch1] = useReducer(); - const [state4, dispatch2] = useReducer(); + const [state4, dispatch2] = React.useReducer(); const [state5, maybeSetState] = useFunnyState(); const [state6, maybeDispatch] = useFunnyReducer(); diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index ffb6771800ce..df1c5f3c6164 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -199,23 +199,33 @@ export default { if (def != null && def.node.init != null) { const init = def.node.init; if (init.callee != null) { + let callee = init.callee; if ( - init.callee.name === 'useRef' && - def.node.id.type === 'Identifier' - ) { - info.isKnownToBeStatic = true; - } else if ( - init.callee.name === 'useState' || - init.callee.name === 'useReducer' + callee.type === 'MemberExpression' && + callee.object.name === 'React' && + callee.property != null ) { + callee = callee.property; + } + if (callee.type === 'Identifier') { if ( - def.node.id.type === 'ArrayPattern' && - def.node.id.elements.length === 2 && - Array.isArray(reference.resolved.identifiers) && - def.node.id.elements[1] === - reference.resolved.identifiers[0] + callee.name === 'useRef' && + def.node.id.type === 'Identifier' ) { info.isKnownToBeStatic = true; + } else if ( + callee.name === 'useState' || + callee.name === 'useReducer' + ) { + if ( + def.node.id.type === 'ArrayPattern' && + def.node.id.elements.length === 2 && + Array.isArray(reference.resolved.identifiers) && + def.node.id.elements[1] === + reference.resolved.identifiers[0] + ) { + info.isKnownToBeStatic = true; + } } } } From 54c2e16f0642f9f82920e1fee21825c23e75349e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 19 Feb 2019 16:23:48 +0000 Subject: [PATCH 25/33] Add special message for ref.current --- .../__tests__/ESLintRuleReactiveDeps-test.js | 38 +++++++++++++++++-- .../src/ReactiveDeps.js | 21 +++++++++- 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index ee96f02ffa1c..e30eff76e68c 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -1263,12 +1263,43 @@ const tests = { }, [ref1, ref2, props.someOtherRefs, props.color]); } `, - // TODO: special message for the ref case. errors: [ "React Hook useEffect has missing dependencies: 'props.someOtherRefs' and 'props.color'. " + 'Either include them or remove the dependency array.', ], }, + { + code: ` + function MyComponent(props) { + const ref1 = useRef(); + const ref2 = useRef(); + useEffect(() => { + ref1.current.focus(); + console.log(ref2.current.textContent); + alert(props.someOtherRefs.current.innerHTML); + fetch(props.color); + }, [ref1.current, ref2.current, props.someOtherRefs, props.color]); + } + `, + output: ` + function MyComponent(props) { + const ref1 = useRef(); + const ref2 = useRef(); + useEffect(() => { + ref1.current.focus(); + console.log(ref2.current.textContent); + alert(props.someOtherRefs.current.innerHTML); + fetch(props.color); + }, [props.someOtherRefs, props.color, ref1, ref2]); + } + `, + errors: [ + "React Hook useEffect has unnecessary dependencies: 'ref1.current' and 'ref2.current'. " + + 'Either exclude them or remove the dependency array. ' + + "Mutable values like 'ref1.current' aren't valid dependencies " + + "because their mutation doesn't re-render the component.", + ], + }, { code: ` function MyComponent() { @@ -1286,10 +1317,11 @@ const tests = { }, [ref]); } `, - // TODO: special message for the ref case. errors: [ "React Hook useEffect has an unnecessary dependency: 'ref.current'. " + - 'Either exclude it or remove the dependency array.', + 'Either exclude it or remove the dependency array. ' + + "Mutable values like 'ref.current' aren't valid dependencies " + + "because their mutation doesn't re-render the component.", ], }, { diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index df1c5f3c6164..e0744c9a18e2 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -382,6 +382,24 @@ export default { } or remove the dependency array.` ); }; + let extraWarning = ''; + if (unnecessaryDependencies.size > 0) { + let badRef = null; + Array.from(unnecessaryDependencies.keys()).forEach(key => { + if (badRef !== null) { + return; + } + if (key.endsWith('.current')) { + badRef = key; + } + }); + if (badRef !== null) { + extraWarning = + ` Mutable values like '${badRef}' aren't valid dependencies ` + + "because their mutation doesn't re-render the component."; + } + } + context.report({ node: declaredDependenciesNode, message: @@ -389,7 +407,8 @@ export default { // To avoid a long message, show the next actionable item. (list(missingDependencies, 'a', 'missing', 'include') || list(unnecessaryDependencies, 'an', 'unnecessary', 'exclude') || - list(duplicateDependencies, 'a', 'duplicate', 'omit')), + list(duplicateDependencies, 'a', 'duplicate', 'omit')) + + extraWarning, fix(fixer) { // TODO: consider keeping the comments? return fixer.replaceText( From 6fd2d12b4396970bf7127196dd8c347d7f01e99c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 19 Feb 2019 19:19:08 +0000 Subject: [PATCH 26/33] Add a TODO case --- .../__tests__/ESLintRuleReactiveDeps-test.js | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index e30eff76e68c..8c911e65068f 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -1348,6 +1348,32 @@ const tests = { 'Either include it or remove the dependency array.', ], }, + { + code: ` + function MyComponent(props) { + useEffect(() => { + if (props.onChange) { + props.onChange(); + } + }, []); + } + `, + // TODO: [props.onChange] is superfluous. Fix to just [props.onChange]. + output: ` + function MyComponent(props) { + useEffect(() => { + if (props.onChange) { + props.onChange(); + } + }, [props.onChange, props]); + } + `, + errors: [ + // TODO: reporting props separately is superfluous. Fix to just props.onChange. + "React Hook useEffect has missing dependencies: 'props.onChange' and 'props'. " + + 'Either include them or remove the dependency array.', + ], + }, ], }; From 0c722bb0d9d2e2256b0cd29398b3da96f5d20752 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 20 Feb 2019 15:24:37 +0000 Subject: [PATCH 27/33] Alphabetize the autofix --- .../__tests__/ESLintRuleReactiveDeps-test.js | 54 +++++++++++++++---- .../src/ReactiveDeps.js | 8 ++- 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index 8c911e65068f..cea8e472817c 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -174,6 +174,17 @@ const tests = { } `, }, + { + // TODO: we might want to forbid dot-access in deps. + code: ` + function MyComponent(props) { + useEffect(() => { + console.log(props.foo); + console.log(props.bar); + }, [props.bar, props.foo]); + } + `, + }, { // TODO: we might want to forbid dot-access in deps. code: ` @@ -991,11 +1002,34 @@ const tests = { useEffect(() => { console.log(props.foo); console.log(props.bar); - }, [props.foo, props.bar]); + }, [props.bar, props.foo]); + } + `, + errors: [ + "React Hook useEffect has missing dependencies: 'props.bar' and 'props.foo'. " + + 'Either include them or remove the dependency array.', + ], + }, + { + code: ` + function MyComponent(props) { + let a, b, c, d, e, f, g; + useEffect(() => { + console.log(b, e, d, c, a, g, f); + }, [c, a, g]); + } + `, + // Alphabetize during the autofix. + output: ` + function MyComponent(props) { + let a, b, c, d, e, f, g; + useEffect(() => { + console.log(b, e, d, c, a, g, f); + }, [a, b, c, d, e, f, g]); } `, errors: [ - "React Hook useEffect has missing dependencies: 'props.foo' and 'props.bar'. " + + "React Hook useEffect has missing dependencies: 'b', 'd', 'e', and 'f'. " + 'Either include them or remove the dependency array.', ], }, @@ -1019,11 +1053,11 @@ const tests = { console.log(props.foo); console.log(props.bar); console.log(local); - }, [props.foo, props.bar, local]); + }, [local, props.bar, props.foo]); } `, errors: [ - "React Hook useEffect has missing dependencies: 'props.foo', 'props.bar', and 'local'. " + + "React Hook useEffect has missing dependencies: 'local', 'props.bar', and 'props.foo'. " + 'Either include them or remove the dependency array.', ], }, @@ -1045,7 +1079,7 @@ const tests = { console.log(props.foo); console.log(props.bar); console.log(local); - }, [props, local]); + }, [local, props]); } `, errors: [ @@ -1260,11 +1294,11 @@ const tests = { console.log(ref2.current.textContent); alert(props.someOtherRefs.current.innerHTML); fetch(props.color); - }, [ref1, ref2, props.someOtherRefs, props.color]); + }, [props.color, props.someOtherRefs, ref1, ref2]); } `, errors: [ - "React Hook useEffect has missing dependencies: 'props.someOtherRefs' and 'props.color'. " + + "React Hook useEffect has missing dependencies: 'props.color' and 'props.someOtherRefs'. " + 'Either include them or remove the dependency array.', ], }, @@ -1290,7 +1324,7 @@ const tests = { console.log(ref2.current.textContent); alert(props.someOtherRefs.current.innerHTML); fetch(props.color); - }, [props.someOtherRefs, props.color, ref1, ref2]); + }, [props.color, props.someOtherRefs, ref1, ref2]); } `, errors: [ @@ -1365,12 +1399,12 @@ const tests = { if (props.onChange) { props.onChange(); } - }, [props.onChange, props]); + }, [props, props.onChange]); } `, errors: [ // TODO: reporting props separately is superfluous. Fix to just props.onChange. - "React Hook useEffect has missing dependencies: 'props.onChange' and 'props'. " + + "React Hook useEffect has missing dependencies: 'props' and 'props.onChange'. " + 'Either include them or remove the dependency array.', ], }, diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index e0744c9a18e2..ef01c86097ff 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -341,6 +341,8 @@ export default { // Already did that. Do nothing. } }); + // Alphabetize for the autofix. + suggestedDependencies.sort(); const problemCount = duplicateDependencies.size + @@ -376,7 +378,11 @@ export default { ' ' + (set.size > 1 ? 'dependencies' : 'dependency') + ': ' + - join(Array.from(set).map(quote)) + + join( + Array.from(set) + .sort() + .map(quote), + ) + `. Either ${fixVerb} ${ set.size > 1 ? 'them' : 'it' } or remove the dependency array.` From 12aeaca91144323e06e5139e68eb1dd267b1ad5e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 20 Feb 2019 16:20:44 +0000 Subject: [PATCH 28/33] Only alphabetize if it already was --- .../__tests__/ESLintRuleReactiveDeps-test.js | 50 ++++++++++++++++++- .../src/ReactiveDeps.js | 16 +++++- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index cea8e472817c..6c08930778aa 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -1019,7 +1019,30 @@ const tests = { }, [c, a, g]); } `, - // Alphabetize during the autofix. + // Don't alphabetize if it wasn't alphabetized in the first place. + output: ` + function MyComponent(props) { + let a, b, c, d, e, f, g; + useEffect(() => { + console.log(b, e, d, c, a, g, f); + }, [c, a, g, b, e, d, f]); + } + `, + errors: [ + "React Hook useEffect has missing dependencies: 'b', 'd', 'e', and 'f'. " + + 'Either include them or remove the dependency array.', + ], + }, + { + code: ` + function MyComponent(props) { + let a, b, c, d, e, f, g; + useEffect(() => { + console.log(b, e, d, c, a, g, f); + }, [a, c, g]); + } + `, + // Alphabetize if it was alphabetized. output: ` function MyComponent(props) { let a, b, c, d, e, f, g; @@ -1033,6 +1056,29 @@ const tests = { 'Either include them or remove the dependency array.', ], }, + { + code: ` + function MyComponent(props) { + let a, b, c, d, e, f, g; + useEffect(() => { + console.log(b, e, d, c, a, g, f); + }, []); + } + `, + // Alphabetize if it was empty. + output: ` + function MyComponent(props) { + let a, b, c, d, e, f, g; + useEffect(() => { + console.log(b, e, d, c, a, g, f); + }, [a, b, c, d, e, f, g]); + } + `, + errors: [ + "React Hook useEffect has missing dependencies: 'a', 'b', 'c', 'd', 'e', 'f', and 'g'. " + + 'Either include them or remove the dependency array.', + ], + }, { code: ` function MyComponent(props) { @@ -1324,7 +1370,7 @@ const tests = { console.log(ref2.current.textContent); alert(props.someOtherRefs.current.innerHTML); fetch(props.color); - }, [props.color, props.someOtherRefs, ref1, ref2]); + }, [props.someOtherRefs, props.color, ref1, ref2]); } `, errors: [ diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index ef01c86097ff..b947ca7661a3 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -341,8 +341,20 @@ export default { // Already did that. Do nothing. } }); - // Alphabetize for the autofix. - suggestedDependencies.sort(); + + function areDeclaredDepsAlphabetized() { + if (declaredDependencies.length === 0) { + return true; + } + const declaredDepKeys = declaredDependencies.map(dep => dep.key); + const sortedDeclaredDepKeys = declaredDepKeys.slice().sort(); + return declaredDepKeys.join(',') === sortedDeclaredDepKeys.join(','); + } + + if (areDeclaredDepsAlphabetized()) { + // Alphabetize the autofix, but only if deps were already alphabetized. + suggestedDependencies.sort(); + } const problemCount = duplicateDependencies.size + From c40bcaac356ef98254ca0d6ab7b48376ed051b1a Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 20 Feb 2019 16:36:17 +0000 Subject: [PATCH 29/33] Don't add static deps by default --- .../__tests__/ESLintRuleReactiveDeps-test.js | 59 ++++++++++++++++++- .../src/ReactiveDeps.js | 4 +- 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index 6c08930778aa..d4cc326d1320 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -1310,7 +1310,36 @@ const tests = { useEffect(() => { ref.current = 42; setState(state + 1); - }, [ref, setState, state]); + }, [state]); + } + `, + errors: [ + "React Hook useEffect has a missing dependency: 'state'. " + + 'Either include it or remove the dependency array.', + ], + }, + { + code: ` + function MyComponent() { + const ref = useRef(); + const [state, setState] = useState(); + useEffect(() => { + ref.current = 42; + setState(state + 1); + }, [ref]); + } + `, + // We don't ask to remove static deps but don't add them either. + // Don't suggest removing "ref" (it's fine either way) + // but *do* add "state". *Don't* add "setState" ourselves. + output: ` + function MyComponent() { + const ref = useRef(); + const [state, setState] = useState(); + useEffect(() => { + ref.current = 42; + setState(state + 1); + }, [ref, state]); } `, errors: [ @@ -1340,7 +1369,7 @@ const tests = { console.log(ref2.current.textContent); alert(props.someOtherRefs.current.innerHTML); fetch(props.color); - }, [props.color, props.someOtherRefs, ref1, ref2]); + }, [props.color, props.someOtherRefs]); } `, errors: [ @@ -1370,7 +1399,7 @@ const tests = { console.log(ref2.current.textContent); alert(props.someOtherRefs.current.innerHTML); fetch(props.color); - }, [props.someOtherRefs, props.color, ref1, ref2]); + }, [props.someOtherRefs, props.color]); } `, errors: [ @@ -1389,6 +1418,30 @@ const tests = { }, [ref.current]); } `, + output: ` + function MyComponent() { + const ref = useRef(); + useEffect(() => { + console.log(ref.current); + }, []); + } + `, + errors: [ + "React Hook useEffect has an unnecessary dependency: 'ref.current'. " + + 'Either exclude it or remove the dependency array. ' + + "Mutable values like 'ref.current' aren't valid dependencies " + + "because their mutation doesn't re-render the component.", + ], + }, + { + code: ` + function MyComponent() { + const ref = useRef(); + useEffect(() => { + console.log(ref.current); + }, [ref.current, ref]); + } + `, output: ` function MyComponent() { const ref = useRef(); diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index b947ca7661a3..b5cb94f09a9b 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -332,9 +332,9 @@ export default { satisfies(key, suggestedDep), ) ) { - // Legit missing. - suggestedDependencies.push(key); if (!info.isKnownToBeStatic) { + // Legit missing. + suggestedDependencies.push(key); missingDependencies.add(key); } } else { From 56785b571ed8ad54be9aaf90610bf73af8481c98 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 20 Feb 2019 16:39:31 +0000 Subject: [PATCH 30/33] Add an undefined variable case --- .../__tests__/ESLintRuleReactiveDeps-test.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index d4cc326d1320..25a551980a01 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -61,6 +61,20 @@ const tests = { } `, }, + { + // OK because `props` wasn't defined. + // We don't technically know if `props` is supposed + // to be an import that hasn't been added yet, or + // a component-level variable. Ignore it until it + // gets defined (a different rule would flag it anyway). + code: ` + function MyComponent() { + useEffect(() => { + console.log(props.foo); + }, []); + } + `, + }, { code: ` function MyComponent() { From 294b4742b22714e3add14dddbe9a0281bea95739 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 20 Feb 2019 16:54:54 +0000 Subject: [PATCH 31/33] Tweak wording --- .../__tests__/ESLintRuleReactiveDeps-test.js | 16 ++++++++-------- .../src/ReactiveDeps.js | 8 ++++---- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js index 25a551980a01..8fee1b2c1413 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js @@ -836,7 +836,7 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', - 'React Hook useEffect has a spread element in its dependency list. ' + + 'React Hook useEffect has a spread element in its dependency array. ' + "This means we can't statically verify whether you've passed the " + 'correct dependencies.', ], @@ -859,7 +859,7 @@ const tests = { } `, errors: [ - 'React Hook useEffect has a spread element in its dependency list. ' + + 'React Hook useEffect has a spread element in its dependency array. ' + "This means we can't statically verify whether you've passed the " + 'correct dependencies.', ], @@ -886,8 +886,8 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', - "Unsupported expression in React Hook useEffect's dependency list. " + - 'Currently only simple variables are supported.', + 'React Hook useEffect has a complex expression in the dependency array. ' + + 'Extract it to a separate variable so it can be statically checked.', ], }, { @@ -1277,8 +1277,8 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', - "Unsupported expression in React Hook useEffect's dependency list. " + - 'Currently only simple variables are supported.', + 'React Hook useEffect has a complex expression in the dependency array. ' + + 'Extract it to a separate variable so it can be statically checked.', ], }, { @@ -1302,8 +1302,8 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'local'. " + 'Either include it or remove the dependency array.', - "Unsupported expression in React Hook useEffect's dependency list. " + - 'Currently only simple variables are supported.', + 'React Hook useEffect has a complex expression in the dependency array. ' + + 'Extract it to a separate variable so it can be statically checked.', ], }, { diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js index b5cb94f09a9b..37db170c1ebd 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js @@ -262,7 +262,7 @@ export default { node: declaredDependencyNode, message: `React Hook ${context.getSource(reactiveHook)} has a spread ` + - "element in its dependency list. This means we can't " + + "element in its dependency array. This means we can't " + "statically verify whether you've passed the " + 'correct dependencies.', }); @@ -280,9 +280,9 @@ export default { context.report({ node: declaredDependencyNode, message: - 'Unsupported expression in React Hook ' + - `${context.getSource(reactiveHook)}'s dependency list. ` + - 'Currently only simple variables are supported.', + `React Hook ${context.getSource(reactiveHook)} has a ` + + `complex expression in the dependency array. ` + + 'Extract it to a separate variable so it can be statically checked.', }); return; } else { From 2f86a699b4c4dc01caa0ec9f41ac48f6844e97f3 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 20 Feb 2019 17:07:13 +0000 Subject: [PATCH 32/33] Rename to exhaustive-deps --- fixtures/eslint/.eslintrc.json | 2 +- fixtures/eslint/index.js | 2 +- ...eReactiveDeps-test.js => ESLintRuleExhaustiveDeps-test.js} | 2 +- .../src/{ReactiveDeps.js => ExhaustiveDeps.js} | 0 packages/eslint-plugin-react-hooks/src/index.js | 4 ++-- 5 files changed, 5 insertions(+), 5 deletions(-) rename packages/eslint-plugin-react-hooks/__tests__/{ESLintRuleReactiveDeps-test.js => ESLintRuleExhaustiveDeps-test.js} (99%) rename packages/eslint-plugin-react-hooks/src/{ReactiveDeps.js => ExhaustiveDeps.js} (100%) diff --git a/fixtures/eslint/.eslintrc.json b/fixtures/eslint/.eslintrc.json index 0c97574cfee8..2c6f6d2c0b6c 100644 --- a/fixtures/eslint/.eslintrc.json +++ b/fixtures/eslint/.eslintrc.json @@ -10,6 +10,6 @@ "plugins": ["react-hooks"], "rules": { "react-hooks/rules-of-hooks": 2, - "react-hooks/reactive-deps": 2 + "react-hooks/exhaustive-deps": 2 } } diff --git a/fixtures/eslint/index.js b/fixtures/eslint/index.js index 8b7db9ff07fb..1052cac180fc 100644 --- a/fixtures/eslint/index.js +++ b/fixtures/eslint/index.js @@ -24,6 +24,6 @@ function Comment({comment, commentSource}) { ); return () => subscription.dispose(); }, - [commentID, environment, currentUserID, commentSource] + [commentID, commentSource, currentUserID, environment] ); } diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js similarity index 99% rename from packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js rename to packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 8fee1b2c1413..6d16066ce032 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -9,7 +9,7 @@ const ESLintTester = require('eslint').RuleTester; const ReactHooksESLintPlugin = require('eslint-plugin-react-hooks'); -const ReactHooksESLintRule = ReactHooksESLintPlugin.rules['reactive-deps']; +const ReactHooksESLintRule = ReactHooksESLintPlugin.rules['exhaustive-deps']; ESLintTester.setDefaultConfig({ parser: 'babel-eslint', diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js similarity index 100% rename from packages/eslint-plugin-react-hooks/src/ReactiveDeps.js rename to packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js diff --git a/packages/eslint-plugin-react-hooks/src/index.js b/packages/eslint-plugin-react-hooks/src/index.js index f7e5c6fa2eb6..606a6dff4e25 100644 --- a/packages/eslint-plugin-react-hooks/src/index.js +++ b/packages/eslint-plugin-react-hooks/src/index.js @@ -8,9 +8,9 @@ 'use strict'; import RuleOfHooks from './RulesOfHooks'; -import ReactiveDeps from './ReactiveDeps'; +import ExhaustiveDeps from './ExhaustiveDeps'; export const rules = { 'rules-of-hooks': RuleOfHooks, - 'reactive-deps': ReactiveDeps, + 'exhaustive-deps': ExhaustiveDeps, }; From 8b554ca49a656ea8471ac8012b81525e994eec64 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 20 Feb 2019 18:00:00 +0000 Subject: [PATCH 33/33] Clean up / refactor a little bit --- .../ESLintRuleExhaustiveDeps-test.js | 87 +++++ .../src/ExhaustiveDeps.js | 319 +++++++++--------- 2 files changed, 254 insertions(+), 152 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 6d16066ce032..a6d75435463b 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -890,6 +890,93 @@ const tests = { 'Extract it to a separate variable so it can be statically checked.', ], }, + { + // only: true, + code: ` + function MyComponent(props) { + useEffect(() => { + console.log(props.items[0]); + }, [props.items[0]]); + } + `, + output: ` + function MyComponent(props) { + useEffect(() => { + console.log(props.items[0]); + }, [props.items]); + } + `, + errors: [ + "React Hook useEffect has a missing dependency: 'props.items'. " + + 'Either include it or remove the dependency array.', + 'React Hook useEffect has a complex expression in the dependency array. ' + + 'Extract it to a separate variable so it can be statically checked.', + ], + }, + { + code: ` + function MyComponent(props) { + useEffect(() => { + console.log(props.items[0]); + }, [props.items, props.items[0]]); + } + `, + // TODO: ideally autofix would remove the bad expression? + output: ` + function MyComponent(props) { + useEffect(() => { + console.log(props.items[0]); + }, [props.items, props.items[0]]); + } + `, + errors: [ + 'React Hook useEffect has a complex expression in the dependency array. ' + + 'Extract it to a separate variable so it can be statically checked.', + ], + }, + { + code: ` + function MyComponent({ items }) { + useEffect(() => { + console.log(items[0]); + }, [items[0]]); + } + `, + output: ` + function MyComponent({ items }) { + useEffect(() => { + console.log(items[0]); + }, [items]); + } + `, + errors: [ + "React Hook useEffect has a missing dependency: 'items'. " + + 'Either include it or remove the dependency array.', + 'React Hook useEffect has a complex expression in the dependency array. ' + + 'Extract it to a separate variable so it can be statically checked.', + ], + }, + { + code: ` + function MyComponent({ items }) { + useEffect(() => { + console.log(items[0]); + }, [items, items[0]]); + } + `, + // TODO: ideally autofix would remove the bad expression? + output: ` + function MyComponent({ items }) { + useEffect(() => { + console.log(items[0]); + }, [items, items[0]]); + } + `, + errors: [ + 'React Hook useEffect has a complex expression in the dependency array. ' + + 'Extract it to a separate variable so it can be statically checked.', + ], + }, { // TODO: need to think more about this case. code: ` diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 37db170c1ebd..34304c0c91e5 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -9,64 +9,60 @@ 'use strict'; -// -1 if not a reactive Hook. -// 0 for useEffect/useMemo/useCallback. -// 1 for useImperativeHandle. -// For additionally configured Hooks, assume 0. -function getReactiveHookCallbackIndex(node, options) { +// const [state, setState] = useState() / React.useState() +// ^^^ true for this reference +// const [state, dispatch] = useReducer() / React.useReducer() +// ^^^ true for this reference +// const ref = useRef() +// ^^ true for this reference +// False for everything else. +function isDefinitelyStaticDependency(reference) { + // This function is written defensively because I'm not sure about corner cases. + // TODO: we can strengthen this if we're sure about the types. + const resolved = reference.resolved; + if (resolved == null || !Array.isArray(resolved.defs)) { + return false; + } + const def = resolved.defs[0]; + if (def == null || def.node.init == null) { + return false; + } + // Look for `let stuff = SomeHook();` + const init = def.node.init; + if (init.callee == null) { + return false; + } + let callee = init.callee; + // Step into `= React.something` initializer. if ( - node.type === 'MemberExpression' && - node.object.type === 'Identifier' && - node.object.name === 'React' && - node.property.type === 'Identifier' && - !node.computed - ) { - return getReactiveHookCallbackIndex(node.property); - } else if ( - node.type === 'Identifier' && - (node.name === 'useEffect' || - node.name === 'useLayoutEffect' || - node.name === 'useCallback' || - node.name === 'useMemo') + callee.type === 'MemberExpression' && + callee.object.name === 'React' && + callee.property != null && + !callee.computed ) { - return 0; - } else if ( - node.type === 'Identifier' && - node.name === 'useImperativeHandle' - ) { - return 1; - } else if (options && options.additionalHooks) { - // Allow the user to provide a regular expression which enables the lint to - // target custom reactive hooks. - let name; - try { - name = getAdditionalHookName(node); - } catch (error) { - if (/Unsupported node type/.test(error.message)) { - return 0; - } else { - throw error; - } - } - return options.additionalHooks.test(name) ? 0 : -1; - } else { - return -1; + callee = callee.property; } -} - -/** - * Create a name we will test against our `additionalHooks` regular expression. - */ -function getAdditionalHookName(node) { - if (node.type === 'Identifier') { - return node.name; - } else if (node.type === 'MemberExpression' && !node.computed) { - const object = getAdditionalHookName(node.object); - const property = getAdditionalHookName(node.property); - return `${object}.${property}`; - } else { - throw new Error(`Unsupported node type: ${node.type}`); + if (callee.type !== 'Identifier') { + return; + } + const id = def.node.id; + if (callee.name === 'useRef' && id.type === 'Identifier') { + // useRef() return value is static. + return true; + } else if (callee.name === 'useState' || callee.name === 'useReducer') { + // Only consider second value in initializing tuple static. + if ( + id.type === 'ArrayPattern' && + id.elements.length === 2 && + Array.isArray(reference.resolved.identifiers) && + // Is second tuple value the same reference we're checking? + id.elements[1] === reference.resolved.identifiers[0] + ) { + return true; + } } + // By default assume it's dynamic. + return false; } export default { @@ -162,6 +158,7 @@ export default { } // Get dependencies from all our resolved references in pure scopes. + // Key is dependency string, value is whether it's static. const dependencies = new Map(); gatherDependenciesRecursively(scope); @@ -183,54 +180,13 @@ export default { reference.identifier, ); const dependencyNode = getDependency(referenceNode); - const dependency = normalizeDependencyNode(dependencyNode); + const dependency = toPropertyAccessString(dependencyNode); + // Add the dependency to a map so we can make sure it is referenced - // again in our dependencies array. - let info = dependencies.get(dependency); - if (!info) { - info = {isKnownToBeStatic: false}; - dependencies.set(dependency, info); - - if ( - reference.resolved != null && - Array.isArray(reference.resolved.defs) - ) { - const def = reference.resolved.defs[0]; - if (def != null && def.node.init != null) { - const init = def.node.init; - if (init.callee != null) { - let callee = init.callee; - if ( - callee.type === 'MemberExpression' && - callee.object.name === 'React' && - callee.property != null - ) { - callee = callee.property; - } - if (callee.type === 'Identifier') { - if ( - callee.name === 'useRef' && - def.node.id.type === 'Identifier' - ) { - info.isKnownToBeStatic = true; - } else if ( - callee.name === 'useState' || - callee.name === 'useReducer' - ) { - if ( - def.node.id.type === 'ArrayPattern' && - def.node.id.elements.length === 2 && - Array.isArray(reference.resolved.identifiers) && - def.node.id.elements[1] === - reference.resolved.identifiers[0] - ) { - info.isKnownToBeStatic = true; - } - } - } - } - } - } + // again in our dependencies array. Remember whether it's static. + if (!dependencies.has(dependency)) { + const isStatic = isDefinitelyStaticDependency(reference); + dependencies.set(dependency, isStatic); } } for (const childScope of currentScope.childScopes) { @@ -272,11 +228,9 @@ export default { // will be thrown. We will catch that error and report an error. let declaredDependency; try { - declaredDependency = normalizeDependencyNode( - declaredDependencyNode, - ); + declaredDependency = toPropertyAccessString(declaredDependencyNode); } catch (error) { - if (/Unexpected node type/.test(error.message)) { + if (/Unsupported node type/.test(error.message)) { context.report({ node: declaredDependencyNode, message: @@ -297,19 +251,18 @@ export default { }); } + // TODO: we can do a pass at this code and pick more appropriate + // data structures to avoid nested loops if we can. let suggestedDependencies = []; - let duplicateDependencies = new Set(); let unnecessaryDependencies = new Set(); let missingDependencies = new Set(); - let actualDependencies = Array.from(dependencies.keys()); function satisfies(actualDep, dep) { return actualDep === dep || actualDep.startsWith(dep + '.'); } - // TODO: this could use some refactoring and optimizations. // First, ensure what user specified makes sense. declaredDependencies.forEach(({key}) => { if (actualDependencies.some(actualDep => satisfies(actualDep, key))) { @@ -325,14 +278,15 @@ export default { unnecessaryDependencies.add(key); } }); + // Then fill in the missing ones. - dependencies.forEach((info, key) => { + dependencies.forEach((isStatic, key) => { if ( !suggestedDependencies.some(suggestedDep => satisfies(key, suggestedDep), ) ) { - if (!info.isKnownToBeStatic) { + if (!isStatic) { // Legit missing. suggestedDependencies.push(key); missingDependencies.add(key); @@ -365,41 +319,27 @@ export default { return; } - const quote = name => "'" + name + "'"; - const join = (arr, forceComma) => { - let s = ''; - for (let i = 0; i < arr.length; i++) { - s += arr[i]; - if (i === 0 && arr.length === 2) { - s += ' and '; - } else if (i === arr.length - 2 && arr.length > 2) { - s += ', and '; - } else if (i < arr.length - 1) { - s += ', '; - } - } - return s; - }; - const list = (set, singlePrefix, label, fixVerb) => { - if (set.size === 0) { + function getWarningMessage(deps, singlePrefix, label, fixVerb) { + if (deps.size === 0) { return null; } return ( - (set.size > 1 ? '' : singlePrefix + ' ') + + (deps.size > 1 ? '' : singlePrefix + ' ') + label + ' ' + - (set.size > 1 ? 'dependencies' : 'dependency') + + (deps.size > 1 ? 'dependencies' : 'dependency') + ': ' + - join( - Array.from(set) + joinEnglish( + Array.from(deps) .sort() - .map(quote), + .map(name => "'" + name + "'"), ) + `. Either ${fixVerb} ${ - set.size > 1 ? 'them' : 'it' + deps.size > 1 ? 'them' : 'it' } or remove the dependency array.` ); - }; + } + let extraWarning = ''; if (unnecessaryDependencies.size > 0) { let badRef = null; @@ -423,12 +363,22 @@ export default { message: `React Hook ${context.getSource(reactiveHook)} has ` + // To avoid a long message, show the next actionable item. - (list(missingDependencies, 'a', 'missing', 'include') || - list(unnecessaryDependencies, 'an', 'unnecessary', 'exclude') || - list(duplicateDependencies, 'a', 'duplicate', 'omit')) + + (getWarningMessage(missingDependencies, 'a', 'missing', 'include') || + getWarningMessage( + unnecessaryDependencies, + 'an', + 'unnecessary', + 'exclude', + ) || + getWarningMessage( + duplicateDependencies, + 'a', + 'duplicate', + 'omit', + )) + extraWarning, fix(fixer) { - // TODO: consider keeping the comments? + // TODO: consider preserving the comments or formatting? return fixer.replaceText( declaredDependenciesNode, `[${suggestedDependencies.join(', ')}]`, @@ -440,12 +390,10 @@ export default { }; /** - * Gets a dependency for our reactive callback from an identifier. If the - * identifier is the object part of a member expression then we use the entire - * member expression as a dependency. - * - * For instance, if we get `props` in `props.foo` then our dependency should be - * the full member expression. + * Assuming () means the passed/returned node: + * (props) => (props) + * props.(foo) => (props.foo) + * props.foo.(bar) => (props.foo).bar */ function getDependency(node) { if ( @@ -466,20 +414,72 @@ function getDependency(node) { } /** - * Normalizes a dependency into a standard string representation which can - * easily be compared. - * - * Throws an error if the node type is not a valid dependency. + * Assuming () means the passed node. + * (foo) -> 'foo' + * foo.(bar) -> 'foo.bar' + * foo.bar.(baz) -> 'foo.bar.baz' + * Otherwise throw. */ -function normalizeDependencyNode(node) { +function toPropertyAccessString(node) { if (node.type === 'Identifier') { return node.name; } else if (node.type === 'MemberExpression' && !node.computed) { - const object = normalizeDependencyNode(node.object); - const property = normalizeDependencyNode(node.property); + const object = toPropertyAccessString(node.object); + const property = toPropertyAccessString(node.property); return `${object}.${property}`; } else { - throw new Error(`Unexpected node type: ${node.type}`); + throw new Error(`Unsupported node type: ${node.type}`); + } +} + +// What's the index of callback that needs to be analyzed for a given Hook? +// -1 if it's not a Hook we care about (e.g. useState). +// 0 for useEffect/useMemo/useCallback(fn). +// 1 for useImperativeHandle(ref, fn). +// For additionally configured Hooks, assume that they're like useEffect (0). +function getReactiveHookCallbackIndex(node, options) { + let isOnReactObject = false; + if ( + node.type === 'MemberExpression' && + node.object.type === 'Identifier' && + node.object.name === 'React' && + node.property.type === 'Identifier' && + !node.computed + ) { + node = node.property; + isOnReactObject = true; + } + if (node.type !== 'Identifier') { + return; + } + switch (node.name) { + case 'useEffect': + case 'useLayoutEffect': + case 'useCallback': + case 'useMemo': + // useEffect(fn) + return 0; + case 'useImperativeHandle': + // useImperativeHandle(ref, fn) + return 1; + default: + if (!isOnReactObject && options && options.additionalHooks) { + // Allow the user to provide a regular expression which enables the lint to + // target custom reactive hooks. + let name; + try { + name = toPropertyAccessString(node); + } catch (error) { + if (/Unsupported node type/.test(error.message)) { + return 0; + } else { + throw error; + } + } + return options.additionalHooks.test(name) ? 0 : -1; + } else { + return -1; + } } } @@ -529,6 +529,21 @@ function fastFindReferenceWithParent(start, target) { return null; } +function joinEnglish(arr) { + let s = ''; + for (let i = 0; i < arr.length; i++) { + s += arr[i]; + if (i === 0 && arr.length === 2) { + s += ' and '; + } else if (i === arr.length - 2 && arr.length > 2) { + s += ', and '; + } else if (i < arr.length - 1) { + s += ', '; + } + } + return s; +} + function isNodeLike(val) { return ( typeof val === 'object' &&