From 9b7e1d1389e080d19e71680bbbe979ec58fa7389 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 6 Mar 2019 23:50:02 +0000 Subject: [PATCH] [ESLint] Suggest moving inside a Hook or useCallback when bare function is a dependency (#15026) * Warn about bare function deps and suggest moving or useCallback * Clearer wording --- .../ESLintRuleExhaustiveDeps-test.js | 479 ++++++++++++++++-- .../src/ExhaustiveDeps.js | 133 ++++- 2 files changed, 579 insertions(+), 33 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 79713c57cdec8..0c3bd909ca253 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -313,7 +313,7 @@ const tests = { }, { code: ` - function MyComponent({ maybeRef2 }) { + function MyComponent({ maybeRef2, foo }) { const definitelyRef1 = useRef(); const definitelyRef2 = useRef(); const maybeRef1 = useSomeOtherRefyThing(); @@ -323,8 +323,8 @@ const tests = { const [state4, dispatch2] = React.useReducer(); const [state5, maybeSetState] = useFunnyState(); const [state6, maybeDispatch] = useFunnyReducer(); - function mySetState() {} - function myDispatch() {} + const mySetState = useCallback(() => {}, []); + let myDispatch = useCallback(() => {}, []); useEffect(() => { // Known to be static @@ -380,8 +380,8 @@ const tests = { const [state5, maybeSetState] = useFunnyState(); const [state6, maybeDispatch] = useFunnyReducer(); - function mySetState() {} - function myDispatch() {} + const mySetState = useCallback(() => {}, []); + let myDispatch = useCallback(() => {}, []); useEffect(() => { // Known to be static @@ -662,30 +662,6 @@ const tests = { } `, }, - { - code: ` - function MyComponent(props) { - function handleNext1() { - console.log('hello'); - } - const handleNext2 = () => { - console.log('hello'); - }; - let handleNext3 = function() { - console.log('hello'); - }; - useEffect(() => { - return Store.subscribe(handleNext1); - }, [handleNext1]); - useLayoutEffect(() => { - return Store.subscribe(handleNext2); - }, [handleNext2]); - useMemo(() => { - return Store.subscribe(handleNext3); - }, [handleNext3]); - } - `, - }, { // Declaring handleNext is optional because // it doesn't use anything in the function scope. @@ -950,8 +926,12 @@ const tests = { } `, errors: [ - "React Hook useMemo doesn't serve any purpose without a dependency array as a second argument.", - "React Hook useCallback doesn't serve any purpose without a dependency array as a second argument.", + "React Hook useMemo doesn't serve any purpose without a dependency array. " + + 'To enable this optimization, pass an array of values used by the inner ' + + 'function as the second argument to useMemo.', + "React Hook useCallback doesn't serve any purpose without a dependency array. " + + 'To enable this optimization, pass an array of values used by the inner ' + + 'function as the second argument to useCallback.', ], }, { @@ -3313,6 +3293,443 @@ const tests = { 'Either include it or remove the dependency array.', ], }, + { + // Even if the function only references static values, + // once you specify it in deps, it will invalidate them. + code: ` + function MyComponent(props) { + let [, setState] = useState(); + + function handleNext(value) { + setState(value); + } + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + // Not gonna autofix a function definition + // because it's not always safe due to hoisting. + output: ` + function MyComponent(props) { + let [, setState] = useState(); + + function handleNext(value) { + setState(value); + } + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + errors: [ + `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, ` + + `wrap the 'handleNext' definition into its own useCallback() Hook.`, + ], + }, + { + // Even if the function only references static values, + // once you specify it in deps, it will invalidate them. + code: ` + function MyComponent(props) { + let [, setState] = useState(); + + const handleNext = (value) => { + setState(value); + }; + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + // We don't autofix moving (too invasive). But that's the suggested fix + // when only effect uses this function. Otherwise, we'd useCallback. + output: ` + function MyComponent(props) { + let [, setState] = useState(); + + const handleNext = (value) => { + setState(value); + }; + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + errors: [ + `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, ` + + `wrap the 'handleNext' definition into its own useCallback() Hook.`, + ], + }, + { + // Even if the function only references static values, + // once you specify it in deps, it will invalidate them. + // However, we can't suggest moving handleNext into the + // effect because it is *also* used outside of it. + // So our suggestion is useCallback(). + code: ` + function MyComponent(props) { + let [, setState] = useState(); + + const handleNext = (value) => { + setState(value); + }; + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + + return
; + } + `, + // We autofix this one with useCallback since it's + // the easy fix and you can't just move it into effect. + output: ` + function MyComponent(props) { + let [, setState] = useState(); + + const handleNext = useCallback((value) => { + setState(value); + }); + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + + return
; + } + `, + errors: [ + `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.`, + ], + }, + { + code: ` + function MyComponent(props) { + function handleNext1() { + console.log('hello'); + } + const handleNext2 = () => { + console.log('hello'); + }; + let handleNext3 = function() { + console.log('hello'); + }; + useEffect(() => { + return Store.subscribe(handleNext1); + }, [handleNext1]); + useLayoutEffect(() => { + return Store.subscribe(handleNext2); + }, [handleNext2]); + useMemo(() => { + return Store.subscribe(handleNext3); + }, [handleNext3]); + } + `, + // Autofix doesn't wrap into useCallback here + // because they are only referenced by effect itself. + output: ` + function MyComponent(props) { + function handleNext1() { + console.log('hello'); + } + const handleNext2 = () => { + console.log('hello'); + }; + let handleNext3 = function() { + console.log('hello'); + }; + useEffect(() => { + return Store.subscribe(handleNext1); + }, [handleNext1]); + useLayoutEffect(() => { + return Store.subscribe(handleNext2); + }, [handleNext2]); + useMemo(() => { + return Store.subscribe(handleNext3); + }, [handleNext3]); + } + `, + 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 ' + + "'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 ' + + "'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 ' + + "'handleNext3' definition into its own useCallback() Hook.", + ], + }, + { + code: ` + function MyComponent(props) { + function handleNext1() { + console.log('hello'); + } + const handleNext2 = () => { + console.log('hello'); + }; + let handleNext3 = function() { + console.log('hello'); + }; + useEffect(() => { + handleNext1(); + return Store.subscribe(() => handleNext1()); + }, [handleNext1]); + useLayoutEffect(() => { + handleNext2(); + return Store.subscribe(() => handleNext2()); + }, [handleNext2]); + useMemo(() => { + handleNext3(); + return Store.subscribe(() => handleNext3()); + }, [handleNext3]); + } + `, + // Autofix doesn't wrap into useCallback here + // because they are only referenced by effect itself. + output: ` + function MyComponent(props) { + function handleNext1() { + console.log('hello'); + } + const handleNext2 = () => { + console.log('hello'); + }; + let handleNext3 = function() { + console.log('hello'); + }; + useEffect(() => { + handleNext1(); + return Store.subscribe(() => handleNext1()); + }, [handleNext1]); + useLayoutEffect(() => { + handleNext2(); + return Store.subscribe(() => handleNext2()); + }, [handleNext2]); + useMemo(() => { + handleNext3(); + return Store.subscribe(() => handleNext3()); + }, [handleNext3]); + } + `, + 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 ' + + "'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 ' + + "'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 ' + + "'handleNext3' definition into its own useCallback() Hook.", + ], + }, + { + code: ` + function MyComponent(props) { + function handleNext1() { + console.log('hello'); + } + const handleNext2 = () => { + console.log('hello'); + }; + let handleNext3 = function() { + console.log('hello'); + }; + useEffect(() => { + handleNext1(); + return Store.subscribe(() => handleNext1()); + }, [handleNext1]); + useLayoutEffect(() => { + handleNext2(); + return Store.subscribe(() => handleNext2()); + }, [handleNext2]); + useMemo(() => { + handleNext3(); + return Store.subscribe(() => handleNext3()); + }, [handleNext3]); + return ( +
{ + handleNext1(); + setTimeout(handleNext2); + setTimeout(() => { + handleNext3(); + }); + }} + /> + ); + } + `, + // Autofix wraps into useCallback where possible (variables only) + // because they are only referenced outside the effect. + output: ` + function MyComponent(props) { + function handleNext1() { + console.log('hello'); + } + const handleNext2 = useCallback(() => { + console.log('hello'); + }); + let handleNext3 = useCallback(function() { + console.log('hello'); + }); + useEffect(() => { + handleNext1(); + return Store.subscribe(() => handleNext1()); + }, [handleNext1]); + useLayoutEffect(() => { + handleNext2(); + return Store.subscribe(() => handleNext2()); + }, [handleNext2]); + useMemo(() => { + handleNext3(); + return Store.subscribe(() => handleNext3()); + }, [handleNext3]); + return ( +
{ + handleNext1(); + setTimeout(handleNext2); + setTimeout(() => { + handleNext3(); + }); + }} + /> + ); + } + `, + errors: [ + "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.", + "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.", + "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.", + ], + }, + { + code: ` + function MyComponent(props) { + const handleNext1 = () => { + console.log('hello'); + }; + function handleNext2() { + console.log('hello'); + } + useEffect(() => { + return Store.subscribe(handleNext1); + return Store.subscribe(handleNext2); + }, [handleNext1, handleNext2]); + useEffect(() => { + return Store.subscribe(handleNext1); + return Store.subscribe(handleNext2); + }, [handleNext1, handleNext2]); + } + `, + // Normally we'd suggest moving handleNext inside an + // effect. But it's used by more than one. So we + // suggest useCallback() and use it for the autofix + // where possible (variable but not declaration). + output: ` + function MyComponent(props) { + const handleNext1 = useCallback(() => { + console.log('hello'); + }); + function handleNext2() { + console.log('hello'); + } + useEffect(() => { + return Store.subscribe(handleNext1); + return Store.subscribe(handleNext2); + }, [handleNext1, handleNext2]); + useEffect(() => { + return Store.subscribe(handleNext1); + return Store.subscribe(handleNext2); + }, [handleNext1, handleNext2]); + } + `, + // TODO: we could coalesce messages for the same function if it affects multiple Hooks. + errors: [ + "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.", + "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.", + "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.", + "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.", + ], + }, + { + code: ` + function MyComponent(props) { + let [, setState] = useState(); + let taint = props.foo; + + function handleNext(value) { + let value2 = value * taint; + setState(value2); + console.log('hello'); + } + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + output: ` + function MyComponent(props) { + let [, setState] = useState(); + let taint = props.foo; + + function handleNext(value) { + let value2 = value * taint; + setState(value2); + console.log('hello'); + } + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + errors: [ + `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 ` + + `'handleNext' definition into its own useCallback() Hook.`, + ], + }, { code: ` function Counter() { diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 51efab5f5dee7..252f29a703d0d 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -93,10 +93,12 @@ export default { reactiveHookName === 'useCallback' ) { context.report({ - node: node, + node: node.parent.callee, message: `React Hook ${reactiveHookName} doesn't serve any purpose ` + - `without a dependency array as a second argument.`, + `without a dependency array. To enable ` + + `this optimization, pass an array of values used by the ` + + `inner function as the second argument to ${reactiveHookName}.`, }); } return; @@ -550,7 +552,57 @@ export default { duplicateDependencies.size + missingDependencies.size + 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({ + 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 += + ` To fix this, move the '${fn.name.name}' function ` + + `inside the ${reactiveHookName} callback. Alternatively, ` + + `wrap the '${ + fn.name.name + }' definition into its own useCallback() Hook.`; + } + context.report({ + node: fn.node, + message, + fix(fixer) { + // Only handle the simple case: arrow functions. + // Wrapping function declarations can mess up hoisting. + if (suggestUseCallback && fn.type === 'Variable') { + 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. For now, + // this is fine. Note we're intentionally not adding + // [] because that changes semantics. + fixer.insertTextAfter(fn.node.init, ')'), + ]; + } + }, + }); + }); return; } @@ -858,6 +910,83 @@ function collectRecommendations({ }; } +// Finds functions declared as dependencies +// that would invalidate on every render. +function scanForDeclaredBareFunctions({ + declaredDependencies, + declaredDependenciesNode, + componentScope, + scope, +}) { + const bareFunctions = declaredDependencies + .map(({key}) => { + const fnRef = componentScope.set.get(key); + if (fnRef == null) { + return null; + } + let fnNode = fnRef.defs[0]; + if (fnNode == null) { + return null; + } + // const handleChange = function () {} + // const handleChange = () => {} + if ( + fnNode.type === 'Variable' && + fnNode.node.type === 'VariableDeclarator' && + fnNode.node.init != null && + (fnNode.node.init.type === 'ArrowFunctionExpression' || + fnNode.node.init.type === 'FunctionExpression') + ) { + return fnRef; + } + // function handleChange() {} + if ( + fnNode.type === 'FunctionName' && + fnNode.node.type === 'FunctionDeclaration' + ) { + return fnRef; + } + return null; + }) + .filter(Boolean); + + function isUsedOutsideOfHook(fnRef) { + let foundWriteExpr = false; + for (let i = 0; i < fnRef.references.length; i++) { + const reference = fnRef.references[i]; + if (reference.writeExpr) { + if (foundWriteExpr) { + // Two writes to the same function. + return true; + } else { + // Ignore first write as it's not usage. + foundWriteExpr = true; + continue; + } + } + let currentScope = reference.from; + while (currentScope !== scope && currentScope != null) { + currentScope = currentScope.upper; + } + 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 { + return true; + } + } + } + return false; + } + + return bareFunctions.map(fnRef => ({ + fn: fnRef.defs[0], + suggestUseCallback: isUsedOutsideOfHook(fnRef), + })); +} + /** * Assuming () means the passed/returned node: * (props) => (props)