diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index ac710f7f90127..bc0dc20ccff1b 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) { @@ -307,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() { @@ -692,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 @@ -1264,17 +1356,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]); } `, }, @@ -1867,18 +2045,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]); } `, }, @@ -3479,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() { @@ -3683,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) { @@ -4074,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() { @@ -4448,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` @@ -6008,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 1bb4d3252fdc9..b8dd7fd043a20 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,6 +588,7 @@ export default { if ( parent != null && // ref.current + // Note: no need to handle OptionalMemberExpression because it can't be LHS. parent.type === 'MemberExpression' && !parent.computed && parent.property.type === 'Identifier' && @@ -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; } @@ -1016,11 +1024,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; } @@ -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; @@ -1062,7 +1071,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++) { @@ -1238,7 +1247,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 +1260,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 +1269,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 +1281,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 +1293,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; @@ -1454,12 +1445,14 @@ 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 ( + // Note: we don't check OptionalMemberExpression because it can't be LHS. node.type === 'MemberExpression' && node.parent && node.parent.type === 'AssignmentExpression' @@ -1475,20 +1468,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}`); }