From 2142f31e1db30f49531d5f5d5fcd867a3e2b74ae Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 6 Jul 2020 23:06:23 +0100 Subject: [PATCH 1/7] Revert "Fix ExhaustiveDeps ESLint rule throwing with optional chaining (#19260)" This reverts commit 0f84b0f02b5579d780a9f54497007c4c84aaebb7. --- .../ESLintRuleExhaustiveDeps-test.js | 38 ------------------- .../src/ExhaustiveDeps.js | 6 +-- 2 files changed, 3 insertions(+), 41 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index ac710f7f90127..68a3bd6b77bc8 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -6628,44 +6628,6 @@ const testsTypescript = { }, ], }, - { - // https://github.com/facebook/react/issues/19243 - code: normalizeIndent` - function MyComponent() { - const pizza = {}; - - useEffect(() => ({ - crust: pizza.crust, - toppings: pizza?.toppings, - }), []); - } - `, - errors: [ - { - message: - "React Hook useEffect has missing dependencies: 'pizza.crust' and 'pizza.toppings'. " + - 'Either include them or remove the dependency array.', - suggestions: [ - { - // TODO the description and suggestions should probably also - // preserve the optional chaining. - desc: - 'Update the dependencies array to be: [pizza.crust, pizza.toppings]', - output: normalizeIndent` - function MyComponent() { - const pizza = {}; - - useEffect(() => ({ - crust: pizza.crust, - toppings: pizza?.toppings, - }), [pizza.crust, pizza.toppings]); - } - `, - }, - ], - }, - ], - }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 1bb4d3252fdc9..8a23c88cec20b 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -1016,11 +1016,11 @@ export default { // Is this a variable from top scope? const topScopeRef = componentScope.set.get(missingDep); const usedDep = dependencies.get(missingDep); - if (usedDep && usedDep.references[0].resolved !== topScopeRef) { + if (usedDep.references[0].resolved !== topScopeRef) { return; } // Is this a destructured prop? - const def = topScopeRef && topScopeRef.defs[0]; + const def = topScopeRef.defs[0]; if (def == null || def.name == null || def.type !== 'Parameter') { return; } @@ -1062,7 +1062,7 @@ export default { return; } const usedDep = dependencies.get(missingDep); - const references = usedDep ? usedDep.references : []; + const references = usedDep.references; let id; let maybeCall; for (let i = 0; i < references.length; i++) { From 02f10d95a1f45c29cba215ca2c4c0e29aa973f42 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 7 Jul 2020 15:40:14 +0100 Subject: [PATCH 2/7] Re-add a test from #19260 --- .../ESLintRuleExhaustiveDeps-test.js | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 68a3bd6b77bc8..ac710f7f90127 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -6628,6 +6628,44 @@ const testsTypescript = { }, ], }, + { + // https://github.com/facebook/react/issues/19243 + code: normalizeIndent` + function MyComponent() { + const pizza = {}; + + useEffect(() => ({ + crust: pizza.crust, + toppings: pizza?.toppings, + }), []); + } + `, + errors: [ + { + message: + "React Hook useEffect has missing dependencies: 'pizza.crust' and 'pizza.toppings'. " + + 'Either include them or remove the dependency array.', + suggestions: [ + { + // TODO the description and suggestions should probably also + // preserve the optional chaining. + desc: + 'Update the dependencies array to be: [pizza.crust, pizza.toppings]', + output: normalizeIndent` + function MyComponent() { + const pizza = {}; + + useEffect(() => ({ + crust: pizza.crust, + toppings: pizza?.toppings, + }), [pizza.crust, pizza.toppings]); + } + `, + }, + ], + }, + ], + }, ], }; From 0ec1b83a2980e0f3aca141436b792c3953991adb Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 7 Jul 2020 15:40:26 +0100 Subject: [PATCH 3/7] Remove all code for optional chaining support --- .../src/ExhaustiveDeps.js | 26 +++---------------- 1 file changed, 4 insertions(+), 22 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 8a23c88cec20b..ed5990cb89e32 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -1238,7 +1238,7 @@ function collectRecommendations({ const keys = path.split('.'); let node = rootNode; for (const key of keys) { - let child = getChildByKey(node, key); + let child = node.children.get(key); if (!child) { child = createDepTree(); node.children.set(key, child); @@ -1251,7 +1251,7 @@ function collectRecommendations({ const keys = path.split('.'); let node = rootNode; for (const key of keys) { - const child = getChildByKey(node, key); + const child = node.children.get(key); if (!child) { return; } @@ -1260,21 +1260,6 @@ function collectRecommendations({ } } - /** - * Match key with optional chaining - * key -> key - * key? -> key - * key -> key? - * Otherwise undefined. - */ - function getChildByKey(node, key) { - return ( - node.children.get(key) || - node.children.get(key.split('?')[0]) || - node.children.get(key + '?') - ); - } - // Now we can learn which dependencies are missing or necessary. const missingDependencies = new Set(); const satisfyingDependencies = new Set(); @@ -1287,13 +1272,10 @@ function collectRecommendations({ function scanTreeRecursively(node, missingPaths, satisfyingPaths, keyToPath) { node.children.forEach((child, key) => { const path = keyToPath(key); - // For analyzing dependencies, we want the "normalized" path, without any optional chaining ("?.") operator - // foo?.bar -> foo.bar - const normalizedPath = path.replace(/\?$/, ''); if (child.isSatisfiedRecursively) { if (child.hasRequiredNodesBelow) { // Remember this dep actually satisfied something. - satisfyingPaths.add(normalizedPath); + satisfyingPaths.add(path); } // It doesn't matter if there's something deeper. // It would be transitively satisfied since we assume immutability. @@ -1302,7 +1284,7 @@ function collectRecommendations({ } if (child.isRequired) { // Remember that no declared deps satisfied this node. - missingPaths.add(normalizedPath); + missingPaths.add(path); // If we got here, nothing in its subtree was satisfied. // No need to search further. return; From c0daad95446c0114db0ebac3056e6be4c95cb7d3 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 7 Jul 2020 16:39:25 +0100 Subject: [PATCH 4/7] Consistently treat optional chaining as regular chaining This is not ideal because our suggestions use normal chaining. But it gets rid of all current edge cases. --- .../__tests__/ESLintRuleExhaustiveDeps-test.js | 13 ++++++------- .../src/ExhaustiveDeps.js | 15 +++++++++------ 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index ac710f7f90127..c131c6d400c5a 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -1264,17 +1264,16 @@ const tests = { errors: [ { message: - "React Hook useCallback has a missing dependency: 'props.foo?.toString'. " + + "React Hook useCallback has a missing dependency: 'props.foo.toString'. " + 'Either include it or remove the dependency array.', suggestions: [ { - desc: - 'Update the dependencies array to be: [props.foo?.toString]', + desc: 'Update the dependencies array to be: [props.foo.toString]', output: normalizeIndent` function MyComponent(props) { useCallback(() => { console.log(props.foo?.toString()); - }, [props.foo?.toString]); + }, [props.foo.toString]); } `, }, @@ -1867,18 +1866,18 @@ const tests = { errors: [ { message: - "React Hook useEffect has a missing dependency: 'history?.foo'. " + + "React Hook useEffect has a missing dependency: 'history.foo'. " + 'Either include it or remove the dependency array.', suggestions: [ { - desc: 'Update the dependencies array to be: [history?.foo]', + desc: 'Update the dependencies array to be: [history.foo]', output: normalizeIndent` function MyComponent({ history }) { useEffect(() => { return [ history?.foo ]; - }, [history?.foo]); + }, [history.foo]); } `, }, diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index ed5990cb89e32..c1fd6cae9ef00 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -1457,20 +1457,23 @@ function getDependency(node) { * (foo) -> 'foo' * foo.(bar) -> 'foo.bar' * foo.bar.(baz) -> 'foo.bar.baz' - * foo?.(bar) -> 'foo?.bar' * Otherwise throw. */ function toPropertyAccessString(node) { if (node.type === 'Identifier') { return node.name; - } else if (node.type === 'MemberExpression' && !node.computed) { + } else if ( + (node.type === 'MemberExpression' || + node.type === 'OptionalMemberExpression') && + !node.computed + ) { const object = toPropertyAccessString(node.object); const property = toPropertyAccessString(node.property); + // Note: we intentionally omit ? even for optional chaining + // because the returned string represents a path to the node, and + // is used as a key in Maps where being optional doesn't matter. + // The result string is not being interpolated in the code output. return `${object}.${property}`; - } else if (node.type === 'OptionalMemberExpression' && !node.computed) { - const object = toPropertyAccessString(node.object); - const property = toPropertyAccessString(node.property); - return `${object}?.${property}`; } else { throw new Error(`Unsupported node type: ${node.type}`); } From 17ff93d4f2b9ad7f0236324d0b43cb9e0d568966 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 7 Jul 2020 16:41:15 +0100 Subject: [PATCH 5/7] Add more tests --- .../ESLintRuleExhaustiveDeps-test.js | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index c131c6d400c5a..4a47613ea33d6 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -270,6 +270,44 @@ const tests = { } `, }, + { + code: normalizeIndent` + function MyComponent(props) { + useEffect(() => { + console.log(props.foo?.bar); + }, [props.foo.bar]); + } + `, + }, + { + code: normalizeIndent` + function MyComponent(props) { + useEffect(() => { + console.log(props.foo.bar); + }, [props.foo?.bar]); + } + `, + }, + { + code: normalizeIndent` + function MyComponent(props) { + useEffect(() => { + console.log(props.foo.bar); + console.log(props.foo?.bar); + }, [props.foo?.bar]); + } + `, + }, + { + code: normalizeIndent` + function MyComponent(props) { + useEffect(() => { + console.log(props.foo.bar); + console.log(props.foo?.bar); + }, [props.foo.bar]); + } + `, + }, { code: normalizeIndent` function MyComponent(props) { From b9ee425b73a42c1ecc0f99bdbc84fc33c477401f Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 7 Jul 2020 17:08:39 +0100 Subject: [PATCH 6/7] More consistency in treating normal and optional expressions --- .../ESLintRuleExhaustiveDeps-test.js | 129 +++++++++++++++++- .../src/ExhaustiveDeps.js | 25 +++- 2 files changed, 144 insertions(+), 10 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 4a47613ea33d6..6ee000b7c3020 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -345,6 +345,42 @@ const tests = { } `, }, + { + code: normalizeIndent` + function MyComponent(props) { + useCallback(() => { + console.log(props.foo.bar?.toString()); + }, [props.foo.bar]); + } + `, + }, + { + code: normalizeIndent` + function MyComponent(props) { + useCallback(() => { + console.log(props.foo?.bar?.toString()); + }, [props.foo.bar]); + } + `, + }, + { + code: normalizeIndent` + function MyComponent(props) { + useCallback(() => { + console.log(props.foo.bar.toString()); + }, [props?.foo?.bar]); + } + `, + }, + { + code: normalizeIndent` + function MyComponent(props) { + useCallback(() => { + console.log(props.foo?.bar?.baz); + }, [props?.foo.bar?.baz]); + } + `, + }, { code: normalizeIndent` function MyComponent() { @@ -1302,16 +1338,103 @@ const tests = { errors: [ { message: - "React Hook useCallback has a missing dependency: 'props.foo.toString'. " + + "React Hook useCallback has a missing dependency: 'props.foo'. " + 'Either include it or remove the dependency array.', suggestions: [ { - desc: 'Update the dependencies array to be: [props.foo.toString]', + desc: 'Update the dependencies array to be: [props.foo]', output: normalizeIndent` function MyComponent(props) { useCallback(() => { console.log(props.foo?.toString()); - }, [props.foo.toString]); + }, [props.foo]); + } + `, + }, + ], + }, + ], + }, + { + code: normalizeIndent` + function MyComponent(props) { + useCallback(() => { + console.log(props.foo?.bar.baz); + }, []); + } + `, + errors: [ + { + message: + // TODO: Ideally this would suggest props.foo?.bar.baz instead. + "React Hook useCallback has a missing dependency: 'props.foo.bar.baz'. " + + 'Either include it or remove the dependency array.', + suggestions: [ + { + desc: 'Update the dependencies array to be: [props.foo.bar.baz]', + output: normalizeIndent` + function MyComponent(props) { + useCallback(() => { + console.log(props.foo?.bar.baz); + }, [props.foo.bar.baz]); + } + `, + }, + ], + }, + ], + }, + { + code: normalizeIndent` + function MyComponent(props) { + useCallback(() => { + console.log(props.foo?.bar?.baz); + }, []); + } + `, + errors: [ + { + message: + // TODO: Ideally this would suggest props.foo?.bar?.baz instead. + "React Hook useCallback has a missing dependency: 'props.foo.bar.baz'. " + + 'Either include it or remove the dependency array.', + suggestions: [ + { + desc: 'Update the dependencies array to be: [props.foo.bar.baz]', + output: normalizeIndent` + function MyComponent(props) { + useCallback(() => { + console.log(props.foo?.bar?.baz); + }, [props.foo.bar.baz]); + } + `, + }, + ], + }, + ], + }, + { + code: normalizeIndent` + function MyComponent(props) { + useCallback(() => { + console.log(props.foo?.bar.toString()); + }, []); + } + `, + errors: [ + { + message: + // TODO: Ideally this would suggest props.foo?.bar instead. + "React Hook useCallback has a missing dependency: 'props.foo.bar'. " + + 'Either include it or remove the dependency array.', + suggestions: [ + { + desc: 'Update the dependencies array to be: [props.foo.bar]', + output: normalizeIndent` + function MyComponent(props) { + useCallback(() => { + console.log(props.foo?.bar.toString()); + }, [props.foo.bar]); } `, }, diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index c1fd6cae9ef00..222d25377ece5 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -525,7 +525,8 @@ export default { isEffect && // ... and this look like accessing .current... dependencyNode.type === 'Identifier' && - dependencyNode.parent.type === 'MemberExpression' && + (dependencyNode.parent.type === 'MemberExpression' || + dependencyNode.parent.type === 'OptionalMemberExpression') && !dependencyNode.parent.computed && dependencyNode.parent.property.type === 'Identifier' && dependencyNode.parent.property.name === 'current' && @@ -587,7 +588,8 @@ export default { if ( parent != null && // ref.current - parent.type === 'MemberExpression' && + (parent.type === 'MemberExpression' || + parent.type === 'OptionalMemberExpression') && !parent.computed && parent.property.type === 'Identifier' && parent.property.name === 'current' && @@ -790,7 +792,10 @@ export default { } let maybeID = declaredDependencyNode; - while (maybeID.type === 'MemberExpression') { + while ( + maybeID.type === 'MemberExpression' || + maybeID.type === 'OptionalMemberExpression' + ) { maybeID = maybeID.object; } const isDeclaredInComponent = !componentScope.through.some( @@ -991,7 +996,10 @@ export default { isPropsOnlyUsedInMembers = false; break; } - if (parent.type !== 'MemberExpression') { + if ( + parent.type !== 'MemberExpression' && + parent.type !== 'OptionalMemberExpression' + ) { isPropsOnlyUsedInMembers = false; break; } @@ -1032,7 +1040,8 @@ export default { if ( id != null && id.parent != null && - id.parent.type === 'CallExpression' && + (id.parent.type === 'CallExpression' || + id.parent.type === 'OptionalCallExpression') && id.parent.callee === id ) { isFunctionCall = true; @@ -1436,13 +1445,15 @@ function getDependency(node) { !node.parent.computed && !( node.parent.parent != null && - node.parent.parent.type === 'CallExpression' && + (node.parent.parent.type === 'CallExpression' || + node.parent.parent.type === 'OptionalCallExpression') && node.parent.parent.callee === node.parent ) ) { return getDependency(node.parent); } else if ( - node.type === 'MemberExpression' && + (node.type === 'MemberExpression' || + node.type === 'OptionalMemberExpression') && node.parent && node.parent.type === 'AssignmentExpression' ) { From 26271d4853bac9b184b7dfb9e8fb2a41f2fe0271 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 7 Jul 2020 17:29:15 +0100 Subject: [PATCH 7/7] Add regression tests for every occurrence of Optional* --- .../ESLintRuleExhaustiveDeps-test.js | 197 ++++++++++++++++++ .../src/ExhaustiveDeps.js | 8 +- 2 files changed, 201 insertions(+), 4 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 6ee000b7c3020..bc0dc20ccff1b 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -766,6 +766,24 @@ const tests = { } `, }, + { + // Valid because we assign ref.current + // ourselves. Therefore it's likely not + // a ref managed by React. + code: normalizeIndent` + function MyComponent() { + const myRef = useRef(); + useEffect(() => { + const handleMove = () => {}; + myRef.current = {}; + return () => { + console.log(myRef?.current?.toString()) + }; + }, []); + return
; + } + `, + }, { // Valid because we assign ref.current // ourselves. Therefore it's likely not @@ -3639,6 +3657,47 @@ const tests = { }, ], }, + { + code: normalizeIndent` + function MyComponent(props) { + const ref1 = useRef(); + const ref2 = useRef(); + useEffect(() => { + ref1?.current?.focus(); + console.log(ref2?.current?.textContent); + alert(props.someOtherRefs.current.innerHTML); + fetch(props.color); + }, [ref1?.current, ref2?.current, props.someOtherRefs, props.color]); + } + `, + errors: [ + { + message: + "React Hook useEffect has unnecessary dependencies: 'ref1.current' and 'ref2.current'. " + + 'Either exclude them or remove the dependency array. ' + + "Mutable values like 'ref1.current' aren't valid dependencies " + + "because mutating them doesn't re-render the component.", + suggestions: [ + { + desc: + 'Update the dependencies array to be: [props.someOtherRefs, props.color]', + output: normalizeIndent` + function MyComponent(props) { + const ref1 = useRef(); + const ref2 = useRef(); + useEffect(() => { + ref1?.current?.focus(); + console.log(ref2?.current?.textContent); + alert(props.someOtherRefs.current.innerHTML); + fetch(props.color); + }, [props.someOtherRefs, props.color]); + } + `, + }, + ], + }, + ], + }, { code: normalizeIndent` function MyComponent() { @@ -3843,6 +3902,42 @@ const tests = { }, ], }, + { + code: normalizeIndent` + function MyComponent(props) { + useEffect(() => { + if (props?.onChange) { + props?.onChange(); + } + }, []); + } + `, + errors: [ + { + message: + "React Hook useEffect has a missing dependency: 'props'. " + + 'Either include it or remove the dependency array. ' + + `However, 'props' will change when *any* prop changes, so the ` + + `preferred fix is to destructure the 'props' object outside ` + + `of the useEffect call and refer to those specific ` + + `props inside useEffect.`, + suggestions: [ + { + desc: 'Update the dependencies array to be: [props]', + output: normalizeIndent` + function MyComponent(props) { + useEffect(() => { + if (props?.onChange) { + props?.onChange(); + } + }, [props]); + } + `, + }, + ], + }, + ], + }, { code: normalizeIndent` function MyComponent(props) { @@ -4234,6 +4329,29 @@ const tests = { }, ], }, + { + code: normalizeIndent` + function MyComponent() { + const myRef = useRef(); + useEffect(() => { + const handleMove = () => {}; + myRef?.current?.addEventListener('mousemove', handleMove); + return () => myRef?.current?.removeEventListener('mousemove', handleMove); + }, []); + return
; + } + `, + errors: [ + { + message: + `The ref value 'myRef.current' will likely have changed by the time ` + + `this effect cleanup function runs. If this ref points to a node ` + + `rendered by React, copy 'myRef.current' to a variable inside the effect, ` + + `and use that variable in the cleanup function.`, + suggestions: undefined, + }, + ], + }, { code: normalizeIndent` function MyComponent() { @@ -4608,6 +4726,51 @@ const tests = { }, ], }, + { + code: normalizeIndent` + import MutableStore from 'store'; + let z = {}; + + function MyComponent(props) { + let x = props.foo; + { + let y = props.bar; + const fn = useCallback(() => { + // nothing + }, [MutableStore?.hello?.world, props.foo, x, y, z, global?.stuff]); + } + } + `, + errors: [ + { + message: + 'React Hook useCallback has unnecessary dependencies: ' + + "'MutableStore.hello.world', 'global.stuff', 'props.foo', 'x', 'y', and 'z'. " + + 'Either exclude them or remove the dependency array. ' + + "Outer scope values like 'MutableStore.hello.world' aren't valid dependencies " + + "because mutating them doesn't re-render the component.", + suggestions: [ + { + desc: 'Update the dependencies array to be: []', + output: normalizeIndent` + import MutableStore from 'store'; + let z = {}; + + function MyComponent(props) { + let x = props.foo; + { + let y = props.bar; + const fn = useCallback(() => { + // nothing + }, []); + } + } + `, + }, + ], + }, + ], + }, { // Every almost-static function is tainted by a dynamic value. code: normalizeIndent` @@ -6168,6 +6331,40 @@ const tests = { }, ], }, + { + code: normalizeIndent` + function Podcasts({ fetchPodcasts, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + console.log(fetchPodcasts); + fetchPodcasts?.(id).then(setPodcasts); + }, [id]); + } + `, + errors: [ + { + message: + `React Hook useEffect has a missing dependency: 'fetchPodcasts'. ` + + `Either include it or remove the dependency array. ` + + `If 'fetchPodcasts' changes too often, ` + + `find the parent component that defines it and wrap that definition in useCallback.`, + suggestions: [ + { + desc: 'Update the dependencies array to be: [fetchPodcasts, id]', + output: normalizeIndent` + function Podcasts({ fetchPodcasts, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + console.log(fetchPodcasts); + fetchPodcasts?.(id).then(setPodcasts); + }, [fetchPodcasts, id]); + } + `, + }, + ], + }, + ], + }, { // The mistake here is that it was moved inside the effect // so it can't be referenced in the deps array. diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 222d25377ece5..b8dd7fd043a20 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -588,8 +588,8 @@ export default { if ( parent != null && // ref.current - (parent.type === 'MemberExpression' || - parent.type === 'OptionalMemberExpression') && + // Note: no need to handle OptionalMemberExpression because it can't be LHS. + parent.type === 'MemberExpression' && !parent.computed && parent.property.type === 'Identifier' && parent.property.name === 'current' && @@ -1452,8 +1452,8 @@ function getDependency(node) { ) { return getDependency(node.parent); } else if ( - (node.type === 'MemberExpression' || - node.type === 'OptionalMemberExpression') && + // Note: we don't check OptionalMemberExpression because it can't be LHS. + node.type === 'MemberExpression' && node.parent && node.parent.type === 'AssignmentExpression' ) {