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), })); }