From f2d74d311ed8575edac1f8711aebc41fc5fd08b4 Mon Sep 17 00:00:00 2001 From: Caleb Meredith Date: Wed, 31 Oct 2018 18:53:33 +0000 Subject: [PATCH 1/9] 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 00000000000..e5113ae4778 --- /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 00000000000..5bd79115320 --- /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 a3d6216e097..e828c8bca70 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 41fec63d05b57a2caadcb7cc5bda6cdfa2a8c83d Mon Sep 17 00:00:00 2001 From: Jamie Kyle Date: Wed, 31 Oct 2018 13:59:45 -0700 Subject: [PATCH 2/9] 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 5bd79115320..d04763f67a9 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 f227df89002eb41ed09011e0d4320ac88bd1749e Mon Sep 17 00:00:00 2001 From: Jamie Kyle Date: Wed, 31 Oct 2018 15:22:14 -0700 Subject: [PATCH 3/9] 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 d04763f67a9..63493b356f4 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 5d5b180b5e426a4f4ae3cb1007d38a4e079b92b7 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 10 Jan 2019 19:47:05 +0000 Subject: [PATCH 4/9] 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 63493b356f4..a3662fca76b 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 816b8ae4f58afd2bca368bb8b75cb70ddff21af8 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 10 Jan 2019 20:04:59 +0000 Subject: [PATCH 5/9] 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 e5113ae4778..fb66b55fe99 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 e7f44f112a8bc68a073f9227b48f5e26b1736de2 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 11 Jan 2019 14:35:44 +0000 Subject: [PATCH 6/9] 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 a3662fca76b..728c6fb501f 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 c1ab460d9e01e8b7d02372e826a5c07389647440 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 11 Jan 2019 15:11:25 +0000 Subject: [PATCH 7/9] 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 728c6fb501f..b3c3a220874 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 902cb08b48182a4e12df3e84b0cef0bfac2d90a4 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 11 Jan 2019 18:26:57 +0000 Subject: [PATCH 8/9] 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 fb66b55fe99..7ec8c90c17c 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 b3c3a220874..6da40cce853 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 8797fa6f55c99f33b0bd312a5e87d25e8b3ef450 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 17 Jan 2019 01:22:23 +0000 Subject: [PATCH 9/9] 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 ++-- scripts/rollup/results.json | 4 ++-- 5 files changed, 7 insertions(+), 7 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 982d6c8eb65..0c97574cfee 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 7ec8c90c17c..e374059b01d 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 e828c8bca70..f7e5c6fa2eb 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, }; diff --git a/scripts/rollup/results.json b/scripts/rollup/results.json index 91d4f827bac..5b4047e4737 100644 --- a/scripts/rollup/results.json +++ b/scripts/rollup/results.json @@ -928,8 +928,8 @@ "filename": "eslint-plugin-react-hooks.development.js", "bundleType": "NODE_DEV", "packageName": "eslint-plugin-react-hooks", - "size": 25592, - "gzip": 5885 + "size": 44512, + "gzip": 9733 }, { "filename": "eslint-plugin-react-hooks.production.min.js",