diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index a1c7932368507..c30146cac3635 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -849,6 +849,106 @@ const tests = { } `, }, + { + code: ` + function withFetch(fetchPodcasts) { + return function Podcasts({ id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + fetchPodcasts(id).then(setPodcasts); + }, [id]); + } + } + `, + }, + { + code: ` + function Podcasts({ id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + function doFetch({ fetchPodcasts }) { + fetchPodcasts(id).then(setPodcasts); + } + doFetch({ fetchPodcasts: API.fetchPodcasts }); + }, [id]); + } + `, + }, + { + code: ` + function Counter() { + let [count, setCount] = useState(0); + + function increment(x) { + return x + 1; + } + + useEffect(() => { + let id = setInterval(() => { + setCount(increment); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + }, + { + code: ` + function Counter() { + let [count, setCount] = useState(0); + + function increment(x) { + return x + 1; + } + + useEffect(() => { + let id = setInterval(() => { + setCount(count => increment(count)); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + }, + { + code: ` + import increment from './increment'; + function Counter() { + let [count, setCount] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + }, + { + code: ` + function withStuff(increment) { + return function Counter() { + let [count, setCount] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + } + `, + }, ], invalid: [ { @@ -2203,7 +2303,10 @@ const tests = { `, errors: [ "React Hook useEffect has a missing dependency: 'state'. " + - 'Either include it or remove the dependency array.', + 'Either include it or remove the dependency array. ' + + `If 'state' is only necessary for calculating the next state, ` + + `consider refactoring to the setState(state => ...) form which ` + + `doesn't need to depend on the state from outside.`, ], }, { @@ -2232,7 +2335,10 @@ const tests = { `, errors: [ "React Hook useEffect has a missing dependency: 'state'. " + - 'Either include it or remove the dependency array.', + 'Either include it or remove the dependency array. ' + + `If 'state' is only necessary for calculating the next state, ` + + `consider refactoring to the setState(state => ...) form which ` + + `doesn't need to depend on the state from outside.`, ], }, { @@ -2447,7 +2553,9 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'props'. " + 'Either include it or remove the dependency array. ' + - 'Alternatively, destructure the necessary props outside the callback.', + `However, the preferred fix is to destructure the 'props' ` + + `object outside of the useEffect call and refer to specific ` + + `props directly by their names.`, ], }, { @@ -2478,7 +2586,9 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'props'. " + 'Either include it or remove the dependency array. ' + - 'Alternatively, destructure the necessary props outside the callback.', + `However, the preferred fix is to destructure the 'props' ` + + `object outside of the useEffect call and refer to specific ` + + `props directly by their names.`, ], }, { @@ -2529,7 +2639,9 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'props'. " + 'Either include it or remove the dependency array. ' + - 'Alternatively, destructure the necessary props outside the callback.', + `However, the preferred fix is to destructure the 'props' ` + + `object outside of the useEffect call and refer to specific ` + + `props directly by their names.`, ], }, { @@ -2556,7 +2668,9 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'props'. " + 'Either include it or remove the dependency array. ' + - 'Alternatively, destructure the necessary props outside the callback.', + `However, the preferred fix is to destructure the 'props' ` + + `object outside of the useEffect call and refer to specific ` + + `props directly by their names.`, ], }, { @@ -2583,7 +2697,9 @@ const tests = { errors: [ "React Hook useEffect has missing dependencies: 'props' and 'skillsCount'. " + 'Either include them or remove the dependency array. ' + - 'Alternatively, destructure the necessary props outside the callback.', + `However, the preferred fix is to destructure the 'props' ` + + `object outside of the useEffect call and refer to specific ` + + `props directly by their names.`, ], }, { @@ -2638,11 +2754,15 @@ const tests = { let value; let value2; let value3; + let value4; let asyncValue; useEffect(() => { - value = {}; + if (value4) { + value = {}; + } value2 = 100; value = 43; + value4 = true; console.log(value2); console.log(value3); setTimeout(() => { @@ -2659,11 +2779,15 @@ const tests = { let value; let value2; let value3; + let value4; let asyncValue; useEffect(() => { - value = {}; + if (value4) { + value = {}; + } value2 = 100; value = 43; + value4 = true; console.log(value2); console.log(value3); setTimeout(() => { @@ -2673,14 +2797,20 @@ const tests = { } `, errors: [ + // value2 + `Assignments to the 'value2' variable from inside a React useEffect Hook ` + + `will not persist between re-renders. ` + + `If it's only needed by this Hook, move the variable inside it. ` + + `Alternatively, declare a ref with the useRef Hook, ` + + `and keep the mutable value in its 'current' property.`, // value `Assignments to the 'value' variable from inside a React useEffect Hook ` + `will not persist between re-renders. ` + `If it's only needed by this Hook, move the variable inside it. ` + `Alternatively, declare a ref with the useRef Hook, ` + `and keep the mutable value in its 'current' property.`, - // value2 - `Assignments to the 'value2' variable from inside a React useEffect Hook ` + + // value4 + `Assignments to the 'value4' variable from inside a React useEffect Hook ` + `will not persist between re-renders. ` + `If it's only needed by this Hook, move the variable inside it. ` + `Alternatively, declare a ref with the useRef Hook, ` + @@ -3339,7 +3469,7 @@ const tests = { `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 11) change on every render. ` + `To fix this, move the 'handleNext' function ` + - `inside the useEffect callback. Alternatively, ` + + `inside the useEffect callback (at line 9). Alternatively, ` + `wrap the 'handleNext' definition into its own useCallback() Hook.`, ], }, @@ -3378,7 +3508,7 @@ const tests = { `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 11) change on every render. ` + `To fix this, move the 'handleNext' function ` + - `inside the useEffect callback. Alternatively, ` + + `inside the useEffect callback (at line 9). Alternatively, ` + `wrap the 'handleNext' definition into its own useCallback() Hook.`, ], }, @@ -3476,15 +3606,15 @@ const tests = { errors: [ "The 'handleNext1' function makes the dependencies of useEffect Hook " + "(at line 14) change on every render. To fix this, move the 'handleNext1' " + - 'function inside the useEffect callback. Alternatively, wrap the ' + + 'function inside the useEffect callback (at line 12). Alternatively, wrap the ' + "'handleNext1' definition into its own useCallback() Hook.", "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + "(at line 17) change on every render. To fix this, move the 'handleNext2' " + - 'function inside the useLayoutEffect callback. Alternatively, wrap the ' + + 'function inside the useLayoutEffect callback (at line 15). Alternatively, wrap the ' + "'handleNext2' definition into its own useCallback() Hook.", "The 'handleNext3' function makes the dependencies of useMemo Hook " + "(at line 20) change on every render. To fix this, move the 'handleNext3' " + - 'function inside the useMemo callback. Alternatively, wrap the ' + + 'function inside the useMemo callback (at line 18). Alternatively, wrap the ' + "'handleNext3' definition into its own useCallback() Hook.", ], }, @@ -3544,15 +3674,15 @@ const tests = { errors: [ "The 'handleNext1' function makes the dependencies of useEffect Hook " + "(at line 15) change on every render. To fix this, move the 'handleNext1' " + - 'function inside the useEffect callback. Alternatively, wrap the ' + + 'function inside the useEffect callback (at line 12). Alternatively, wrap the ' + "'handleNext1' definition into its own useCallback() Hook.", "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + "(at line 19) change on every render. To fix this, move the 'handleNext2' " + - 'function inside the useLayoutEffect callback. Alternatively, wrap the ' + + 'function inside the useLayoutEffect callback (at line 16). Alternatively, wrap the ' + "'handleNext2' definition into its own useCallback() Hook.", "The 'handleNext3' function makes the dependencies of useMemo Hook " + "(at line 23) change on every render. To fix this, move the 'handleNext3' " + - 'function inside the useMemo callback. Alternatively, wrap the ' + + 'function inside the useMemo callback (at line 20). Alternatively, wrap the ' + "'handleNext3' definition into its own useCallback() Hook.", ], }, @@ -3700,6 +3830,47 @@ const tests = { "'handleNext2' definition into its own useCallback() Hook.", ], }, + { + code: ` + function MyComponent(props) { + let handleNext = () => { + console.log('hello'); + }; + if (props.foo) { + handleNext = () => { + console.log('hello'); + }; + } + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + // 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. + output: ` + function MyComponent(props) { + let handleNext = useCallback(() => { + console.log('hello'); + }); + if (props.foo) { + handleNext = () => { + console.log('hello'); + }; + } + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + errors: [ + "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.", + ], + }, { code: ` function MyComponent(props) { @@ -3737,7 +3908,7 @@ const tests = { `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 14) change on every render. ` + `To fix this, move the 'handleNext' function inside ` + - `the useEffect callback. Alternatively, wrap the ` + + `the useEffect callback (at line 12). Alternatively, wrap the ` + `'handleNext' definition into its own useCallback() Hook.`, ], }, @@ -3770,13 +3941,260 @@ const tests = { return

{count}

; } `, - // TODO: ideally this should suggest useState updater form - // since this code doesn't actually work. errors: [ "React Hook useEffect has a missing dependency: 'count'. " + + 'Either include it or remove the dependency array. ' + + `If 'count' is only necessary for calculating the next state, ` + + `consider refactoring to the setCount(count => ...) form which ` + + `doesn't need to depend on the state from outside.`, + ], + }, + { + code: ` + function Counter() { + let [count, setCount] = useState(0); + let [increment, setIncrement] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count + increment); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + output: ` + function Counter() { + let [count, setCount] = useState(0); + let [increment, setIncrement] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count + increment); + }, 1000); + return () => clearInterval(id); + }, [count, increment]); + + return

{count}

; + } + `, + errors: [ + "React Hook useEffect has missing dependencies: 'count' and 'increment'. " + + 'Either include them or remove the dependency array. ' + + `If 'count' is only necessary for calculating the next state, ` + + `consider refactoring to the setCount(count => ...) form which ` + + `doesn't need to depend on the state from outside.`, + ], + }, + { + code: ` + function Counter() { + let [count, setCount] = useState(0); + let [increment, setIncrement] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + output: ` + function Counter() { + let [count, setCount] = useState(0); + let [increment, setIncrement] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, [increment]); + + return

{count}

; + } + `, + errors: [ + "React Hook useEffect has a missing dependency: 'increment'. " + + 'Either include it or remove the dependency array. ' + + `If 'increment' is only necessary for calculating the next state, ` + + `consider refactoring to the useReducer Hook. This ` + + `lets you move the calculation of next state outside the effect.`, + ], + }, + { + code: ` + function Counter() { + let [count, setCount] = useState(0); + let increment = useCustomHook(); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + output: ` + function Counter() { + let [count, setCount] = useState(0); + let increment = useCustomHook(); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, [increment]); + + return

{count}

; + } + `, + // This intentionally doesn't show the reducer message + // because we don't know if it's safe for it to close over a value. + // We only show it for state variables (and possibly props). + errors: [ + "React Hook useEffect has a missing dependency: 'increment'. " + + 'Either include it or remove the dependency array.', + ], + }, + { + code: ` + function Counter({ step }) { + let [count, setCount] = useState(0); + + function increment(x) { + return x + step; + } + + useEffect(() => { + let id = setInterval(() => { + setCount(count => increment(count)); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + output: ` + function Counter({ step }) { + let [count, setCount] = useState(0); + + function increment(x) { + return x + step; + } + + useEffect(() => { + let id = setInterval(() => { + setCount(count => increment(count)); + }, 1000); + return () => clearInterval(id); + }, [increment]); + + return

{count}

; + } + `, + // This intentionally doesn't show the reducer message + // because we don't know if it's safe for it to close over a value. + // We only show it for state variables (and possibly props). + errors: [ + "React Hook useEffect has a missing dependency: 'increment'. " + 'Either include it or remove the dependency array.', ], }, + { + code: ` + function Counter({ step }) { + let [count, setCount] = useState(0); + + function increment(x) { + return x + step; + } + + useEffect(() => { + let id = setInterval(() => { + setCount(count => increment(count)); + }, 1000); + return () => clearInterval(id); + }, [increment]); + + return

{count}

; + } + `, + output: ` + function Counter({ step }) { + let [count, setCount] = useState(0); + + function increment(x) { + return x + step; + } + + useEffect(() => { + let id = setInterval(() => { + setCount(count => increment(count)); + }, 1000); + return () => clearInterval(id); + }, [increment]); + + return

{count}

; + } + `, + errors: [ + `The 'increment' function makes the dependencies of useEffect Hook ` + + `(at line 14) change on every render. To fix this, move the ` + + `'increment' function inside the useEffect callback (at line 9). ` + + `Alternatively, wrap the \'increment\' definition into its own ` + + `useCallback() Hook.`, + ], + }, + { + code: ` + function Counter({ increment }) { + let [count, setCount] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + output: ` + function Counter({ increment }) { + let [count, setCount] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, [increment]); + + return

{count}

; + } + `, + errors: [ + "React Hook useEffect has a missing dependency: 'increment'. " + + 'Either include it or remove the dependency array. ' + + `If 'increment' is only necessary for calculating the next state, ` + + `consider refactoring to the useReducer Hook. This lets you move ` + + `the calculation of next state outside the effect. ` + + `You can then read 'increment' from the reducer ` + + `by putting it directly in your component.`, + ], + }, { code: ` function Counter() { @@ -3849,6 +4267,112 @@ const tests = { `Either include it or remove the dependency array.`, ], }, + { + code: ` + function Podcasts({ fetchPodcasts, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + fetchPodcasts(id).then(setPodcasts); + }, [id]); + } + `, + output: ` + function Podcasts({ fetchPodcasts, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + fetchPodcasts(id).then(setPodcasts); + }, [fetchPodcasts, id]); + } + `, + errors: [ + `React Hook useEffect has a missing dependency: 'fetchPodcasts'. ` + + `Either include it or remove the dependency array. ` + + `If specifying 'fetchPodcasts' makes the dependencies change too often, ` + + `find the parent component that defines it and wrap that definition in useCallback.`, + ], + }, + { + code: ` + function Podcasts({ api: { fetchPodcasts }, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + fetchPodcasts(id).then(setPodcasts); + }, [id]); + } + `, + output: ` + function Podcasts({ api: { fetchPodcasts }, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + fetchPodcasts(id).then(setPodcasts); + }, [fetchPodcasts, id]); + } + `, + errors: [ + `React Hook useEffect has a missing dependency: 'fetchPodcasts'. ` + + `Either include it or remove the dependency array. ` + + `If specifying 'fetchPodcasts' makes the dependencies change too often, ` + + `find the parent component that defines it and wrap that definition in useCallback.`, + ], + }, + { + code: ` + function Podcasts({ fetchPodcasts, fetchPodcasts2, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + setTimeout(() => { + console.log(id); + fetchPodcasts(id).then(setPodcasts); + fetchPodcasts2(id).then(setPodcasts); + }); + }, [id]); + } + `, + output: ` + function Podcasts({ fetchPodcasts, fetchPodcasts2, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + setTimeout(() => { + console.log(id); + fetchPodcasts(id).then(setPodcasts); + fetchPodcasts2(id).then(setPodcasts); + }); + }, [fetchPodcasts, fetchPodcasts2, id]); + } + `, + errors: [ + `React Hook useEffect has missing dependencies: 'fetchPodcasts' and 'fetchPodcasts2'. ` + + `Either include them or remove the dependency array. ` + + `If specifying 'fetchPodcasts' makes the dependencies change too often, ` + + `find the parent component that defines it and wrap that definition in useCallback.`, + ], + }, + { + code: ` + function Podcasts({ fetchPodcasts, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + console.log(fetchPodcasts); + fetchPodcasts(id).then(setPodcasts); + }, [id]); + } + `, + output: ` + function Podcasts({ fetchPodcasts, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + console.log(fetchPodcasts); + fetchPodcasts(id).then(setPodcasts); + }, [fetchPodcasts, id]); + } + `, + errors: [ + `React Hook useEffect has a missing dependency: 'fetchPodcasts'. ` + + `Either include it or remove the dependency array. ` + + `If specifying 'fetchPodcasts' makes the dependencies change too often, ` + + `find the parent component that defines it and wrap that definition in useCallback.`, + ], + }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 2f8b18c0069a7..512f71db07500 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -35,6 +35,8 @@ export default { const options = {additionalHooks}; // Should be shared between visitors. + let setStateCallSites = new WeakMap(); + let stateVariables = new WeakSet(); let staticKnownValueCache = new WeakMap(); let functionWithoutCapturedValueCache = new WeakMap(); function memoizeWithWeakMap(fn, map) { @@ -204,19 +206,40 @@ export default { return false; } const id = def.node.id; - if (callee.name === 'useRef' && id.type === 'Identifier') { + const {name} = callee; + if (name === 'useRef' && id.type === 'Identifier') { // useRef() return value is static. return true; - } else if (callee.name === 'useState' || callee.name === 'useReducer') { + } else if (name === 'useState' || name === 'useReducer') { // Only consider second value in initializing tuple static. if ( id.type === 'ArrayPattern' && id.elements.length === 2 && - Array.isArray(resolved.identifiers) && - // Is second tuple value the same reference we're checking? - id.elements[1] === resolved.identifiers[0] + Array.isArray(resolved.identifiers) ) { - return true; + // Is second tuple value the same reference we're checking? + if (id.elements[1] === resolved.identifiers[0]) { + if (name === 'useState') { + const references = resolved.references; + for (let i = 0; i < references.length; i++) { + setStateCallSites.set( + references[i].identifier, + id.elements[0], + ); + } + } + // Setter is static. + return true; + } else if (id.elements[0] === resolved.identifiers[0]) { + if (name === 'useState') { + const references = resolved.references; + for (let i = 0; i < references.length; i++) { + stateVariables.add(references[i].identifier); + } + } + // State variable itself is dynamic. + return false; + } } } // By default assume it's dynamic. @@ -365,8 +388,10 @@ export default { memoizedIsFunctionWithoutCapturedValues(resolved); dependencies.set(dependency, { isStatic, - reference, + references: [reference], }); + } else { + dependencies.get(dependency).references.push(reference); } } for (const childScope of currentScope.childScopes) { @@ -514,9 +539,12 @@ export default { // Warn about assigning to variables in the outer scope. // Those are usually bugs. - let foundStaleAssignments = false; + let staleAssignments = new Set(); function reportStaleAssignment(writeExpr, key) { - foundStaleAssignments = true; + if (staleAssignments.has(key)) { + return; + } + staleAssignments.add(key); context.report({ node: writeExpr, message: @@ -532,15 +560,17 @@ export default { // Remember which deps are optional and report bad usage first. const optionalDependencies = new Set(); - dependencies.forEach(({isStatic, reference}, key) => { + dependencies.forEach(({isStatic, references}, key) => { if (isStatic) { optionalDependencies.add(key); } - if (reference.writeExpr) { - reportStaleAssignment(reference.writeExpr, key); - } + references.forEach(reference => { + if (reference.writeExpr) { + reportStaleAssignment(reference.writeExpr, key); + } + }); }); - if (foundStaleAssignments) { + if (staleAssignments.size > 0) { // The intent isn't clear so we'll wait until you fix those first. return; } @@ -588,8 +618,10 @@ export default { } else { message += ` To fix this, move the '${fn.name.name}' function ` + - `inside the ${reactiveHookName} callback. Alternatively, ` + - `wrap the '${ + `inside the ${reactiveHookName} callback (at line ${ + node.loc.start.line + }). ` + + `Alternatively, wrap the '${ fn.name.name }' definition into its own useCallback() Hook.`; } @@ -697,7 +729,7 @@ export default { if (propDep == null) { return; } - const refs = propDep.reference.resolved.references; + const refs = propDep.references; if (!Array.isArray(refs)) { return; } @@ -724,8 +756,154 @@ export default { } if (isPropsOnlyUsedInMembers) { extraWarning = - ' Alternatively, destructure the necessary props ' + - 'outside the callback.'; + ` However, the preferred fix is to destructure the 'props' ` + + `object outside of the ${reactiveHookName} call and ` + + `refer to specific props directly by their names.`; + } + } + + if (!extraWarning && missingDependencies.size > 0) { + // See if the user is trying to avoid specifying a callable prop. + // This usually means they're unaware of useCallback. + let missingCallbackDep = null; + missingDependencies.forEach(missingDep => { + if (missingCallbackDep) { + return; + } + // Is this a variable from top scope? + const topScopeRef = componentScope.set.get(missingDep); + const usedDep = dependencies.get(missingDep); + if (usedDep.references[0].resolved !== topScopeRef) { + return; + } + // Is this a destructured prop? + const def = topScopeRef.defs[0]; + if (def == null || def.name == null || def.type !== 'Parameter') { + return; + } + // Was it called in at least one case? Then it's a function. + let isFunctionCall = false; + let id; + for (let i = 0; i < usedDep.references.length; i++) { + id = usedDep.references[i].identifier; + if ( + id != null && + id.parent != null && + id.parent.type === 'CallExpression' && + id.parent.callee === id + ) { + isFunctionCall = true; + break; + } + } + if (!isFunctionCall) { + return; + } + // If it's missing (i.e. in component scope) *and* it's a parameter + // then it is definitely coming from props destructuring. + // (It could also be props itself but we wouldn't be calling it then.) + missingCallbackDep = missingDep; + }); + if (missingCallbackDep !== null) { + extraWarning = + ` If specifying '${missingCallbackDep}'` + + ` makes the dependencies change too often, ` + + `find the parent component that defines it ` + + `and wrap that definition in useCallback.`; + } + } + + if (!extraWarning && missingDependencies.size > 0) { + let setStateRecommendation = null; + missingDependencies.forEach(missingDep => { + if (setStateRecommendation !== null) { + return; + } + const usedDep = dependencies.get(missingDep); + const references = usedDep.references; + let id; + let maybeCall; + for (let i = 0; i < references.length; i++) { + id = references[i].identifier; + maybeCall = id.parent; + // Try to see if we have setState(someExpr(missingDep)). + while (maybeCall != null && maybeCall !== componentScope.block) { + if (maybeCall.type === 'CallExpression') { + const correspondingStateVariable = setStateCallSites.get( + maybeCall.callee, + ); + if (correspondingStateVariable != null) { + if (correspondingStateVariable.name === missingDep) { + // setCount(count + 1) + setStateRecommendation = { + missingDep, + setter: maybeCall.callee.name, + form: 'updater', + }; + } else if (stateVariables.has(id)) { + // setCount(count + increment) + setStateRecommendation = { + missingDep, + setter: maybeCall.callee.name, + form: 'reducer', + }; + } else { + const resolved = references[i].resolved; + if (resolved != null) { + // If it's a parameter *and* a missing dep, + // it must be a prop or something inside a prop. + // Therefore, recommend an inline reducer. + const def = resolved.defs[0]; + if (def != null && def.type === 'Parameter') { + setStateRecommendation = { + missingDep, + setter: maybeCall.callee.name, + form: 'inlineReducer', + }; + } + } + } + break; + } + } + maybeCall = maybeCall.parent; + } + if (setStateRecommendation !== null) { + break; + } + } + }); + if (setStateRecommendation !== null) { + let suggestion; + switch (setStateRecommendation.form) { + case 'reducer': + suggestion = + 'useReducer Hook. This lets you move the calculation ' + + 'of next state outside the effect.'; + break; + case 'inlineReducer': + suggestion = + 'useReducer Hook. This lets you move the calculation ' + + 'of next state outside the effect. You can then ' + + `read '${ + setStateRecommendation.missingDep + }' from the reducer ` + + `by putting it directly in your component.`; + break; + case 'updater': + suggestion = + `${setStateRecommendation.setter}(${ + setStateRecommendation.missingDep + } => ...) form ` + + `which doesn't need to depend on the state from outside.`; + break; + default: + throw new Error('Unknown case.'); + } + extraWarning = + ` If '${setStateRecommendation.missingDep}'` + + ` is only necessary for calculating the next state, ` + + `consider refactoring to the ${suggestion}`; } } @@ -981,9 +1159,7 @@ function scanForDeclaredBareFunctions({ if (currentScope !== scope) { // This reference is outside the Hook callback. // It can only be legit if it's the deps array. - if (isAncestorNodeOf(declaredDependenciesNode, reference.identifier)) { - continue; - } else { + if (!isAncestorNodeOf(declaredDependenciesNode, reference.identifier)) { return true; } }