From 75ad1f528e0c093131983b699646acfd3835de63 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 30 Mar 2020 17:37:30 +0100 Subject: [PATCH 1/3] Refactor: visit CallExpression Instead of visiting the functions and looking up to see if they're in a Hook call, visit Hook calls and look down to see if there's a callback inside. I will need this refactor so I can visit functions declared outside the call. --- .../src/ExhaustiveDeps.js | 58 ++++++++++--------- 1 file changed, 32 insertions(+), 26 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index b30694014e25e..c66216c424acd 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -33,6 +33,8 @@ export default { : undefined; const options = {additionalHooks}; + const scopeManager = context.getSourceCode().scopeManager; + // Should be shared between visitors. let setStateCallSites = new WeakMap(); let stateVariables = new WeakSet(); @@ -52,41 +54,45 @@ export default { } return { - FunctionExpression: visitFunctionExpression, - ArrowFunctionExpression: visitFunctionExpression, + CallExpression: visitCallExpression, }; - /** - * Visitor for both function expressions and arrow function expressions. - */ - function visitFunctionExpression(node) { - // We only want to lint nodes which are reactive hook callbacks. - if ( - (node.type !== 'FunctionExpression' && - node.type !== 'ArrowFunctionExpression') || - node.parent.type !== 'CallExpression' - ) { + function visitCallExpression(node) { + const callbackIndex = getReactiveHookCallbackIndex(node.callee, options); + if (callbackIndex === -1) { + // Not a React Hook call that needs deps. return; } - - const callbackIndex = getReactiveHookCallbackIndex( - node.parent.callee, - options, - ); - if (node.parent.arguments[callbackIndex] !== node) { - return; + const callback = node.arguments[callbackIndex]; + const reactiveHook = node.callee; + const declaredDependenciesNode = node.arguments[callbackIndex + 1]; + switch (callback.type) { + case 'FunctionExpression': + case 'ArrowFunctionExpression': + visitFunctionWithDependencies( + callback, + declaredDependenciesNode, + reactiveHook, + ); + break; } + } + /** + * Visitor for both function expressions and arrow function expressions. + */ + function visitFunctionWithDependencies( + node, + declaredDependenciesNode, + reactiveHook, + ) { // Get the reactive hook node. - const reactiveHook = node.parent.callee; const reactiveHookName = getNodeWithoutReactNamespace(reactiveHook).name; const isEffect = /Effect($|[^a-z])/g.test(reactiveHookName); // 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 depsIndex = callbackIndex + 1; - const declaredDependenciesNode = node.parent.arguments[depsIndex]; if (!declaredDependenciesNode && !isEffect) { // These are only used for optimization. if ( @@ -95,7 +101,7 @@ export default { ) { // TODO: Can this have a suggestion? context.report({ - node: node.parent.callee, + node: reactiveHook, message: `React Hook ${reactiveHookName} does nothing when called with ` + `only one argument. Did you forget to pass an array of ` + @@ -124,7 +130,7 @@ export default { } // Get the current scope. - const scope = context.getScope(); + const scope = scopeManager.acquire(node); // 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 @@ -550,7 +556,7 @@ export default { isEffect: true, }); context.report({ - node: node.parent.callee, + node: reactiveHook, message: `React Hook ${reactiveHookName} contains a call to '${setStateInsideEffectWithoutDeps}'. ` + `Without a list of dependencies, this can lead to an infinite chain of updates. ` + @@ -1341,7 +1347,7 @@ function getNodeWithoutReactNamespace(node, options) { function getReactiveHookCallbackIndex(calleeNode, options) { let node = getNodeWithoutReactNamespace(calleeNode); if (node.type !== 'Identifier') { - return null; + return -1; } switch (node.name) { case 'useEffect': From c7251b01878e087494e2baeaa2815c0be50e0d16 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 30 Mar 2020 19:18:06 +0100 Subject: [PATCH 2/3] Check deps when callback body is outside the Hook call --- .../ESLintRuleExhaustiveDeps-test.js | 320 ++++++++++++++++++ .../src/ExhaustiveDeps.js | 86 ++++- 2 files changed, 403 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index d45728aedb0fc..92739e679f672 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -259,6 +259,104 @@ const tests = { } `, }, + { + code: normalizeIndent` + function MyComponent() { + const myEffect = () => { + // Doesn't use anything + }; + useEffect(myEffect, []); + } + `, + }, + { + code: normalizeIndent` + const local = {}; + function MyComponent() { + const myEffect = () => { + console.log(local); + }; + useEffect(myEffect, []); + } + `, + }, + { + code: normalizeIndent` + const local = {}; + function MyComponent() { + function myEffect() { + console.log(local); + } + useEffect(myEffect, []); + } + `, + }, + { + code: normalizeIndent` + function MyComponent() { + const local = {}; + function myEffect() { + console.log(local); + } + useEffect(myEffect, [local]); + } + `, + }, + { + code: normalizeIndent` + function MyComponent() { + function myEffect() { + console.log(global); + } + useEffect(myEffect, []); + } + `, + }, + { + code: normalizeIndent` + const local = {}; + function MyComponent() { + const myEffect = () => { + otherThing() + } + const otherThing = () => { + console.log(local); + } + useEffect(myEffect, []); + } + `, + }, + { + // Valid because even though we don't inspect the function itself, + // at least it's passed as a dependency. + code: normalizeIndent` + function MyComponent({delay}) { + const local = {}; + const myEffect = debounce(() => { + console.log(local); + }, delay); + useEffect(myEffect, [myEffect]); + } + `, + }, + { + code: normalizeIndent` + let local = {}; + function myEffect() { + console.log(local); + } + function MyComponent() { + useEffect(myEffect, []); + } + `, + }, + { + code: normalizeIndent` + function MyComponent({myEffect}) { + useEffect(myEffect, [myEffect]); + } + `, + }, { code: normalizeIndent` function MyComponent(props) { @@ -5998,6 +6096,228 @@ const tests = { }, ], }, + { + code: normalizeIndent` + function MyComponent() { + const local = {}; + function myEffect() { + console.log(local); + } + useEffect(myEffect, []); + } + `, + errors: [ + { + message: + "React Hook useEffect has a missing dependency: 'local'. " + + 'Either include it or remove the dependency array.', + suggestions: [ + { + desc: 'Update the dependencies array to be: [local]', + output: normalizeIndent` + function MyComponent() { + const local = {}; + function myEffect() { + console.log(local); + } + useEffect(myEffect, [local]); + } + `, + }, + ], + }, + ], + }, + { + code: normalizeIndent` + function MyComponent() { + const local = {}; + const myEffect = () => { + console.log(local); + }; + useEffect(myEffect, []); + } + `, + errors: [ + { + message: + "React Hook useEffect has a missing dependency: 'local'. " + + 'Either include it or remove the dependency array.', + suggestions: [ + { + desc: 'Update the dependencies array to be: [local]', + output: normalizeIndent` + function MyComponent() { + const local = {}; + const myEffect = () => { + console.log(local); + }; + useEffect(myEffect, [local]); + } + `, + }, + ], + }, + ], + }, + { + code: normalizeIndent` + function MyComponent() { + const local = {}; + const myEffect = function() { + console.log(local); + }; + useEffect(myEffect, []); + } + `, + errors: [ + { + message: + "React Hook useEffect has a missing dependency: 'local'. " + + 'Either include it or remove the dependency array.', + suggestions: [ + { + desc: 'Update the dependencies array to be: [local]', + output: normalizeIndent` + function MyComponent() { + const local = {}; + const myEffect = function() { + console.log(local); + }; + useEffect(myEffect, [local]); + } + `, + }, + ], + }, + ], + }, + { + code: normalizeIndent` + function MyComponent() { + const local = {}; + const myEffect = () => { + otherThing(); + }; + const otherThing = () => { + console.log(local); + }; + useEffect(myEffect, []); + } + `, + errors: [ + { + message: + "React Hook useEffect has a missing dependency: 'otherThing'. " + + 'Either include it or remove the dependency array.', + suggestions: [ + { + desc: 'Update the dependencies array to be: [otherThing]', + output: normalizeIndent` + function MyComponent() { + const local = {}; + const myEffect = () => { + otherThing(); + }; + const otherThing = () => { + console.log(local); + }; + useEffect(myEffect, [otherThing]); + } + `, + }, + ], + }, + ], + }, + { + code: normalizeIndent` + function MyComponent() { + const local = {}; + const myEffect = debounce(() => { + console.log(local); + }, delay); + useEffect(myEffect, []); + } + `, + errors: [ + { + message: + "React Hook useEffect has a missing dependency: 'myEffect'. " + + 'Either include it or remove the dependency array.', + suggestions: [ + { + desc: 'Update the dependencies array to be: [myEffect]', + output: normalizeIndent` + function MyComponent() { + const local = {}; + const myEffect = debounce(() => { + console.log(local); + }, delay); + useEffect(myEffect, [myEffect]); + } + `, + }, + ], + }, + ], + }, + { + code: normalizeIndent` + function MyComponent() { + const local = {}; + const myEffect = debounce(() => { + console.log(local); + }, delay); + useEffect(myEffect, [local]); + } + `, + errors: [ + { + message: + "React Hook useEffect has a missing dependency: 'myEffect'. " + + 'Either include it or remove the dependency array.', + suggestions: [ + { + desc: 'Update the dependencies array to be: [myEffect]', + output: normalizeIndent` + function MyComponent() { + const local = {}; + const myEffect = debounce(() => { + console.log(local); + }, delay); + useEffect(myEffect, [myEffect]); + } + `, + }, + ], + }, + ], + }, + { + code: normalizeIndent` + function MyComponent({myEffect}) { + useEffect(myEffect, []); + } + `, + errors: [ + { + message: + "React Hook useEffect has a missing dependency: 'myEffect'. " + + 'Either include it or remove the dependency array.', + suggestions: [ + { + desc: 'Update the dependencies array to be: [myEffect]', + output: normalizeIndent` + function MyComponent({myEffect}) { + useEffect(myEffect, [myEffect]); + } + `, + }, + ], + }, + ], + }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index c66216c424acd..cd3592ddff2cd 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -66,6 +66,7 @@ export default { const callback = node.arguments[callbackIndex]; const reactiveHook = node.callee; const declaredDependenciesNode = node.arguments[callbackIndex + 1]; + switch (callback.type) { case 'FunctionExpression': case 'ArrowFunctionExpression': @@ -74,8 +75,88 @@ export default { declaredDependenciesNode, reactiveHook, ); - break; + return; // Handled + case 'Identifier': + // The function passed as a callback is not written inline. + // But perhaps it's in the dependencies array? + if ( + declaredDependenciesNode && + declaredDependenciesNode.elements && + declaredDependenciesNode.elements.some( + el => el.type === 'Identifier' && el.name === callback.name, + ) + ) { + // If it's already in the list of deps, we don't care because + // this is valid regardless. + return; // Handled + } + // We'll do our best effort to find it, complain otherwise. + const variable = context.getScope().set.get(callback.name); + if (variable == null || variable.defs == null) { + // If it's not in scope, we don't care. + return; // Handled + } + // The function passed as a callback is not written inline. + // But it's defined somewhere in the render scope. + // We'll do our best effort to find and check it, complain otherwise. + const def = variable.defs[0]; + if (!def || !def.node) { + break; // Unhandled + } + if (def.type !== 'Variable' && def.type !== 'FunctionName') { + // Parameter or an unusual pattern. Bail out. + break; // Unhandled + } + switch (def.node.type) { + case 'FunctionDeclaration': + // useEffect(() => { ... }, []); + visitFunctionWithDependencies( + def.node, + declaredDependenciesNode, + reactiveHook, + ); + return; // Handled + case 'VariableDeclarator': + const init = def.node.init; + if (!init) { + break; // Unhandled + } + switch (init.type) { + // const effectBody = () => {...}; + // useEffect(effectBody, []); + case 'ArrowFunctionExpression': + case 'FunctionExpression': + // We can inspect this function as if it were inline. + visitFunctionWithDependencies( + init, + declaredDependenciesNode, + reactiveHook, + ); + return; // Handled + } + break; // Unhandled + } + break; // Unhandled } + // Something unusual. Fall back to suggesting to add the body itself as a dep. + const reactiveHookName = getNodeWithoutReactNamespace(reactiveHook).name; + context.report({ + node: reactiveHook, + message: + `React Hook ${reactiveHookName} has a missing dependency: '${callback.name}'. ` + + `Either include it or remove the dependency array.`, + suggest: [ + { + desc: `Update the dependencies array to be: [${callback.name}]`, + fix(fixer) { + return fixer.replaceText( + declaredDependenciesNode, + `[${callback.name}]`, + ); + }, + }, + ], + }); } /** @@ -86,11 +167,10 @@ export default { declaredDependenciesNode, reactiveHook, ) { - // Get the reactive hook node. const reactiveHookName = getNodeWithoutReactNamespace(reactiveHook).name; const isEffect = /Effect($|[^a-z])/g.test(reactiveHookName); - // Get the declared dependencies for this reactive hook. If there is no + // Check 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. if (!declaredDependenciesNode && !isEffect) { From 4bd43396b359b30bd6da220e92a584672a22b3c6 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 31 Mar 2020 01:29:48 +0100 Subject: [PATCH 3/3] Handle the unknown case --- .../__tests__/ESLintRuleExhaustiveDeps-test.js | 18 ++++++++++++++++++ .../src/ExhaustiveDeps.js | 12 +++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 92739e679f672..7c4639607b2e4 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -6318,6 +6318,24 @@ const tests = { }, ], }, + { + code: normalizeIndent` + function MyComponent() { + const local = {}; + useEffect(debounce(() => { + console.log(local); + }, delay), []); + } + `, + errors: [ + { + message: + 'React Hook useEffect received a function whose dependencies ' + + 'are unknown. Pass an inline function instead.', + suggestions: [], + }, + ], + }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index cd3592ddff2cd..4482d119b9853 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -65,6 +65,7 @@ export default { } const callback = node.arguments[callbackIndex]; const reactiveHook = node.callee; + const reactiveHookName = getNodeWithoutReactNamespace(reactiveHook).name; const declaredDependenciesNode = node.arguments[callbackIndex + 1]; switch (callback.type) { @@ -137,9 +138,18 @@ export default { break; // Unhandled } break; // Unhandled + default: + // useEffect(generateEffectBody(), []); + context.report({ + node: reactiveHook, + message: + `React Hook ${reactiveHookName} received a function whose dependencies ` + + `are unknown. Pass an inline function instead.`, + }); + return; // Handled } + // Something unusual. Fall back to suggesting to add the body itself as a dep. - const reactiveHookName = getNodeWithoutReactNamespace(reactiveHook).name; context.report({ node: reactiveHook, message: