From bc363706cab6bd1c7111cbd65bc67655a041f1b9 Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Tue, 11 Aug 2020 16:57:37 -0700 Subject: [PATCH 1/9] [eslint-plugin-react-cooks] Report constant constructions The dependency array passed to a React hook can be thought of as a list of cache keys. On each render, if any dependency is not `===` its previous value, the hook will be rerun. Constructing a new object/array/function/etc directly within your render function means that the value will be referentially unique on each render. If you then use that value as a hook dependency, that hook will get a "cache miss" on every render, making the dependency array useless. This can be especially dangerous since it can cascade. If a hook such as `useMemo` is rerun on each render, not only are we bypassing the option to avoid potentially expensive work, but the value _returned_ by `useMemo` may end up being referentially unique on each render causing other downstream hooks or memoized components to become deoptimized. --- .../ESLintRuleExhaustiveDeps-test.js | 401 ++++++++++++++++++ .../src/ExhaustiveDeps.js | 197 ++++++--- 2 files changed, 529 insertions(+), 69 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index de6d0aa4f7e07..227bcf70ffd02 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -1376,6 +1376,72 @@ const tests = { } `, }, + { + code: normalizeIndent` + function useFoo(foo){ + return useMemo(() => foo, [foo]); + } + `, + }, + { + code: normalizeIndent` + function useFoo(){ + const foo = "hi!"; + return useMemo(() => foo, [foo]); + } + `, + }, + { + code: normalizeIndent` + function useFoo(){ + const foo = new MyObj(); + return useMemo(() => foo, [foo]); + } + `, + }, + { + code: normalizeIndent` + function useFoo(){ + let foo = {}; + foo = 1; + return useMemo(() => foo, [foo]); + } + `, + }, + { + code: normalizeIndent` + function useFoo(){ + let {foo} = {foo: 1}; + return useMemo(() => foo, [foo]); + } + `, + }, + { + code: normalizeIndent` + function useFoo(){ + let [foo] = [1]; + return useMemo(() => foo, [foo]); + } + `, + }, + { + code: normalizeIndent` + function useFoo() { + const foo = "fine"; + if(true) { + const foo = {}; + } + return useMemo(() => foo, [foo]); + } + `, + }, + { + code: normalizeIndent` + function MyComponent({foo}) { + return useMemo(() => foo, [foo]) + } + `, + }, ], invalid: [ { @@ -6967,6 +7033,341 @@ const tests = { }, ], }, + { + code: normalizeIndent` + function Component() { + const foo = {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = []; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' array makes the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = () => {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' function makes the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + 'useCallback() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = function bar(){}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' function makes the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + 'useCallback() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = class {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' class makes the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = true ? {} : "fine"; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' conditional could make the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = bar || {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = bar ?? {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = bar && {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = bar ? baz ? {} : null : null; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' conditional could make the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + let foo = {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + var foo = {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = {}; + useCallback(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useCallback Hook (at line 6) change on every render. " + + "Move it inside the useCallback callback. Alternatively, wrap the 'foo' definition into its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = {}; + useEffect(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useEffect Hook (at line 6) change on every render. " + + "Move it inside the useEffect callback. Alternatively, wrap the 'foo' definition into its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = {}; + useLayoutEffect(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useLayoutEffect Hook (at line 6) change on every render. " + + "Move it inside the useLayoutEffect callback. Alternatively, wrap the 'foo' definition into its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = {}; + useImperativeHandle( + ref, + () => { + console.log(foo); + }, + [foo] + ); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useImperativeHandle Hook (at line 9) change on every render. " + + "Move it inside the useImperativeHandle callback. Alternatively, wrap the 'foo' definition into its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo(section) { + const foo = section.section_components?.edges ?? []; + useEffect(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' logical expression could make the dependencies of useEffect Hook (at line 6) change on every render. " + + "Move it inside the useEffect callback. Alternatively, wrap the 'foo' definition into its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo(section) { + const foo = {}; + console.log(foo); + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useMemo Hook (at line 7) change on every render. " + + "To fix this, wrap the 'foo' definition into its own useMemo() Hook.", + suggestions: [ + { + desc: "Wrap the 'foo' definition into its own useMemo() Hook.", + output: normalizeIndent` + function Foo(section) { + const foo = useMemo(() => { return {}; }); + console.log(foo); + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + }, + ], + }, + ], + }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 9e344e8662d6d..a8204c5426fa9 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -844,59 +844,70 @@ export default { unnecessaryDependencies.size; if (problemCount === 0) { - // If nothing else to report, check if some callbacks - // are bare and would invalidate on every render. - const bareFunctions = scanForDeclaredBareFunctions({ + // If nothing else to report, check if some dependencies would + // invalidate on every render. + const constructions = scanForConstructions({ declaredDependencies, declaredDependenciesNode, componentScope, scope, }); - bareFunctions.forEach(({fn, suggestUseCallback}) => { - let message = - `The '${fn.name.name}' function makes the dependencies of ` + - `${reactiveHookName} Hook (at line ${declaredDependenciesNode.loc.start.line}) ` + - `change on every render.`; - if (suggestUseCallback) { - message += - ` To fix this, ` + - `wrap the '${fn.name.name}' definition into its own useCallback() Hook.`; - } else { - message += - ` Move it inside the ${reactiveHookName} callback. ` + - `Alternatively, wrap the '${fn.name.name}' definition into its own useCallback() Hook.`; - } - - let suggest; - // Only handle the simple case: arrow functions. - // Wrapping function declarations can mess up hoisting. - if (suggestUseCallback && fn.type === 'Variable') { - suggest = [ - { - desc: `Wrap the '${fn.name.name}' definition into its own useCallback() Hook.`, - fix(fixer) { - return [ - // TODO: also add an import? - fixer.insertTextBefore(fn.node.init, 'useCallback('), - // TODO: ideally we'd gather deps here but it would require - // restructuring the rule code. This will cause a new lint - // error to appear immediately for useCallback. Note we're - // not adding [] because would that changes semantics. - fixer.insertTextAfter(fn.node.init, ')'), - ]; + constructions.forEach( + ({construction, isUsedOutsideOfHook, depType}) => { + const wrapperHook = + depType === 'function' ? 'useCallback' : 'useMemo'; + + const defaultAdvice = `wrap the '${construction.name.name}' definition into its own ${wrapperHook}() Hook.`; + + const advice = isUsedOutsideOfHook + ? `To fix this, ${defaultAdvice}` + : `Move it inside the ${reactiveHookName} callback. Alternatively, ${defaultAdvice}`; + + const causation = + depType === 'conditional' || depType === 'logical expression' + ? 'could make' + : 'makes'; + + const message = + `The '${construction.name.name}' ${depType} ${causation} the dependencies of ` + + `${reactiveHookName} Hook (at line ${declaredDependenciesNode.loc.start.line}) ` + + `change on every render. ${advice}`; + + let suggest; + // Only handle the simple case of variable assignments. + // Wrapping function declarations can mess up hoisting. + if (isUsedOutsideOfHook && construction.type === 'Variable') { + suggest = [ + { + desc: `Wrap the '${construction.name.name}' definition into its own ${wrapperHook}() Hook.`, + fix(fixer) { + const [before, after] = + wrapperHook === 'useMemo' + ? [`useMemo(() => { return `, '; })'] + : ['useCallback(', ')']; + return [ + // TODO: also add an import? + fixer.insertTextBefore(construction.node.init, before), + // TODO: ideally we'd gather deps here but it would require + // restructuring the rule code. This will cause a new lint + // error to appear immediately for useCallback. Note we're + // not adding [] because would that changes semantics. + fixer.insertTextAfter(construction.node.init, after), + ]; + }, }, - }, - ]; - } - // TODO: What if the function needs to change on every render anyway? - // Should we suggest removing effect deps as an appropriate fix too? - reportProblem({ - // TODO: Why not report this at the dependency site? - node: fn.node, - message, - suggest, - }); - }); + ]; + } + // TODO: What if the function needs to change on every render anyway? + // Should we suggest removing effect deps as an appropriate fix too? + reportProblem({ + // TODO: Why not report this at the dependency site? + node: construction.node, + message, + suggest, + }); + }, + ); return; } @@ -1381,50 +1392,97 @@ function collectRecommendations({ }; } -// Finds functions declared as dependencies +// If the node will result in constructing a referentially unique value, return +// its human readable type name, else return null. +function getConstructionExpresionType(node) { + switch (node.type) { + case 'ObjectExpression': + return 'object'; + case 'ArrayExpression': + return 'array'; + case 'ArrowFunctionExpression': + case 'FunctionExpression': + return 'function'; + case 'ClassExpression': + return 'class'; + case 'ConditionalExpression': + if ( + getConstructionExpresionType(node.consequent) != null || + getConstructionExpresionType(node.alternate) != null + ) { + return 'conditional'; + } + return null; + case 'LogicalExpression': + if ( + getConstructionExpresionType(node.left) != null || + getConstructionExpresionType(node.right) != null + ) { + return 'logical expression'; + } + return null; + } + return null; +} + +// Finds variables declared as dependencies // that would invalidate on every render. -function scanForDeclaredBareFunctions({ +function scanForConstructions({ declaredDependencies, declaredDependenciesNode, componentScope, scope, }) { - const bareFunctions = declaredDependencies + const constructions = declaredDependencies .map(({key}) => { - const fnRef = componentScope.set.get(key); - if (fnRef == null) { + const ref = componentScope.variables.find(v => v.name === key); + if (ref == null) { + return null; + } + + if (ref.references.some(r => !r.init && r.isWrite())) { + // The variable gets reassigned. This complicates things so we won't + // try to reason about it for now. return null; } - const fnNode = fnRef.defs[0]; - if (fnNode == null) { + + const node = ref.defs[0]; + if (node == null) { return null; } // const handleChange = function () {} // const handleChange = () => {} + // const foo = {} + // const foo = [] + // etc. if ( - fnNode.type === 'Variable' && - fnNode.node.type === 'VariableDeclarator' && - fnNode.node.init != null && - (fnNode.node.init.type === 'ArrowFunctionExpression' || - fnNode.node.init.type === 'FunctionExpression') + node.type === 'Variable' && + node.node.type === 'VariableDeclarator' && + node.node.id.type === 'Identifier' && // Ensure this is not destructed assignment + node.node.init != null ) { - return fnRef; + const constantExpressionType = getConstructionExpresionType( + node.node.init, + ); + if (constantExpressionType != null) { + return [ref, constantExpressionType]; + } } // function handleChange() {} if ( - fnNode.type === 'FunctionName' && - fnNode.node.type === 'FunctionDeclaration' + node.type === 'FunctionName' && + node.node.type === 'FunctionDeclaration' ) { - return fnRef; + return [ref, 'function']; } return null; }) .filter(Boolean); - function isUsedOutsideOfHook(fnRef) { + function isUsedOutsideOfHook(ref) { let foundWriteExpr = false; - for (let i = 0; i < fnRef.references.length; i++) { - const reference = fnRef.references[i]; + for (let i = 0; i < ref.references.length; i++) { + const reference = ref.references[i]; if (reference.writeExpr) { if (foundWriteExpr) { // Two writes to the same function. @@ -1450,9 +1508,10 @@ function scanForDeclaredBareFunctions({ return false; } - return bareFunctions.map(fnRef => ({ - fn: fnRef.defs[0], - suggestUseCallback: isUsedOutsideOfHook(fnRef), + return constructions.map(([ref, depType]) => ({ + construction: ref.defs[0], + depType, + isUsedOutsideOfHook: isUsedOutsideOfHook(ref), })); } From 450cdfd410619b6a10d64d009a1f12b3be5014d9 Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Tue, 11 Aug 2020 18:02:30 -0700 Subject: [PATCH 2/9] Fix/remove existing tests --- .../ESLintRuleExhaustiveDeps-test.js | 94 +++++-------------- 1 file changed, 22 insertions(+), 72 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 227bcf70ffd02..06b6d0df20fa4 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -56,7 +56,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = useMemo(() => ({}), []); useEffect(() => { console.log(local); }, [local]); @@ -94,9 +94,9 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = useMemo(() => ({}), []); { - const local2 = {}; + const local2 = useMemo(() => ({}), []); useCallback(() => { console.log(local1); console.log(local2); @@ -108,9 +108,9 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = useMemo(() => ({}), []); function MyNestedComponent() { - const local2 = {}; + const local2 = useMemo(() => ({}), []); useCallback(() => { console.log(local1); console.log(local2); @@ -122,7 +122,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = useMemo(() => ({}), []); useEffect(() => { console.log(local); console.log(local); @@ -142,7 +142,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = useMemo(() => ({}), []); useEffect(() => { console.log(local); }, [,,,local,,,]); @@ -222,7 +222,7 @@ const tests = { { code: normalizeIndent` function MyComponent(props) { - const local = {}; + const local = useMemo(() => ({}), []); useEffect(() => { console.log(props.foo); console.log(props.bar); @@ -243,7 +243,7 @@ const tests = { console.log(props.bar); }, [props, props.foo]); - let color = {} + let color = useMemo(() => ({}), []); useEffect(() => { console.log(props.foo.bar.baz); console.log(color); @@ -416,7 +416,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = useMemo(() => ({}), []); function myEffect() { console.log(local); } @@ -731,7 +731,7 @@ const tests = { // direct assignments. code: normalizeIndent` function MyComponent(props) { - let obj = {}; + let obj = useMemo(() => ({}), []); useEffect(() => { obj.foo = true; }, [obj]); @@ -1560,7 +1560,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = useMemo(() => ({}), []); useEffect(() => { console.log(local); }, []); @@ -1576,7 +1576,7 @@ const tests = { desc: 'Update the dependencies array to be: [local]', output: normalizeIndent` function MyComponent() { - const local = {}; + const local = useMemo(() => ({}), []); useEffect(() => { console.log(local); }, [local]); @@ -1702,7 +1702,7 @@ const tests = { // Regression test code: normalizeIndent` function MyComponent() { - const local = {}; + const local = useMemo(() => ({}), []); useEffect(() => { if (true) { console.log(local); @@ -1720,7 +1720,7 @@ const tests = { desc: 'Update the dependencies array to be: [local]', output: normalizeIndent` function MyComponent() { - const local = {}; + const local = useMemo(() => ({}), []); useEffect(() => { if (true) { console.log(local); @@ -1808,9 +1808,9 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = useMemo(() => ({}), []); { - const local2 = {}; + const local2 = useMemo(() => ({}), []); useEffect(() => { console.log(local1); console.log(local2); @@ -1828,9 +1828,9 @@ const tests = { desc: 'Update the dependencies array to be: [local1, local2]', output: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = useMemo(() => ({}), []); { - const local2 = {}; + const local2 = useMemo(() => ({}), []); useEffect(() => { console.log(local1); console.log(local2); @@ -1912,7 +1912,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = useMemo(() => ({}), []); function MyNestedComponent() { const local2 = {}; useCallback(() => { @@ -1934,7 +1934,7 @@ const tests = { desc: 'Update the dependencies array to be: [local2]', output: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = useMemo(() => ({}), []); function MyNestedComponent() { const local2 = {}; useCallback(() => { @@ -2361,7 +2361,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = useMemo(() => ({}), []); useEffect(() => { console.log(local); }, [local, ...dependencies]); @@ -5813,56 +5813,6 @@ const tests = { }, ], }, - { - code: normalizeIndent` - function MyComponent(props) { - let handleNext = () => { - console.log('hello'); - }; - if (props.foo) { - handleNext = () => { - console.log('hello'); - }; - } - useEffect(() => { - return Store.subscribe(handleNext); - }, [handleNext]); - } - `, - errors: [ - { - message: - "The 'handleNext' function makes the dependencies of useEffect Hook " + - '(at line 13) change on every render. To fix this, wrap the ' + - "'handleNext' definition into its own useCallback() Hook.", - // Normally we'd suggest moving handleNext inside an - // effect. But it's used more than once. - // TODO: our autofix here isn't quite sufficient because - // it only wraps the first definition. But seems ok. - suggestions: [ - { - desc: - "Wrap the 'handleNext' definition into its own useCallback() Hook.", - output: normalizeIndent` - function MyComponent(props) { - let handleNext = useCallback(() => { - console.log('hello'); - }); - if (props.foo) { - handleNext = () => { - console.log('hello'); - }; - } - useEffect(() => { - return Store.subscribe(handleNext); - }, [handleNext]); - } - `, - }, - ], - }, - ], - }, { code: normalizeIndent` function MyComponent(props) { From 75fcb35beec99a77f163725a3268c1022d85ea50 Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Wed, 12 Aug 2020 11:13:13 -0700 Subject: [PATCH 3/9] Don't give an autofix of wrapping object declarations It may not be safe to just wrap the declaration of an object, since the object may get mutated. Only offer this autofix for functions which are unlikely to get mutated. Also, update the message to clarify that the entire construction of the value should get wrapped. --- .../ESLintRuleExhaustiveDeps-test.js | 97 ++++++++----------- .../src/ExhaustiveDeps.js | 13 ++- 2 files changed, 52 insertions(+), 58 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 06b6d0df20fa4..dc21f12d2bc23 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -5377,7 +5377,7 @@ const tests = { `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 11) change on every render. ` + `Move it inside the useEffect callback. Alternatively, ` + - `wrap the 'handleNext' definition into its own useCallback() Hook.`, + `wrap the construction of 'handleNext' in its own useCallback() Hook.`, // Not gonna fix a function definition // because it's not always safe due to hoisting. suggestions: undefined, @@ -5406,7 +5406,7 @@ const tests = { `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 11) change on every render. ` + `Move it inside the useEffect callback. Alternatively, ` + - `wrap the 'handleNext' definition into its own useCallback() Hook.`, + `wrap the construction of 'handleNext' in its own useCallback() Hook.`, // We don't fix moving (too invasive). But that's the suggested fix // when only effect uses this function. Otherwise, we'd useCallback. suggestions: undefined, @@ -5439,13 +5439,13 @@ const tests = { message: `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 11) change on every render. ` + - `To fix this, wrap the 'handleNext' definition into its own useCallback() Hook.`, + `To fix this, wrap the construction of 'handleNext' in its own useCallback() Hook.`, // We fix this one with useCallback since it's // the easy fix and you can't just move it into effect. suggestions: [ { desc: - "Wrap the 'handleNext' definition into its own useCallback() Hook.", + "Wrap the construction of 'handleNext' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { let [, setState] = useState(); @@ -5494,21 +5494,21 @@ const tests = { message: "The 'handleNext1' function makes the dependencies of useEffect Hook " + '(at line 14) change on every render. Move it inside the useEffect callback. ' + - "Alternatively, wrap the 'handleNext1' definition into its own useCallback() Hook.", + "Alternatively, wrap the construction of 'handleNext1' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + '(at line 17) change on every render. Move it inside the useLayoutEffect callback. ' + - "Alternatively, wrap the 'handleNext2' definition into its own useCallback() Hook.", + "Alternatively, wrap the construction of 'handleNext2' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext3' function makes the dependencies of useMemo Hook " + '(at line 20) change on every render. Move it inside the useMemo callback. ' + - "Alternatively, wrap the 'handleNext3' definition into its own useCallback() Hook.", + "Alternatively, wrap the construction of 'handleNext3' in its own useCallback() Hook.", suggestions: undefined, }, ], @@ -5546,21 +5546,21 @@ const tests = { message: "The 'handleNext1' function makes the dependencies of useEffect Hook " + '(at line 15) change on every render. Move it inside the useEffect callback. ' + - "Alternatively, wrap the 'handleNext1' definition into its own useCallback() Hook.", + "Alternatively, wrap the construction of 'handleNext1' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + '(at line 19) change on every render. Move it inside the useLayoutEffect callback. ' + - "Alternatively, wrap the 'handleNext2' definition into its own useCallback() Hook.", + "Alternatively, wrap the construction of 'handleNext2' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext3' function makes the dependencies of useMemo Hook " + '(at line 23) change on every render. Move it inside the useMemo callback. ' + - "Alternatively, wrap the 'handleNext3' definition into its own useCallback() Hook.", + "Alternatively, wrap the construction of 'handleNext3' in its own useCallback() Hook.", suggestions: undefined, }, ], @@ -5607,20 +5607,20 @@ const tests = { message: "The 'handleNext1' function makes the dependencies of useEffect Hook " + '(at line 15) change on every render. To fix this, wrap the ' + - "'handleNext1' definition into its own useCallback() Hook.", + "construction of 'handleNext1' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + '(at line 19) change on every render. To fix this, wrap the ' + - "'handleNext2' definition into its own useCallback() Hook.", + "construction of 'handleNext2' in its own useCallback() Hook.", // Suggestion wraps into useCallback where possible (variables only) // because they are only referenced outside the effect. suggestions: [ { desc: - "Wrap the 'handleNext2' definition into its own useCallback() Hook.", + "Wrap the construction of 'handleNext2' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { function handleNext1() { @@ -5664,13 +5664,13 @@ const tests = { message: "The 'handleNext3' function makes the dependencies of useMemo Hook " + '(at line 23) change on every render. To fix this, wrap the ' + - "'handleNext3' definition into its own useCallback() Hook.", + "construction of 'handleNext3' in its own useCallback() Hook.", // Autofix wraps into useCallback where possible (variables only) // because they are only referenced outside the effect. suggestions: [ { desc: - "Wrap the 'handleNext3' definition into its own useCallback() Hook.", + "Wrap the construction of 'handleNext3' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { function handleNext1() { @@ -5741,11 +5741,11 @@ const tests = { message: "The 'handleNext1' function makes the dependencies of useEffect Hook " + '(at line 12) change on every render. To fix this, wrap the ' + - "'handleNext1' definition into its own useCallback() Hook.", + "construction of 'handleNext1' in its own useCallback() Hook.", suggestions: [ { desc: - "Wrap the 'handleNext1' definition into its own useCallback() Hook.", + "Wrap the construction of 'handleNext1' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { const handleNext1 = useCallback(() => { @@ -5771,11 +5771,11 @@ const tests = { message: "The 'handleNext1' function makes the dependencies of useEffect Hook " + '(at line 16) change on every render. To fix this, wrap the ' + - "'handleNext1' definition into its own useCallback() Hook.", + "construction of 'handleNext1' in its own useCallback() Hook.", suggestions: [ { desc: - "Wrap the 'handleNext1' definition into its own useCallback() Hook.", + "Wrap the construction of 'handleNext1' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { const handleNext1 = useCallback(() => { @@ -5801,14 +5801,14 @@ const tests = { message: "The 'handleNext2' function makes the dependencies of useEffect Hook " + '(at line 12) change on every render. To fix this, wrap the ' + - "'handleNext2' definition into its own useCallback() Hook.", + "construction of 'handleNext2' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext2' function makes the dependencies of useEffect Hook " + '(at line 16) change on every render. To fix this, wrap the ' + - "'handleNext2' definition into its own useCallback() Hook.", + "construction of 'handleNext2' in its own useCallback() Hook.", suggestions: undefined, }, ], @@ -5836,7 +5836,7 @@ const tests = { `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 14) change on every render. ` + `Move it inside the useEffect callback. Alternatively, wrap the ` + - `'handleNext' definition into its own useCallback() Hook.`, + `construction of 'handleNext' in its own useCallback() Hook.`, suggestions: undefined, }, ], @@ -6101,7 +6101,7 @@ const tests = { message: `The 'increment' function makes the dependencies of useEffect Hook ` + `(at line 14) change on every render. Move it inside the useEffect callback. ` + - `Alternatively, wrap the \'increment\' definition into its own ` + + `Alternatively, wrap the construction of \'increment\' in its own ` + `useCallback() Hook.`, suggestions: undefined, }, @@ -6994,7 +6994,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7011,7 +7011,7 @@ const tests = { { message: "The 'foo' array makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7028,7 +7028,7 @@ const tests = { { message: "The 'foo' function makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + 'useCallback() Hook.', suggestions: undefined, }, @@ -7045,7 +7045,7 @@ const tests = { { message: "The 'foo' function makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + 'useCallback() Hook.', suggestions: undefined, }, @@ -7062,7 +7062,7 @@ const tests = { { message: "The 'foo' class makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7079,7 +7079,7 @@ const tests = { { message: "The 'foo' conditional could make the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7096,7 +7096,7 @@ const tests = { { message: "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7113,7 +7113,7 @@ const tests = { { message: "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7130,7 +7130,7 @@ const tests = { { message: "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7147,7 +7147,7 @@ const tests = { { message: "The 'foo' conditional could make the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7164,7 +7164,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7181,7 +7181,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7200,7 +7200,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useCallback Hook (at line 6) change on every render. " + - "Move it inside the useCallback callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useCallback callback. Alternatively, wrap the construction of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7219,7 +7219,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useEffect Hook (at line 6) change on every render. " + - "Move it inside the useEffect callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useEffect callback. Alternatively, wrap the construction of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7238,7 +7238,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useLayoutEffect Hook (at line 6) change on every render. " + - "Move it inside the useLayoutEffect callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useLayoutEffect callback. Alternatively, wrap the construction of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7261,7 +7261,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useImperativeHandle Hook (at line 9) change on every render. " + - "Move it inside the useImperativeHandle callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useImperativeHandle callback. Alternatively, wrap the construction of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7280,7 +7280,7 @@ const tests = { { message: "The 'foo' logical expression could make the dependencies of useEffect Hook (at line 6) change on every render. " + - "Move it inside the useEffect callback. Alternatively, wrap the 'foo' definition into its own " + + "Move it inside the useEffect callback. Alternatively, wrap the construction of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7300,21 +7300,8 @@ const tests = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 7) change on every render. " + - "To fix this, wrap the 'foo' definition into its own useMemo() Hook.", - suggestions: [ - { - desc: "Wrap the 'foo' definition into its own useMemo() Hook.", - output: normalizeIndent` - function Foo(section) { - const foo = useMemo(() => { return {}; }); - console.log(foo); - useMemo(() => { - console.log(foo); - }, [foo]); - } - `, - }, - ], + "To fix this, wrap the construction of 'foo' in its own useMemo() Hook.", + suggestions: undefined, }, ], }, diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index a8204c5426fa9..f157b3aea5c6b 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -857,7 +857,7 @@ export default { const wrapperHook = depType === 'function' ? 'useCallback' : 'useMemo'; - const defaultAdvice = `wrap the '${construction.name.name}' definition into its own ${wrapperHook}() Hook.`; + const defaultAdvice = `wrap the construction of '${construction.name.name}' in its own ${wrapperHook}() Hook.`; const advice = isUsedOutsideOfHook ? `To fix this, ${defaultAdvice}` @@ -876,10 +876,17 @@ export default { let suggest; // Only handle the simple case of variable assignments. // Wrapping function declarations can mess up hoisting. - if (isUsedOutsideOfHook && construction.type === 'Variable') { + if ( + isUsedOutsideOfHook && + construction.type === 'Variable' && + // Objects may be mutated ater construction, which would make this + // fix unsafe. Functions _probably_ won't be mutated, so we'll + // allow this fix for them. + depType === 'function' + ) { suggest = [ { - desc: `Wrap the '${construction.name.name}' definition into its own ${wrapperHook}() Hook.`, + desc: `Wrap the construction of '${construction.name.name}' in its own ${wrapperHook}() Hook.`, fix(fixer) { const [before, after] = wrapperHook === 'useMemo' From 4b6e20e0ba40b5b9d88687892f1cbf0827d773f5 Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Wed, 12 Aug 2020 14:39:48 -0700 Subject: [PATCH 4/9] Handle the long tail of nodes that will be referentially unique --- .../ESLintRuleExhaustiveDeps-test.js | 170 +++++++++++++++++- .../src/ExhaustiveDeps.js | 25 +++ 2 files changed, 187 insertions(+), 8 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index dc21f12d2bc23..d66a066441b6e 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -1391,14 +1391,6 @@ const tests = { } `, }, - { - code: normalizeIndent` - function useFoo(){ - const foo = new MyObj(); - return useMemo(() => foo, [foo]); - } - `, - }, { code: normalizeIndent` function useFoo(){ @@ -7305,6 +7297,150 @@ const tests = { }, ], }, + { + code: normalizeIndent` + function Foo() { + const foo = <>Hi!; + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' JSX fragment makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + const foo =
Hi!
; + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' JSX element makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + const foo = bar = {}; + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' assignment expression makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + const foo = new String('foo'); // Note 'foo' will be boxed, and thus an object and thus compared by reference. + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object construction makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + const foo = new Map([]); + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object construction makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + const foo = /reg/; + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' regular expression makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + const foo = ({}: any); + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + class Bar {}; + useMemo(() => { + console.log(new Bar()); + }, [Bar]); + } + `, + errors: [ + { + message: + "The 'Bar' class makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'Bar' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, ], }; @@ -7683,6 +7819,24 @@ const testsTypescript = { }, ], }, + { + code: normalizeIndent` + function Foo() { + const foo = {} as any;; + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index f157b3aea5c6b..5cb1974721715 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -1428,6 +1428,26 @@ function getConstructionExpresionType(node) { return 'logical expression'; } return null; + case 'JSXFragment': + return 'JSX fragment'; + case 'JSXElement': + return 'JSX element'; + case 'AssignmentExpression': + if (getConstructionExpresionType(node.right) != null) { + return 'assignment expression'; + } + return null; + case 'NewExpression': + return 'object construction'; + case 'Literal': + if (node.value instanceof RegExp) { + return 'regular expression'; + } + return null; + case 'TypeCastExpression': + return getConstructionExpresionType(node.expression); + case 'TSAsExpression': + return getConstructionExpresionType(node.expression); } return null; } @@ -1482,6 +1502,11 @@ function scanForConstructions({ ) { return [ref, 'function']; } + + // class Foo {} + if (node.type === 'ClassName' && node.node.type === 'ClassDeclaration') { + return [ref, 'class']; + } return null; }) .filter(Boolean); From e0ca072197821d999012773faf329d181851a2f4 Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Thu, 13 Aug 2020 09:25:27 -0700 Subject: [PATCH 5/9] Catch let/var constant constructions on initial assignment --- .../ESLintRuleExhaustiveDeps-test.js | 59 ++++++++++++++++--- .../src/ExhaustiveDeps.js | 6 -- 2 files changed, 50 insertions(+), 15 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index d66a066441b6e..b017018427123 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -1391,15 +1391,6 @@ const tests = { } `, }, - { - code: normalizeIndent` - function useFoo(){ - let foo = {}; - foo = 1; - return useMemo(() => foo, [foo]); - } - `, - }, { code: normalizeIndent` function useFoo(){ @@ -5805,6 +5796,56 @@ const tests = { }, ], }, + { + code: normalizeIndent` + function MyComponent(props) { + let handleNext = () => { + console.log('hello'); + }; + if (props.foo) { + handleNext = () => { + console.log('hello'); + }; + } + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + errors: [ + { + message: + "The 'handleNext' function makes the dependencies of useEffect Hook " + + '(at line 13) change on every render. To fix this, wrap the construction of ' + + "'handleNext' in its own useCallback() Hook.", + // Normally we'd suggest moving handleNext inside an + // effect. But it's used more than once. + // TODO: our autofix here isn't quite sufficient because + // it only wraps the first definition. But seems ok. + suggestions: [ + { + desc: + "Wrap the construction of 'handleNext' in its own useCallback() Hook.", + output: normalizeIndent` + function MyComponent(props) { + let handleNext = useCallback(() => { + console.log('hello'); + }); + if (props.foo) { + handleNext = () => { + console.log('hello'); + }; + } + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + }, + ], + }, + ], + }, { code: normalizeIndent` function MyComponent(props) { diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 5cb1974721715..6c7f40fcbb998 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -1467,12 +1467,6 @@ function scanForConstructions({ return null; } - if (ref.references.some(r => !r.init && r.isWrite())) { - // The variable gets reassigned. This complicates things so we won't - // try to reason about it for now. - return null; - } - const node = ref.defs[0]; if (node == null) { return null; From 4dc35db7b77b345babba9146eb8ba40c2e2922f2 Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Thu, 13 Aug 2020 09:31:38 -0700 Subject: [PATCH 6/9] Trim trailing whitespace --- .../ESLintRuleExhaustiveDeps-test.js | 58 +++++++++---------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index b017018427123..bde084638727a 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -1387,7 +1387,7 @@ const tests = { code: normalizeIndent` function useFoo(){ const foo = "hi!"; - return useMemo(() => foo, [foo]); + return useMemo(() => foo, [foo]); } `, }, @@ -5797,20 +5797,20 @@ const tests = { ], }, { - code: normalizeIndent` - function MyComponent(props) { - let handleNext = () => { - console.log('hello'); - }; - if (props.foo) { - handleNext = () => { - console.log('hello'); - }; - } - useEffect(() => { - return Store.subscribe(handleNext); - }, [handleNext]); - } + code: normalizeIndent` + function MyComponent(props) { + let handleNext = () => { + console.log('hello'); + }; + if (props.foo) { + handleNext = () => { + console.log('hello'); + }; + } + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } `, errors: [ { @@ -5826,20 +5826,20 @@ const tests = { { desc: "Wrap the construction of 'handleNext' in its own useCallback() Hook.", - output: normalizeIndent` - function MyComponent(props) { - let handleNext = useCallback(() => { - console.log('hello'); - }); - if (props.foo) { - handleNext = () => { - console.log('hello'); - }; - } - useEffect(() => { - return Store.subscribe(handleNext); - }, [handleNext]); - } + output: normalizeIndent` + function MyComponent(props) { + let handleNext = useCallback(() => { + console.log('hello'); + }); + if (props.foo) { + handleNext = () => { + console.log('hello'); + }; + } + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } `, }, ], From eaa2aff9424e7ded164799fa2aef27ccfa4ed3d0 Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Thu, 13 Aug 2020 11:12:24 -0700 Subject: [PATCH 7/9] Address feedback from @gaearon --- .../ESLintRuleExhaustiveDeps-test.js | 189 +++++++++--------- .../src/ExhaustiveDeps.js | 25 ++- 2 files changed, 113 insertions(+), 101 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index bde084638727a..9d8f202b22beb 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -56,7 +56,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = useMemo(() => ({}), []); + const local = someFunc(); useEffect(() => { console.log(local); }, [local]); @@ -94,9 +94,9 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = useMemo(() => ({}), []); + const local1 = someFunc(); { - const local2 = useMemo(() => ({}), []); + const local2 = someFunc(); useCallback(() => { console.log(local1); console.log(local2); @@ -108,9 +108,9 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = useMemo(() => ({}), []); + const local1 = someFunc(); function MyNestedComponent() { - const local2 = useMemo(() => ({}), []); + const local2 = someFunc(); useCallback(() => { console.log(local1); console.log(local2); @@ -122,7 +122,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = useMemo(() => ({}), []); + const local = someFunc(); useEffect(() => { console.log(local); console.log(local); @@ -142,7 +142,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = useMemo(() => ({}), []); + const local = someFunc(); useEffect(() => { console.log(local); }, [,,,local,,,]); @@ -222,7 +222,7 @@ const tests = { { code: normalizeIndent` function MyComponent(props) { - const local = useMemo(() => ({}), []); + const local = someFunc(); useEffect(() => { console.log(props.foo); console.log(props.bar); @@ -243,7 +243,7 @@ const tests = { console.log(props.bar); }, [props, props.foo]); - let color = useMemo(() => ({}), []); + let color = someFunc(); useEffect(() => { console.log(props.foo.bar.baz); console.log(color); @@ -416,7 +416,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = useMemo(() => ({}), []); + const local = someFunc(); function myEffect() { console.log(local); } @@ -731,7 +731,7 @@ const tests = { // direct assignments. code: normalizeIndent` function MyComponent(props) { - let obj = useMemo(() => ({}), []); + let obj = someFunc(); useEffect(() => { obj.foo = true; }, [obj]); @@ -1411,7 +1411,8 @@ const tests = { code: normalizeIndent` function useFoo() { const foo = "fine"; - if(true) { + if (true) { + // Shadowed variable with constant construction in a nested scope is fine. const foo = {}; } return useMemo(() => foo, [foo]); @@ -1425,6 +1426,14 @@ const tests = { } `, }, + { + code: normalizeIndent` + function MyComponent() { + const foo = true ? "fine" : "also fine"; + return useMemo(() => foo, [foo]); + } + `, + }, ], invalid: [ { @@ -1543,7 +1552,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = useMemo(() => ({}), []); + const local = someFunc(); useEffect(() => { console.log(local); }, []); @@ -1559,7 +1568,7 @@ const tests = { desc: 'Update the dependencies array to be: [local]', output: normalizeIndent` function MyComponent() { - const local = useMemo(() => ({}), []); + const local = someFunc(); useEffect(() => { console.log(local); }, [local]); @@ -1685,7 +1694,7 @@ const tests = { // Regression test code: normalizeIndent` function MyComponent() { - const local = useMemo(() => ({}), []); + const local = someFunc(); useEffect(() => { if (true) { console.log(local); @@ -1703,7 +1712,7 @@ const tests = { desc: 'Update the dependencies array to be: [local]', output: normalizeIndent` function MyComponent() { - const local = useMemo(() => ({}), []); + const local = someFunc(); useEffect(() => { if (true) { console.log(local); @@ -1791,9 +1800,9 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = useMemo(() => ({}), []); + const local1 = someFunc(); { - const local2 = useMemo(() => ({}), []); + const local2 = someFunc(); useEffect(() => { console.log(local1); console.log(local2); @@ -1811,9 +1820,9 @@ const tests = { desc: 'Update the dependencies array to be: [local1, local2]', output: normalizeIndent` function MyComponent() { - const local1 = useMemo(() => ({}), []); + const local1 = someFunc(); { - const local2 = useMemo(() => ({}), []); + const local2 = someFunc(); useEffect(() => { console.log(local1); console.log(local2); @@ -1895,7 +1904,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = useMemo(() => ({}), []); + const local1 = someFunc(); function MyNestedComponent() { const local2 = {}; useCallback(() => { @@ -1917,7 +1926,7 @@ const tests = { desc: 'Update the dependencies array to be: [local2]', output: normalizeIndent` function MyComponent() { - const local1 = useMemo(() => ({}), []); + const local1 = someFunc(); function MyNestedComponent() { const local2 = {}; useCallback(() => { @@ -2344,7 +2353,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = useMemo(() => ({}), []); + const local = someFunc(); useEffect(() => { console.log(local); }, [local, ...dependencies]); @@ -5360,7 +5369,7 @@ const tests = { `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 11) change on every render. ` + `Move it inside the useEffect callback. Alternatively, ` + - `wrap the construction of 'handleNext' in its own useCallback() Hook.`, + `wrap the definition of 'handleNext' in its own useCallback() Hook.`, // Not gonna fix a function definition // because it's not always safe due to hoisting. suggestions: undefined, @@ -5389,7 +5398,7 @@ const tests = { `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 11) change on every render. ` + `Move it inside the useEffect callback. Alternatively, ` + - `wrap the construction of 'handleNext' in its own useCallback() Hook.`, + `wrap the definition of 'handleNext' in its own useCallback() Hook.`, // We don't fix moving (too invasive). But that's the suggested fix // when only effect uses this function. Otherwise, we'd useCallback. suggestions: undefined, @@ -5422,13 +5431,13 @@ const tests = { message: `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 11) change on every render. ` + - `To fix this, wrap the construction of 'handleNext' in its own useCallback() Hook.`, + `To fix this, wrap the definition of 'handleNext' in its own useCallback() Hook.`, // We fix this one with useCallback since it's // the easy fix and you can't just move it into effect. suggestions: [ { desc: - "Wrap the construction of 'handleNext' in its own useCallback() Hook.", + "Wrap the definition of 'handleNext' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { let [, setState] = useState(); @@ -5477,21 +5486,21 @@ const tests = { message: "The 'handleNext1' function makes the dependencies of useEffect Hook " + '(at line 14) change on every render. Move it inside the useEffect callback. ' + - "Alternatively, wrap the construction of 'handleNext1' in its own useCallback() Hook.", + "Alternatively, wrap the definition of 'handleNext1' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + '(at line 17) change on every render. Move it inside the useLayoutEffect callback. ' + - "Alternatively, wrap the construction of 'handleNext2' in its own useCallback() Hook.", + "Alternatively, wrap the definition of 'handleNext2' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext3' function makes the dependencies of useMemo Hook " + '(at line 20) change on every render. Move it inside the useMemo callback. ' + - "Alternatively, wrap the construction of 'handleNext3' in its own useCallback() Hook.", + "Alternatively, wrap the definition of 'handleNext3' in its own useCallback() Hook.", suggestions: undefined, }, ], @@ -5529,21 +5538,21 @@ const tests = { message: "The 'handleNext1' function makes the dependencies of useEffect Hook " + '(at line 15) change on every render. Move it inside the useEffect callback. ' + - "Alternatively, wrap the construction of 'handleNext1' in its own useCallback() Hook.", + "Alternatively, wrap the definition of 'handleNext1' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + '(at line 19) change on every render. Move it inside the useLayoutEffect callback. ' + - "Alternatively, wrap the construction of 'handleNext2' in its own useCallback() Hook.", + "Alternatively, wrap the definition of 'handleNext2' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext3' function makes the dependencies of useMemo Hook " + '(at line 23) change on every render. Move it inside the useMemo callback. ' + - "Alternatively, wrap the construction of 'handleNext3' in its own useCallback() Hook.", + "Alternatively, wrap the definition of 'handleNext3' in its own useCallback() Hook.", suggestions: undefined, }, ], @@ -5590,20 +5599,20 @@ const tests = { message: "The 'handleNext1' function makes the dependencies of useEffect Hook " + '(at line 15) change on every render. To fix this, wrap the ' + - "construction of 'handleNext1' in its own useCallback() Hook.", + "definition of 'handleNext1' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + '(at line 19) change on every render. To fix this, wrap the ' + - "construction of 'handleNext2' in its own useCallback() Hook.", + "definition of 'handleNext2' in its own useCallback() Hook.", // Suggestion wraps into useCallback where possible (variables only) // because they are only referenced outside the effect. suggestions: [ { desc: - "Wrap the construction of 'handleNext2' in its own useCallback() Hook.", + "Wrap the definition of 'handleNext2' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { function handleNext1() { @@ -5647,13 +5656,13 @@ const tests = { message: "The 'handleNext3' function makes the dependencies of useMemo Hook " + '(at line 23) change on every render. To fix this, wrap the ' + - "construction of 'handleNext3' in its own useCallback() Hook.", + "definition of 'handleNext3' in its own useCallback() Hook.", // Autofix wraps into useCallback where possible (variables only) // because they are only referenced outside the effect. suggestions: [ { desc: - "Wrap the construction of 'handleNext3' in its own useCallback() Hook.", + "Wrap the definition of 'handleNext3' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { function handleNext1() { @@ -5724,11 +5733,11 @@ const tests = { message: "The 'handleNext1' function makes the dependencies of useEffect Hook " + '(at line 12) change on every render. To fix this, wrap the ' + - "construction of 'handleNext1' in its own useCallback() Hook.", + "definition of 'handleNext1' in its own useCallback() Hook.", suggestions: [ { desc: - "Wrap the construction of 'handleNext1' in its own useCallback() Hook.", + "Wrap the definition of 'handleNext1' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { const handleNext1 = useCallback(() => { @@ -5754,11 +5763,11 @@ const tests = { message: "The 'handleNext1' function makes the dependencies of useEffect Hook " + '(at line 16) change on every render. To fix this, wrap the ' + - "construction of 'handleNext1' in its own useCallback() Hook.", + "definition of 'handleNext1' in its own useCallback() Hook.", suggestions: [ { desc: - "Wrap the construction of 'handleNext1' in its own useCallback() Hook.", + "Wrap the definition of 'handleNext1' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { const handleNext1 = useCallback(() => { @@ -5784,14 +5793,14 @@ const tests = { message: "The 'handleNext2' function makes the dependencies of useEffect Hook " + '(at line 12) change on every render. To fix this, wrap the ' + - "construction of 'handleNext2' in its own useCallback() Hook.", + "definition of 'handleNext2' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext2' function makes the dependencies of useEffect Hook " + '(at line 16) change on every render. To fix this, wrap the ' + - "construction of 'handleNext2' in its own useCallback() Hook.", + "definition of 'handleNext2' in its own useCallback() Hook.", suggestions: undefined, }, ], @@ -5816,7 +5825,7 @@ const tests = { { message: "The 'handleNext' function makes the dependencies of useEffect Hook " + - '(at line 13) change on every render. To fix this, wrap the construction of ' + + '(at line 13) change on every render. To fix this, wrap the definition of ' + "'handleNext' in its own useCallback() Hook.", // Normally we'd suggest moving handleNext inside an // effect. But it's used more than once. @@ -5825,22 +5834,22 @@ const tests = { suggestions: [ { desc: - "Wrap the construction of 'handleNext' in its own useCallback() Hook.", + "Wrap the definition of 'handleNext' in its own useCallback() Hook.", output: normalizeIndent` - function MyComponent(props) { - let handleNext = useCallback(() => { - console.log('hello'); - }); - if (props.foo) { - handleNext = () => { - console.log('hello'); - }; - } - useEffect(() => { - return Store.subscribe(handleNext); - }, [handleNext]); - } - `, + function MyComponent(props) { + let handleNext = useCallback(() => { + console.log('hello'); + }); + if (props.foo) { + handleNext = () => { + console.log('hello'); + }; + } + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, }, ], }, @@ -5869,7 +5878,7 @@ const tests = { `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 14) change on every render. ` + `Move it inside the useEffect callback. Alternatively, wrap the ` + - `construction of 'handleNext' in its own useCallback() Hook.`, + `definition of 'handleNext' in its own useCallback() Hook.`, suggestions: undefined, }, ], @@ -6134,7 +6143,7 @@ const tests = { message: `The 'increment' function makes the dependencies of useEffect Hook ` + `(at line 14) change on every render. Move it inside the useEffect callback. ` + - `Alternatively, wrap the construction of \'increment\' in its own ` + + `Alternatively, wrap the definition of \'increment\' in its own ` + `useCallback() Hook.`, suggestions: undefined, }, @@ -7027,7 +7036,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7044,7 +7053,7 @@ const tests = { { message: "The 'foo' array makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7061,7 +7070,7 @@ const tests = { { message: "The 'foo' function makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the definition of 'foo' in its own " + 'useCallback() Hook.', suggestions: undefined, }, @@ -7078,7 +7087,7 @@ const tests = { { message: "The 'foo' function makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the definition of 'foo' in its own " + 'useCallback() Hook.', suggestions: undefined, }, @@ -7095,7 +7104,7 @@ const tests = { { message: "The 'foo' class makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7112,7 +7121,7 @@ const tests = { { message: "The 'foo' conditional could make the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7129,7 +7138,7 @@ const tests = { { message: "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7146,7 +7155,7 @@ const tests = { { message: "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7163,7 +7172,7 @@ const tests = { { message: "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7180,7 +7189,7 @@ const tests = { { message: "The 'foo' conditional could make the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7197,7 +7206,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7214,7 +7223,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7233,7 +7242,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useCallback Hook (at line 6) change on every render. " + - "Move it inside the useCallback callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useCallback callback. Alternatively, wrap the assignment of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7252,7 +7261,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useEffect Hook (at line 6) change on every render. " + - "Move it inside the useEffect callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useEffect callback. Alternatively, wrap the assignment of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7271,7 +7280,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useLayoutEffect Hook (at line 6) change on every render. " + - "Move it inside the useLayoutEffect callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useLayoutEffect callback. Alternatively, wrap the assignment of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7294,7 +7303,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useImperativeHandle Hook (at line 9) change on every render. " + - "Move it inside the useImperativeHandle callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useImperativeHandle callback. Alternatively, wrap the assignment of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7313,7 +7322,7 @@ const tests = { { message: "The 'foo' logical expression could make the dependencies of useEffect Hook (at line 6) change on every render. " + - "Move it inside the useEffect callback. Alternatively, wrap the construction of 'foo' in its own " + + "Move it inside the useEffect callback. Alternatively, wrap the assignment of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7333,7 +7342,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 7) change on every render. " + - "To fix this, wrap the construction of 'foo' in its own useMemo() Hook.", + "To fix this, wrap the assignment of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7351,7 +7360,7 @@ const tests = { { message: "The 'foo' JSX fragment makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7369,7 +7378,7 @@ const tests = { { message: "The 'foo' JSX element makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7387,7 +7396,7 @@ const tests = { { message: "The 'foo' assignment expression makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7405,7 +7414,7 @@ const tests = { { message: "The 'foo' object construction makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7423,7 +7432,7 @@ const tests = { { message: "The 'foo' object construction makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7441,7 +7450,7 @@ const tests = { { message: "The 'foo' regular expression makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7459,7 +7468,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7477,7 +7486,7 @@ const tests = { { message: "The 'Bar' class makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'Bar' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'Bar' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7863,7 +7872,7 @@ const testsTypescript = { { code: normalizeIndent` function Foo() { - const foo = {} as any;; + const foo = {} as any; useMemo(() => { console.log(foo); }, [foo]); @@ -7873,7 +7882,7 @@ const testsTypescript = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the construction of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 6c7f40fcbb998..7144bb04f47ae 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -857,7 +857,10 @@ export default { const wrapperHook = depType === 'function' ? 'useCallback' : 'useMemo'; - const defaultAdvice = `wrap the construction of '${construction.name.name}' in its own ${wrapperHook}() Hook.`; + const constructionType = + depType === 'function' ? 'definition' : 'assignment'; + + const defaultAdvice = `wrap the ${constructionType} of '${construction.name.name}' in its own ${wrapperHook}() Hook.`; const advice = isUsedOutsideOfHook ? `To fix this, ${defaultAdvice}` @@ -886,7 +889,7 @@ export default { ) { suggest = [ { - desc: `Wrap the construction of '${construction.name.name}' in its own ${wrapperHook}() Hook.`, + desc: `Wrap the ${constructionType} of '${construction.name.name}' in its own ${wrapperHook}() Hook.`, fix(fixer) { const [before, after] = wrapperHook === 'useMemo' @@ -1401,7 +1404,7 @@ function collectRecommendations({ // If the node will result in constructing a referentially unique value, return // its human readable type name, else return null. -function getConstructionExpresionType(node) { +function getConstructionExpressionType(node) { switch (node.type) { case 'ObjectExpression': return 'object'; @@ -1414,16 +1417,16 @@ function getConstructionExpresionType(node) { return 'class'; case 'ConditionalExpression': if ( - getConstructionExpresionType(node.consequent) != null || - getConstructionExpresionType(node.alternate) != null + getConstructionExpressionType(node.consequent) != null || + getConstructionExpressionType(node.alternate) != null ) { return 'conditional'; } return null; case 'LogicalExpression': if ( - getConstructionExpresionType(node.left) != null || - getConstructionExpresionType(node.right) != null + getConstructionExpressionType(node.left) != null || + getConstructionExpressionType(node.right) != null ) { return 'logical expression'; } @@ -1433,7 +1436,7 @@ function getConstructionExpresionType(node) { case 'JSXElement': return 'JSX element'; case 'AssignmentExpression': - if (getConstructionExpresionType(node.right) != null) { + if (getConstructionExpressionType(node.right) != null) { return 'assignment expression'; } return null; @@ -1445,9 +1448,9 @@ function getConstructionExpresionType(node) { } return null; case 'TypeCastExpression': - return getConstructionExpresionType(node.expression); + return getConstructionExpressionType(node.expression); case 'TSAsExpression': - return getConstructionExpresionType(node.expression); + return getConstructionExpressionType(node.expression); } return null; } @@ -1482,7 +1485,7 @@ function scanForConstructions({ node.node.id.type === 'Identifier' && // Ensure this is not destructed assignment node.node.init != null ) { - const constantExpressionType = getConstructionExpresionType( + const constantExpressionType = getConstructionExpressionType( node.node.init, ); if (constantExpressionType != null) { From 2b7ae4b79faed0d182b0aaa468b79b63171dc437 Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Thu, 13 Aug 2020 11:17:54 -0700 Subject: [PATCH 8/9] Rename "assignment" to "initialization" --- .../ESLintRuleExhaustiveDeps-test.js | 50 +++++++++---------- .../src/ExhaustiveDeps.js | 2 +- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 9d8f202b22beb..bfdd5fcd55c1a 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -7036,7 +7036,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7053,7 +7053,7 @@ const tests = { { message: "The 'foo' array makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7104,7 +7104,7 @@ const tests = { { message: "The 'foo' class makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7121,7 +7121,7 @@ const tests = { { message: "The 'foo' conditional could make the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7138,7 +7138,7 @@ const tests = { { message: "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7155,7 +7155,7 @@ const tests = { { message: "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7172,7 +7172,7 @@ const tests = { { message: "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7189,7 +7189,7 @@ const tests = { { message: "The 'foo' conditional could make the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7206,7 +7206,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7223,7 +7223,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7242,7 +7242,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useCallback Hook (at line 6) change on every render. " + - "Move it inside the useCallback callback. Alternatively, wrap the assignment of 'foo' in its own " + + "Move it inside the useCallback callback. Alternatively, wrap the initialization of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7261,7 +7261,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useEffect Hook (at line 6) change on every render. " + - "Move it inside the useEffect callback. Alternatively, wrap the assignment of 'foo' in its own " + + "Move it inside the useEffect callback. Alternatively, wrap the initialization of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7280,7 +7280,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useLayoutEffect Hook (at line 6) change on every render. " + - "Move it inside the useLayoutEffect callback. Alternatively, wrap the assignment of 'foo' in its own " + + "Move it inside the useLayoutEffect callback. Alternatively, wrap the initialization of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7303,7 +7303,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useImperativeHandle Hook (at line 9) change on every render. " + - "Move it inside the useImperativeHandle callback. Alternatively, wrap the assignment of 'foo' in its own " + + "Move it inside the useImperativeHandle callback. Alternatively, wrap the initialization of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7322,7 +7322,7 @@ const tests = { { message: "The 'foo' logical expression could make the dependencies of useEffect Hook (at line 6) change on every render. " + - "Move it inside the useEffect callback. Alternatively, wrap the assignment of 'foo' in its own " + + "Move it inside the useEffect callback. Alternatively, wrap the initialization of 'foo' in its own " + 'useMemo() Hook.', suggestions: undefined, }, @@ -7342,7 +7342,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 7) change on every render. " + - "To fix this, wrap the assignment of 'foo' in its own useMemo() Hook.", + "To fix this, wrap the initialization of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7360,7 +7360,7 @@ const tests = { { message: "The 'foo' JSX fragment makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7378,7 +7378,7 @@ const tests = { { message: "The 'foo' JSX element makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7396,7 +7396,7 @@ const tests = { { message: "The 'foo' assignment expression makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7414,7 +7414,7 @@ const tests = { { message: "The 'foo' object construction makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7432,7 +7432,7 @@ const tests = { { message: "The 'foo' object construction makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7450,7 +7450,7 @@ const tests = { { message: "The 'foo' regular expression makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7468,7 +7468,7 @@ const tests = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7486,7 +7486,7 @@ const tests = { { message: "The 'Bar' class makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'Bar' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'Bar' in its own useMemo() Hook.", suggestions: undefined, }, ], @@ -7882,7 +7882,7 @@ const testsTypescript = { { message: "The 'foo' object makes the dependencies of useMemo Hook (at line 6) change on every render. " + - "Move it inside the useMemo callback. Alternatively, wrap the assignment of 'foo' in its own useMemo() Hook.", + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", suggestions: undefined, }, ], diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 7144bb04f47ae..ab552c305a4db 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -858,7 +858,7 @@ export default { depType === 'function' ? 'useCallback' : 'useMemo'; const constructionType = - depType === 'function' ? 'definition' : 'assignment'; + depType === 'function' ? 'definition' : 'initialization'; const defaultAdvice = `wrap the ${constructionType} of '${construction.name.name}' in its own ${wrapperHook}() Hook.`; From 31f75cd4a0579c0be93b71ffc48ce0b308dad334 Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Thu, 13 Aug 2020 11:27:15 -0700 Subject: [PATCH 9/9] Add test for a constant construction used in multiple dependency arrays --- .../ESLintRuleExhaustiveDeps-test.js | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index bfdd5fcd55c1a..26cda5e290875 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -7491,6 +7491,33 @@ const tests = { }, ], }, + { + code: normalizeIndent` + function Foo() { + const foo = {}; + useLayoutEffect(() => { + console.log(foo); + }, [foo]); + useEffect(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useLayoutEffect Hook (at line 6) change on every render. " + + "To fix this, wrap the initialization of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + { + message: + "The 'foo' object makes the dependencies of useEffect Hook (at line 9) change on every render. " + + "To fix this, wrap the initialization of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, ], };