From 925b94b34e5ac1f01c012ed8521d6ea1606e4217 Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Thu, 13 Aug 2020 12:54:33 -0700 Subject: [PATCH] [eslint-plugin-react-hooks] Report constant constructions (#19590) * [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. * Fix/remove existing tests * 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. * Handle the long tail of nodes that will be referentially unique * Catch let/var constant constructions on initial assignment * Trim trailing whitespace * Address feedback from @gaearon * Rename "assignment" to "initialization" * Add test for a constant construction used in multiple dependency arrays --- .../ESLintRuleExhaustiveDeps-test.js | 665 ++++++++++++++++-- .../src/ExhaustiveDeps.js | 224 ++++-- 2 files changed, 773 insertions(+), 116 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..26cda5e290875 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 = someFunc(); useEffect(() => { console.log(local); }, [local]); @@ -94,9 +94,9 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = someFunc(); { - const local2 = {}; + const local2 = someFunc(); useCallback(() => { console.log(local1); console.log(local2); @@ -108,9 +108,9 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = someFunc(); function MyNestedComponent() { - const local2 = {}; + const local2 = someFunc(); useCallback(() => { console.log(local1); console.log(local2); @@ -122,7 +122,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); useEffect(() => { console.log(local); console.log(local); @@ -142,7 +142,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); useEffect(() => { console.log(local); }, [,,,local,,,]); @@ -222,7 +222,7 @@ const tests = { { code: normalizeIndent` function MyComponent(props) { - const local = {}; + 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 = {} + 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 = {}; + const local = someFunc(); function myEffect() { console.log(local); } @@ -731,7 +731,7 @@ const tests = { // direct assignments. code: normalizeIndent` function MyComponent(props) { - let obj = {}; + let obj = someFunc(); useEffect(() => { obj.foo = true; }, [obj]); @@ -1376,6 +1376,64 @@ 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(){ + 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) { + // Shadowed variable with constant construction in a nested scope is fine. + const foo = {}; + } + return useMemo(() => foo, [foo]); + } + `, + }, + { + code: normalizeIndent` + function MyComponent({foo}) { + return useMemo(() => foo, [foo]) + } + `, + }, + { + code: normalizeIndent` + function MyComponent() { + const foo = true ? "fine" : "also fine"; + return useMemo(() => foo, [foo]); + } + `, + }, ], invalid: [ { @@ -1494,7 +1552,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); useEffect(() => { console.log(local); }, []); @@ -1510,7 +1568,7 @@ const tests = { desc: 'Update the dependencies array to be: [local]', output: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); useEffect(() => { console.log(local); }, [local]); @@ -1636,7 +1694,7 @@ const tests = { // Regression test code: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); useEffect(() => { if (true) { console.log(local); @@ -1654,7 +1712,7 @@ const tests = { desc: 'Update the dependencies array to be: [local]', output: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); useEffect(() => { if (true) { console.log(local); @@ -1742,9 +1800,9 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = someFunc(); { - const local2 = {}; + const local2 = someFunc(); useEffect(() => { console.log(local1); console.log(local2); @@ -1762,9 +1820,9 @@ const tests = { desc: 'Update the dependencies array to be: [local1, local2]', output: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = someFunc(); { - const local2 = {}; + const local2 = someFunc(); useEffect(() => { console.log(local1); console.log(local2); @@ -1846,7 +1904,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = someFunc(); function MyNestedComponent() { const local2 = {}; useCallback(() => { @@ -1868,7 +1926,7 @@ const tests = { desc: 'Update the dependencies array to be: [local2]', output: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = someFunc(); function MyNestedComponent() { const local2 = {}; useCallback(() => { @@ -2295,7 +2353,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); useEffect(() => { console.log(local); }, [local, ...dependencies]); @@ -5311,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 'handleNext' definition into 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, @@ -5340,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 'handleNext' definition into 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, @@ -5373,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 'handleNext' definition into 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 'handleNext' definition into its own useCallback() Hook.", + "Wrap the definition of 'handleNext' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { let [, setState] = useState(); @@ -5428,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 'handleNext1' definition into 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 'handleNext2' definition into 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 'handleNext3' definition into its own useCallback() Hook.", + "Alternatively, wrap the definition of 'handleNext3' in its own useCallback() Hook.", suggestions: undefined, }, ], @@ -5480,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 'handleNext1' definition into 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 'handleNext2' definition into 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 'handleNext3' definition into its own useCallback() Hook.", + "Alternatively, wrap the definition of 'handleNext3' in its own useCallback() Hook.", suggestions: undefined, }, ], @@ -5541,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 ' + - "'handleNext1' definition into 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 ' + - "'handleNext2' definition into 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 'handleNext2' definition into its own useCallback() Hook.", + "Wrap the definition of 'handleNext2' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { function handleNext1() { @@ -5598,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 ' + - "'handleNext3' definition into 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 'handleNext3' definition into its own useCallback() Hook.", + "Wrap the definition of 'handleNext3' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { function handleNext1() { @@ -5675,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 ' + - "'handleNext1' definition into its own useCallback() Hook.", + "definition of 'handleNext1' in its own useCallback() Hook.", suggestions: [ { desc: - "Wrap the 'handleNext1' definition into its own useCallback() Hook.", + "Wrap the definition of 'handleNext1' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { const handleNext1 = useCallback(() => { @@ -5705,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 ' + - "'handleNext1' definition into its own useCallback() Hook.", + "definition of 'handleNext1' in its own useCallback() Hook.", suggestions: [ { desc: - "Wrap the 'handleNext1' definition into its own useCallback() Hook.", + "Wrap the definition of 'handleNext1' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { const handleNext1 = useCallback(() => { @@ -5735,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 ' + - "'handleNext2' definition into 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 ' + - "'handleNext2' definition into its own useCallback() Hook.", + "definition of 'handleNext2' in its own useCallback() Hook.", suggestions: undefined, }, ], @@ -5767,8 +5825,8 @@ const tests = { { 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.", + '(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. // TODO: our autofix here isn't quite sufficient because @@ -5776,7 +5834,7 @@ const tests = { suggestions: [ { desc: - "Wrap the 'handleNext' definition into its own useCallback() Hook.", + "Wrap the definition of 'handleNext' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { let handleNext = useCallback(() => { @@ -5820,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 ` + - `'handleNext' definition into its own useCallback() Hook.`, + `definition of 'handleNext' in its own useCallback() Hook.`, suggestions: undefined, }, ], @@ -6085,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 \'increment\' definition into its own ` + + `Alternatively, wrap the definition of \'increment\' in its own ` + `useCallback() Hook.`, suggestions: undefined, }, @@ -6967,6 +7025,499 @@ 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 initialization of 'foo' in 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 initialization of 'foo' in 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 definition of 'foo' in 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 definition of 'foo' in 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 initialization of 'foo' in 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 initialization of 'foo' in 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 initialization of 'foo' in 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 initialization of 'foo' in 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 initialization of 'foo' in 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 initialization of 'foo' in 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 initialization of 'foo' in 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 initialization of 'foo' in 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 initialization of 'foo' in 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 initialization of 'foo' in 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 initialization of 'foo' in 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 initialization of 'foo' in 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 initialization of 'foo' in 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 initialization 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 fragment makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization 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 initialization 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 initialization 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 initialization 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 initialization 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 initialization 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 initialization 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 initialization of 'Bar' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + 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, + }, + ], + }, ], }; @@ -7345,6 +7896,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 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 9e344e8662d6d..ab552c305a4db 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -844,59 +844,80 @@ 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.`; - } + constructions.forEach( + ({construction, isUsedOutsideOfHook, depType}) => { + const wrapperHook = + depType === 'function' ? 'useCallback' : 'useMemo'; - 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, ')'), - ]; + const constructionType = + depType === 'function' ? 'definition' : 'initialization'; + + const defaultAdvice = `wrap the ${constructionType} of '${construction.name.name}' in 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' && + // 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 ${constructionType} of '${construction.name.name}' in 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 +1402,116 @@ 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 getConstructionExpressionType(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 ( + getConstructionExpressionType(node.consequent) != null || + getConstructionExpressionType(node.alternate) != null + ) { + return 'conditional'; + } + return null; + case 'LogicalExpression': + if ( + getConstructionExpressionType(node.left) != null || + getConstructionExpressionType(node.right) != null + ) { + return 'logical expression'; + } + return null; + case 'JSXFragment': + return 'JSX fragment'; + case 'JSXElement': + return 'JSX element'; + case 'AssignmentExpression': + if (getConstructionExpressionType(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 getConstructionExpressionType(node.expression); + case 'TSAsExpression': + return getConstructionExpressionType(node.expression); + } + 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; } - 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 = getConstructionExpressionType( + 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']; + } + + // class Foo {} + if (node.type === 'ClassName' && node.node.type === 'ClassDeclaration') { + return [ref, 'class']; } 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 +1537,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), })); }