@samitouri / QOS-React / commits / 1d5e10f703

[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

Jordan Eldredge committed Aug 13, 2020 at 12:54 UTC 1d5e10f7035f0d3bcbffcd057a15940b1a20b164

2 files changed +773 -116

packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js

+617 -48

@@ -56,7 +56,7 @@ const tests = {

56 {

57 code: normalizeIndent`

58 function MyComponent() {

59 - const local = {};

59 + const local = someFunc();

60 useEffect(() => {

61 console.log(local);

62 }, [local]);

@@ -94,9 +94,9 @@ const tests = {

94 {

95 code: normalizeIndent`

96 function MyComponent() {

97 - const local1 = {};

97 + const local1 = someFunc();

98 {

99 - const local2 = {};

99 + const local2 = someFunc();

100 useCallback(() => {

101 console.log(local1);

102 console.log(local2);

@@ -108,9 +108,9 @@ const tests = {

108 {

109 code: normalizeIndent`

110 function MyComponent() {

111 - const local1 = {};

111 + const local1 = someFunc();

112 function MyNestedComponent() {

113 - const local2 = {};

113 + const local2 = someFunc();

114 useCallback(() => {

115 console.log(local1);

116 console.log(local2);

@@ -122,7 +122,7 @@ const tests = {

122 {

123 code: normalizeIndent`

124 function MyComponent() {

125 - const local = {};

125 + const local = someFunc();

126 useEffect(() => {

127 console.log(local);

128 console.log(local);

@@ -142,7 +142,7 @@ const tests = {

142 {

143 code: normalizeIndent`

144 function MyComponent() {

145 - const local = {};

145 + const local = someFunc();

146 useEffect(() => {

147 console.log(local);

148 }, [,,,local,,,]);

@@ -222,7 +222,7 @@ const tests = {

222 {

223 code: normalizeIndent`

224 function MyComponent(props) {

225 - const local = {};

225 + const local = someFunc();

226 useEffect(() => {

227 console.log(props.foo);

228 console.log(props.bar);

@@ -243,7 +243,7 @@ const tests = {

243 console.log(props.bar);

244 }, [props, props.foo]);

245

246 - let color = {}

246 + let color = someFunc();

247 useEffect(() => {

248 console.log(props.foo.bar.baz);

249 console.log(color);

@@ -416,7 +416,7 @@ const tests = {

416 {

417 code: normalizeIndent`

418 function MyComponent() {

419 - const local = {};

419 + const local = someFunc();

420 function myEffect() {

421 console.log(local);

422 }

@@ -731,7 +731,7 @@ const tests = {

731 // direct assignments.

732 code: normalizeIndent`

733 function MyComponent(props) {

734 - let obj = {};

734 + let obj = someFunc();

735 useEffect(() => {

736 obj.foo = true;

737 }, [obj]);

@@ -1376,6 +1376,64 @@ const tests = {

1376 }

1377 `,

1378 },

1379 + {

1380 + code: normalizeIndent`

1381 + function useFoo(foo){

1382 + return useMemo(() => foo, [foo]);

1383 + }

1384 + `,

1385 + },

1386 + {

1387 + code: normalizeIndent`

1388 + function useFoo(){

1389 + const foo = "hi!";

1390 + return useMemo(() => foo, [foo]);

1391 + }

1392 + `,

1393 + },

1394 + {

1395 + code: normalizeIndent`

1396 + function useFoo(){

1397 + let {foo} = {foo: 1};

1398 + return useMemo(() => foo, [foo]);

1399 + }

1400 + `,

1401 + },

1402 + {

1403 + code: normalizeIndent`

1404 + function useFoo(){

1405 + let [foo] = [1];

1406 + return useMemo(() => foo, [foo]);

1407 + }

1408 + `,

1409 + },

1410 + {

1411 + code: normalizeIndent`

1412 + function useFoo() {

1413 + const foo = "fine";

1414 + if (true) {

1415 + // Shadowed variable with constant construction in a nested scope is fine.

1416 + const foo = {};

1417 + }

1418 + return useMemo(() => foo, [foo]);

1419 + }

1420 + `,

1421 + },

1422 + {

1423 + code: normalizeIndent`

1424 + function MyComponent({foo}) {

1425 + return useMemo(() => foo, [foo])

1426 + }

1427 + `,

1428 + },

1429 + {

1430 + code: normalizeIndent`

1431 + function MyComponent() {

1432 + const foo = true ? "fine" : "also fine";

1433 + return useMemo(() => foo, [foo]);

1434 + }

1435 + `,

1436 + },

1437 ],

1438 invalid: [

1439 {

@@ -1494,7 +1552,7 @@ const tests = {

1552 {

1553 code: normalizeIndent`

1554 function MyComponent() {

1497 - const local = {};

1555 + const local = someFunc();

1556 useEffect(() => {

1557 console.log(local);

1558 }, []);

@@ -1510,7 +1568,7 @@ const tests = {

1568 desc: 'Update the dependencies array to be: [local]',

1569 output: normalizeIndent`

1570 function MyComponent() {

1513 - const local = {};

1571 + const local = someFunc();

1572 useEffect(() => {

1573 console.log(local);

1574 }, [local]);

@@ -1636,7 +1694,7 @@ const tests = {

1694 // Regression test

1695 code: normalizeIndent`

1696 function MyComponent() {

1639 - const local = {};

1697 + const local = someFunc();

1698 useEffect(() => {

1699 if (true) {

1700 console.log(local);

@@ -1654,7 +1712,7 @@ const tests = {

1712 desc: 'Update the dependencies array to be: [local]',

1713 output: normalizeIndent`

1714 function MyComponent() {

1657 - const local = {};

1715 + const local = someFunc();

1716 useEffect(() => {

1717 if (true) {

1718 console.log(local);

@@ -1742,9 +1800,9 @@ const tests = {

1800 {

1801 code: normalizeIndent`

1802 function MyComponent() {

1745 - const local1 = {};

1803 + const local1 = someFunc();

1804 {

1747 - const local2 = {};

1805 + const local2 = someFunc();

1806 useEffect(() => {

1807 console.log(local1);

1808 console.log(local2);

@@ -1762,9 +1820,9 @@ const tests = {

1820 desc: 'Update the dependencies array to be: [local1, local2]',

1821 output: normalizeIndent`

1822 function MyComponent() {

1765 - const local1 = {};

1823 + const local1 = someFunc();

1824 {

1767 - const local2 = {};

1825 + const local2 = someFunc();

1826 useEffect(() => {

1827 console.log(local1);

1828 console.log(local2);

@@ -1846,7 +1904,7 @@ const tests = {

1904 {

1905 code: normalizeIndent`

1906 function MyComponent() {

1849 - const local1 = {};

1907 + const local1 = someFunc();

1908 function MyNestedComponent() {

1909 const local2 = {};

1910 useCallback(() => {

@@ -1868,7 +1926,7 @@ const tests = {

1926 desc: 'Update the dependencies array to be: [local2]',

1927 output: normalizeIndent`

1928 function MyComponent() {

1871 - const local1 = {};

1929 + const local1 = someFunc();

1930 function MyNestedComponent() {

1931 const local2 = {};

1932 useCallback(() => {

@@ -2295,7 +2353,7 @@ const tests = {

2353 {

2354 code: normalizeIndent`

2355 function MyComponent() {

2298 - const local = {};

2356 + const local = someFunc();

2357 useEffect(() => {

2358 console.log(local);

2359 }, [local, ...dependencies]);

@@ -5311,7 +5369,7 @@ const tests = {

5369 `The 'handleNext' function makes the dependencies of ` +

5370 `useEffect Hook (at line 11) change on every render. ` +

5371 `Move it inside the useEffect callback. Alternatively, ` +

5314 - `wrap the 'handleNext' definition into its own useCallback() Hook.`,

5372 + `wrap the definition of 'handleNext' in its own useCallback() Hook.`,

5373 // Not gonna fix a function definition

5374 // because it's not always safe due to hoisting.

5375 suggestions: undefined,

@@ -5340,7 +5398,7 @@ const tests = {

5398 `The 'handleNext' function makes the dependencies of ` +

5399 `useEffect Hook (at line 11) change on every render. ` +

5400 `Move it inside the useEffect callback. Alternatively, ` +

5343 - `wrap the 'handleNext' definition into its own useCallback() Hook.`,

5401 + `wrap the definition of 'handleNext' in its own useCallback() Hook.`,

5402 // We don't fix moving (too invasive). But that's the suggested fix

5403 // when only effect uses this function. Otherwise, we'd useCallback.

5404 suggestions: undefined,

@@ -5373,13 +5431,13 @@ const tests = {

5431 message:

5432 `The 'handleNext' function makes the dependencies of ` +

5433 `useEffect Hook (at line 11) change on every render. ` +

5376 - `To fix this, wrap the 'handleNext' definition into its own useCallback() Hook.`,

5434 + `To fix this, wrap the definition of 'handleNext' in its own useCallback() Hook.`,

5435 // We fix this one with useCallback since it's

5436 // the easy fix and you can't just move it into effect.

5437 suggestions: [

5438 {

5439 desc:

5382 - "Wrap the 'handleNext' definition into its own useCallback() Hook.",

5440 + "Wrap the definition of 'handleNext' in its own useCallback() Hook.",

5441 output: normalizeIndent`

5442 function MyComponent(props) {

5443 let [, setState] = useState();

@@ -5428,21 +5486,21 @@ const tests = {

5486 message:

5487 "The 'handleNext1' function makes the dependencies of useEffect Hook " +

5488 '(at line 14) change on every render. Move it inside the useEffect callback. ' +

5431 - "Alternatively, wrap the 'handleNext1' definition into its own useCallback() Hook.",

5489 + "Alternatively, wrap the definition of 'handleNext1' in its own useCallback() Hook.",

5490 suggestions: undefined,

5491 },

5492 {

5493 message:

5494 "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " +

5495 '(at line 17) change on every render. Move it inside the useLayoutEffect callback. ' +

5438 - "Alternatively, wrap the 'handleNext2' definition into its own useCallback() Hook.",

5496 + "Alternatively, wrap the definition of 'handleNext2' in its own useCallback() Hook.",

5497 suggestions: undefined,

5498 },

5499 {

5500 message:

5501 "The 'handleNext3' function makes the dependencies of useMemo Hook " +

5502 '(at line 20) change on every render. Move it inside the useMemo callback. ' +

5445 - "Alternatively, wrap the 'handleNext3' definition into its own useCallback() Hook.",

5503 + "Alternatively, wrap the definition of 'handleNext3' in its own useCallback() Hook.",

5504 suggestions: undefined,

5505 },

5506 ],

@@ -5480,21 +5538,21 @@ const tests = {

5538 message:

5539 "The 'handleNext1' function makes the dependencies of useEffect Hook " +

5540 '(at line 15) change on every render. Move it inside the useEffect callback. ' +

5483 - "Alternatively, wrap the 'handleNext1' definition into its own useCallback() Hook.",

5541 + "Alternatively, wrap the definition of 'handleNext1' in its own useCallback() Hook.",

5542 suggestions: undefined,

5543 },

5544 {

5545 message:

5546 "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " +

5547 '(at line 19) change on every render. Move it inside the useLayoutEffect callback. ' +

5490 - "Alternatively, wrap the 'handleNext2' definition into its own useCallback() Hook.",

5548 + "Alternatively, wrap the definition of 'handleNext2' in its own useCallback() Hook.",

5549 suggestions: undefined,

5550 },

5551 {

5552 message:

5553 "The 'handleNext3' function makes the dependencies of useMemo Hook " +

5554 '(at line 23) change on every render. Move it inside the useMemo callback. ' +

5497 - "Alternatively, wrap the 'handleNext3' definition into its own useCallback() Hook.",

5555 + "Alternatively, wrap the definition of 'handleNext3' in its own useCallback() Hook.",

5556 suggestions: undefined,

5557 },

5558 ],

@@ -5541,20 +5599,20 @@ const tests = {

5599 message:

5600 "The 'handleNext1' function makes the dependencies of useEffect Hook " +

5601 '(at line 15) change on every render. To fix this, wrap the ' +

5544 - "'handleNext1' definition into its own useCallback() Hook.",

5602 + "definition of 'handleNext1' in its own useCallback() Hook.",

5603 suggestions: undefined,

5604 },

5605 {

5606 message:

5607 "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " +

5608 '(at line 19) change on every render. To fix this, wrap the ' +

5551 - "'handleNext2' definition into its own useCallback() Hook.",

5609 + "definition of 'handleNext2' in its own useCallback() Hook.",

5610 // Suggestion wraps into useCallback where possible (variables only)

5611 // because they are only referenced outside the effect.

5612 suggestions: [

5613 {

5614 desc:

5557 - "Wrap the 'handleNext2' definition into its own useCallback() Hook.",

5615 + "Wrap the definition of 'handleNext2' in its own useCallback() Hook.",

5616 output: normalizeIndent`

5617 function MyComponent(props) {

5618 function handleNext1() {

@@ -5598,13 +5656,13 @@ const tests = {

5656 message:

5657 "The 'handleNext3' function makes the dependencies of useMemo Hook " +

5658 '(at line 23) change on every render. To fix this, wrap the ' +

5601 - "'handleNext3' definition into its own useCallback() Hook.",

5659 + "definition of 'handleNext3' in its own useCallback() Hook.",

5660 // Autofix wraps into useCallback where possible (variables only)

5661 // because they are only referenced outside the effect.

5662 suggestions: [

5663 {

5664 desc:

5607 - "Wrap the 'handleNext3' definition into its own useCallback() Hook.",

5665 + "Wrap the definition of 'handleNext3' in its own useCallback() Hook.",

5666 output: normalizeIndent`

5667 function MyComponent(props) {

5668 function handleNext1() {

@@ -5675,11 +5733,11 @@ const tests = {

5733 message:

5734 "The 'handleNext1' function makes the dependencies of useEffect Hook " +

5735 '(at line 12) change on every render. To fix this, wrap the ' +

5678 - "'handleNext1' definition into its own useCallback() Hook.",

5736 + "definition of 'handleNext1' in its own useCallback() Hook.",

5737 suggestions: [

5738 {

5739 desc:

5682 - "Wrap the 'handleNext1' definition into its own useCallback() Hook.",

5740 + "Wrap the definition of 'handleNext1' in its own useCallback() Hook.",

5741 output: normalizeIndent`

5742 function MyComponent(props) {

5743 const handleNext1 = useCallback(() => {

@@ -5705,11 +5763,11 @@ const tests = {

5763 message:

5764 "The 'handleNext1' function makes the dependencies of useEffect Hook " +

5765 '(at line 16) change on every render. To fix this, wrap the ' +

5708 - "'handleNext1' definition into its own useCallback() Hook.",

5766 + "definition of 'handleNext1' in its own useCallback() Hook.",

5767 suggestions: [

5768 {

5769 desc:

5712 - "Wrap the 'handleNext1' definition into its own useCallback() Hook.",

5770 + "Wrap the definition of 'handleNext1' in its own useCallback() Hook.",

5771 output: normalizeIndent`

5772 function MyComponent(props) {

5773 const handleNext1 = useCallback(() => {

@@ -5735,14 +5793,14 @@ const tests = {

5793 message:

5794 "The 'handleNext2' function makes the dependencies of useEffect Hook " +

5795 '(at line 12) change on every render. To fix this, wrap the ' +

5738 - "'handleNext2' definition into its own useCallback() Hook.",

5796 + "definition of 'handleNext2' in its own useCallback() Hook.",

5797 suggestions: undefined,

5798 },

5799 {

5800 message:

5801 "The 'handleNext2' function makes the dependencies of useEffect Hook " +

5802 '(at line 16) change on every render. To fix this, wrap the ' +

5745 - "'handleNext2' definition into its own useCallback() Hook.",

5803 + "definition of 'handleNext2' in its own useCallback() Hook.",

5804 suggestions: undefined,

5805 },

5806 ],

@@ -5767,8 +5825,8 @@ const tests = {

5825 {

5826 message:

5827 "The 'handleNext' function makes the dependencies of useEffect Hook " +

5770 - '(at line 13) change on every render. To fix this, wrap the ' +

5771 - "'handleNext' definition into its own useCallback() Hook.",

5828 + '(at line 13) change on every render. To fix this, wrap the definition of ' +

5829 + "'handleNext' in its own useCallback() Hook.",

5830 // Normally we'd suggest moving handleNext inside an

5831 // effect. But it's used more than once.

5832 // TODO: our autofix here isn't quite sufficient because

@@ -5776,7 +5834,7 @@ const tests = {

5834 suggestions: [

5835 {

5836 desc:

5779 - "Wrap the 'handleNext' definition into its own useCallback() Hook.",

5837 + "Wrap the definition of 'handleNext' in its own useCallback() Hook.",

5838 output: normalizeIndent`

5839 function MyComponent(props) {

5840 let handleNext = useCallback(() => {

@@ -5820,7 +5878,7 @@ const tests = {

5878 `The 'handleNext' function makes the dependencies of ` +

5879 `useEffect Hook (at line 14) change on every render. ` +

5880 `Move it inside the useEffect callback. Alternatively, wrap the ` +

5823 - `'handleNext' definition into its own useCallback() Hook.`,

5881 + `definition of 'handleNext' in its own useCallback() Hook.`,

5882 suggestions: undefined,

5883 },

5884 ],

@@ -6085,7 +6143,7 @@ const tests = {

6143 message:

6144 `The 'increment' function makes the dependencies of useEffect Hook ` +

6145 `(at line 14) change on every render. Move it inside the useEffect callback. ` +

6088 - `Alternatively, wrap the \'increment\' definition into its own ` +

6146 + `Alternatively, wrap the definition of \'increment\' in its own ` +

6147 `useCallback() Hook.`,

6148 suggestions: undefined,

6149 },

@@ -6967,6 +7025,499 @@ const tests = {

7025 },

7026 ],

7027 },

7028 + {

7029 + code: normalizeIndent`

7030 + function Component() {

7031 + const foo = {};

7032 + useMemo(() => foo, [foo]);

7033 + }

7034 + `,

7035 + errors: [

7036 + {

7037 + message:

7038 + "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " +

7039 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " +

7040 + 'useMemo() Hook.',

7041 + suggestions: undefined,

7042 + },

7043 + ],

7044 + },

7045 + {

7046 + code: normalizeIndent`

7047 + function Component() {

7048 + const foo = [];

7049 + useMemo(() => foo, [foo]);

7050 + }

7051 + `,

7052 + errors: [

7053 + {

7054 + message:

7055 + "The 'foo' array makes the dependencies of useMemo Hook (at line 4) change on every render. " +

7056 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " +

7057 + 'useMemo() Hook.',

7058 + suggestions: undefined,

7059 + },

7060 + ],

7061 + },

7062 + {

7063 + code: normalizeIndent`

7064 + function Component() {

7065 + const foo = () => {};

7066 + useMemo(() => foo, [foo]);

7067 + }

7068 + `,

7069 + errors: [

7070 + {

7071 + message:

7072 + "The 'foo' function makes the dependencies of useMemo Hook (at line 4) change on every render. " +

7073 + "Move it inside the useMemo callback. Alternatively, wrap the definition of 'foo' in its own " +

7074 + 'useCallback() Hook.',

7075 + suggestions: undefined,

7076 + },

7077 + ],

7078 + },

7079 + {

7080 + code: normalizeIndent`

7081 + function Component() {

7082 + const foo = function bar(){};

7083 + useMemo(() => foo, [foo]);

7084 + }

7085 + `,

7086 + errors: [

7087 + {

7088 + message:

7089 + "The 'foo' function makes the dependencies of useMemo Hook (at line 4) change on every render. " +

7090 + "Move it inside the useMemo callback. Alternatively, wrap the definition of 'foo' in its own " +

7091 + 'useCallback() Hook.',

7092 + suggestions: undefined,

7093 + },

7094 + ],

7095 + },

7096 + {

7097 + code: normalizeIndent`

7098 + function Component() {

7099 + const foo = class {};

7100 + useMemo(() => foo, [foo]);

7101 + }

7102 + `,

7103 + errors: [

7104 + {

7105 + message:

7106 + "The 'foo' class makes the dependencies of useMemo Hook (at line 4) change on every render. " +

7107 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " +

7108 + 'useMemo() Hook.',

7109 + suggestions: undefined,

7110 + },

7111 + ],

7112 + },

7113 + {

7114 + code: normalizeIndent`

7115 + function Component() {

7116 + const foo = true ? {} : "fine";

7117 + useMemo(() => foo, [foo]);

7118 + }

7119 + `,

7120 + errors: [

7121 + {

7122 + message:

7123 + "The 'foo' conditional could make the dependencies of useMemo Hook (at line 4) change on every render. " +

7124 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " +

7125 + 'useMemo() Hook.',

7126 + suggestions: undefined,

7127 + },

7128 + ],

7129 + },

7130 + {

7131 + code: normalizeIndent`

7132 + function Component() {

7133 + const foo = bar || {};

7134 + useMemo(() => foo, [foo]);

7135 + }

7136 + `,

7137 + errors: [

7138 + {

7139 + message:

7140 + "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " +

7141 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " +

7142 + 'useMemo() Hook.',

7143 + suggestions: undefined,

7144 + },

7145 + ],

7146 + },

7147 + {

7148 + code: normalizeIndent`

7149 + function Component() {

7150 + const foo = bar ?? {};

7151 + useMemo(() => foo, [foo]);

7152 + }

7153 + `,

7154 + errors: [

7155 + {

7156 + message:

7157 + "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " +

7158 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " +

7159 + 'useMemo() Hook.',

7160 + suggestions: undefined,

7161 + },

7162 + ],

7163 + },

7164 + {

7165 + code: normalizeIndent`

7166 + function Component() {

7167 + const foo = bar && {};

7168 + useMemo(() => foo, [foo]);

7169 + }

7170 + `,

7171 + errors: [

7172 + {

7173 + message:

7174 + "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " +

7175 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " +

7176 + 'useMemo() Hook.',

7177 + suggestions: undefined,

7178 + },

7179 + ],

7180 + },

7181 + {

7182 + code: normalizeIndent`

7183 + function Component() {

7184 + const foo = bar ? baz ? {} : null : null;

7185 + useMemo(() => foo, [foo]);

7186 + }

7187 + `,

7188 + errors: [

7189 + {

7190 + message:

7191 + "The 'foo' conditional could make the dependencies of useMemo Hook (at line 4) change on every render. " +

7192 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " +

7193 + 'useMemo() Hook.',

7194 + suggestions: undefined,

7195 + },

7196 + ],

7197 + },

7198 + {

7199 + code: normalizeIndent`

7200 + function Component() {

7201 + let foo = {};

7202 + useMemo(() => foo, [foo]);

7203 + }

7204 + `,

7205 + errors: [

7206 + {

7207 + message:

7208 + "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " +

7209 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " +

7210 + 'useMemo() Hook.',

7211 + suggestions: undefined,

7212 + },

7213 + ],

7214 + },

7215 + {

7216 + code: normalizeIndent`

7217 + function Component() {

7218 + var foo = {};

7219 + useMemo(() => foo, [foo]);

7220 + }

7221 + `,

7222 + errors: [

7223 + {

7224 + message:

7225 + "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " +

7226 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " +

7227 + 'useMemo() Hook.',

7228 + suggestions: undefined,

7229 + },

7230 + ],

7231 + },

7232 + {

7233 + code: normalizeIndent`

7234 + function Component() {

7235 + const foo = {};

7236 + useCallback(() => {

7237 + console.log(foo);

7238 + }, [foo]);

7239 + }

7240 + `,

7241 + errors: [

7242 + {

7243 + message:

7244 + "The 'foo' object makes the dependencies of useCallback Hook (at line 6) change on every render. " +

7245 + "Move it inside the useCallback callback. Alternatively, wrap the initialization of 'foo' in its own " +

7246 + 'useMemo() Hook.',

7247 + suggestions: undefined,

7248 + },

7249 + ],

7250 + },

7251 + {

7252 + code: normalizeIndent`

7253 + function Component() {

7254 + const foo = {};

7255 + useEffect(() => {

7256 + console.log(foo);

7257 + }, [foo]);

7258 + }

7259 + `,

7260 + errors: [

7261 + {

7262 + message:

7263 + "The 'foo' object makes the dependencies of useEffect Hook (at line 6) change on every render. " +

7264 + "Move it inside the useEffect callback. Alternatively, wrap the initialization of 'foo' in its own " +

7265 + 'useMemo() Hook.',

7266 + suggestions: undefined,

7267 + },

7268 + ],

7269 + },

7270 + {

7271 + code: normalizeIndent`

7272 + function Component() {

7273 + const foo = {};

7274 + useLayoutEffect(() => {

7275 + console.log(foo);

7276 + }, [foo]);

7277 + }

7278 + `,

7279 + errors: [

7280 + {

7281 + message:

7282 + "The 'foo' object makes the dependencies of useLayoutEffect Hook (at line 6) change on every render. " +

7283 + "Move it inside the useLayoutEffect callback. Alternatively, wrap the initialization of 'foo' in its own " +

7284 + 'useMemo() Hook.',

7285 + suggestions: undefined,

7286 + },

7287 + ],

7288 + },

7289 + {

7290 + code: normalizeIndent`

7291 + function Component() {

7292 + const foo = {};

7293 + useImperativeHandle(

7294 + ref,

7295 + () => {

7296 + console.log(foo);

7297 + },

7298 + [foo]

7299 + );

7300 + }

7301 + `,

7302 + errors: [

7303 + {

7304 + message:

7305 + "The 'foo' object makes the dependencies of useImperativeHandle Hook (at line 9) change on every render. " +

7306 + "Move it inside the useImperativeHandle callback. Alternatively, wrap the initialization of 'foo' in its own " +

7307 + 'useMemo() Hook.',

7308 + suggestions: undefined,

7309 + },

7310 + ],

7311 + },

7312 + {

7313 + code: normalizeIndent`

7314 + function Foo(section) {

7315 + const foo = section.section_components?.edges ?? [];

7316 + useEffect(() => {

7317 + console.log(foo);

7318 + }, [foo]);

7319 + }

7320 + `,

7321 + errors: [

7322 + {

7323 + message:

7324 + "The 'foo' logical expression could make the dependencies of useEffect Hook (at line 6) change on every render. " +

7325 + "Move it inside the useEffect callback. Alternatively, wrap the initialization of 'foo' in its own " +

7326 + 'useMemo() Hook.',

7327 + suggestions: undefined,

7328 + },

7329 + ],

7330 + },

7331 + {

7332 + code: normalizeIndent`

7333 + function Foo(section) {

7334 + const foo = {};

7335 + console.log(foo);

7336 + useMemo(() => {

7337 + console.log(foo);

7338 + }, [foo]);

7339 + }

7340 + `,

7341 + errors: [

7342 + {

7343 + message:

7344 + "The 'foo' object makes the dependencies of useMemo Hook (at line 7) change on every render. " +

7345 + "To fix this, wrap the initialization of 'foo' in its own useMemo() Hook.",

7346 + suggestions: undefined,

7347 + },

7348 + ],

7349 + },

7350 + {

7351 + code: normalizeIndent`

7352 + function Foo() {

7353 + const foo = <>Hi!</>;

7354 + useMemo(() => {

7355 + console.log(foo);

7356 + }, [foo]);

7357 + }

7358 + `,

7359 + errors: [

7360 + {

7361 + message:

7362 + "The 'foo' JSX fragment makes the dependencies of useMemo Hook (at line 6) change on every render. " +

7363 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.",

7364 + suggestions: undefined,

7365 + },

7366 + ],

7367 + },

7368 + {

7369 + code: normalizeIndent`

7370 + function Foo() {

7371 + const foo = <div>Hi!</div>;

7372 + useMemo(() => {

7373 + console.log(foo);

7374 + }, [foo]);

7375 + }

7376 + `,

7377 + errors: [

7378 + {

7379 + message:

7380 + "The 'foo' JSX element makes the dependencies of useMemo Hook (at line 6) change on every render. " +

7381 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.",

7382 + suggestions: undefined,

7383 + },

7384 + ],

7385 + },

7386 + {

7387 + code: normalizeIndent`

7388 + function Foo() {

7389 + const foo = bar = {};

7390 + useMemo(() => {

7391 + console.log(foo);

7392 + }, [foo]);

7393 + }

7394 + `,

7395 + errors: [

7396 + {

7397 + message:

7398 + "The 'foo' assignment expression makes the dependencies of useMemo Hook (at line 6) change on every render. " +

7399 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.",

7400 + suggestions: undefined,

7401 + },

7402 + ],

7403 + },

7404 + {

7405 + code: normalizeIndent`

7406 + function Foo() {

7407 + const foo = new String('foo'); // Note 'foo' will be boxed, and thus an object and thus compared by reference.

7408 + useMemo(() => {

7409 + console.log(foo);

7410 + }, [foo]);

7411 + }

7412 + `,

7413 + errors: [

7414 + {

7415 + message:

7416 + "The 'foo' object construction makes the dependencies of useMemo Hook (at line 6) change on every render. " +

7417 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.",

7418 + suggestions: undefined,

7419 + },

7420 + ],

7421 + },

7422 + {

7423 + code: normalizeIndent`

7424 + function Foo() {

7425 + const foo = new Map([]);

7426 + useMemo(() => {

7427 + console.log(foo);

7428 + }, [foo]);

7429 + }

7430 + `,

7431 + errors: [

7432 + {

7433 + message:

7434 + "The 'foo' object construction makes the dependencies of useMemo Hook (at line 6) change on every render. " +

7435 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.",

7436 + suggestions: undefined,

7437 + },

7438 + ],

7439 + },

7440 + {

7441 + code: normalizeIndent`

7442 + function Foo() {

7443 + const foo = /reg/;

7444 + useMemo(() => {

7445 + console.log(foo);

7446 + }, [foo]);

7447 + }

7448 + `,

7449 + errors: [

7450 + {

7451 + message:

7452 + "The 'foo' regular expression makes the dependencies of useMemo Hook (at line 6) change on every render. " +

7453 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.",

7454 + suggestions: undefined,

7455 + },

7456 + ],

7457 + },

7458 + {

7459 + code: normalizeIndent`

7460 + function Foo() {

7461 + const foo = ({}: any);

7462 + useMemo(() => {

7463 + console.log(foo);

7464 + }, [foo]);

7465 + }

7466 + `,

7467 + errors: [

7468 + {

7469 + message:

7470 + "The 'foo' object makes the dependencies of useMemo Hook (at line 6) change on every render. " +

7471 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.",

7472 + suggestions: undefined,

7473 + },

7474 + ],

7475 + },

7476 + {

7477 + code: normalizeIndent`

7478 + function Foo() {

7479 + class Bar {};

7480 + useMemo(() => {

7481 + console.log(new Bar());

7482 + }, [Bar]);

7483 + }

7484 + `,

7485 + errors: [

7486 + {

7487 + message:

7488 + "The 'Bar' class makes the dependencies of useMemo Hook (at line 6) change on every render. " +

7489 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'Bar' in its own useMemo() Hook.",

7490 + suggestions: undefined,

7491 + },

7492 + ],

7493 + },

7494 + {

7495 + code: normalizeIndent`

7496 + function Foo() {

7497 + const foo = {};

7498 + useLayoutEffect(() => {

7499 + console.log(foo);

7500 + }, [foo]);

7501 + useEffect(() => {

7502 + console.log(foo);

7503 + }, [foo]);

7504 + }

7505 + `,

7506 + errors: [

7507 + {

7508 + message:

7509 + "The 'foo' object makes the dependencies of useLayoutEffect Hook (at line 6) change on every render. " +

7510 + "To fix this, wrap the initialization of 'foo' in its own useMemo() Hook.",

7511 + suggestions: undefined,

7512 + },

7513 + {

7514 + message:

7515 + "The 'foo' object makes the dependencies of useEffect Hook (at line 9) change on every render. " +

7516 + "To fix this, wrap the initialization of 'foo' in its own useMemo() Hook.",

7517 + suggestions: undefined,

7518 + },

7519 + ],

7520 + },

7521 ],

7522 };

7523

@@ -7345,6 +7896,24 @@ const testsTypescript = {

7896 },

7897 ],

7898 },

7899 + {

7900 + code: normalizeIndent`

7901 + function Foo() {

7902 + const foo = {} as any;

7903 + useMemo(() => {

7904 + console.log(foo);

7905 + }, [foo]);

7906 + }

7907 + `,

7908 + errors: [

7909 + {

7910 + message:

7911 + "The 'foo' object makes the dependencies of useMemo Hook (at line 6) change on every render. " +

7912 + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.",

7913 + suggestions: undefined,

7914 + },

7915 + ],

7916 + },

7917 ],

7918 };

7919

packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js

+156 -68

@@ -844,59 +844,80 @@ export default {

844 unnecessaryDependencies.size;

845

846 if (problemCount === 0) {

847 - // If nothing else to report, check if some callbacks

848 - // are bare and would invalidate on every render.

849 - const bareFunctions = scanForDeclaredBareFunctions({

847 + // If nothing else to report, check if some dependencies would

848 + // invalidate on every render.

849 + const constructions = scanForConstructions({

850 declaredDependencies,

851 declaredDependenciesNode,

852 componentScope,

853 scope,

854 });

855 - bareFunctions.forEach(({fn, suggestUseCallback}) => {

856 - let message =

857 - `The '${fn.name.name}' function makes the dependencies of ` +

858 - `${reactiveHookName} Hook (at line ${declaredDependenciesNode.loc.start.line}) ` +

859 - `change on every render.`;

860 - if (suggestUseCallback) {

861 - message +=

862 - ` To fix this, ` +

863 - `wrap the '${fn.name.name}' definition into its own useCallback() Hook.`;

864 - } else {

865 - message +=

866 - ` Move it inside the ${reactiveHookName} callback. ` +

867 - `Alternatively, wrap the '${fn.name.name}' definition into its own useCallback() Hook.`;

868 - }

855 + constructions.forEach(

856 + ({construction, isUsedOutsideOfHook, depType}) => {

857 + const wrapperHook =

858 + depType === 'function' ? 'useCallback' : 'useMemo';

859

870 - let suggest;

871 - // Only handle the simple case: arrow functions.

872 - // Wrapping function declarations can mess up hoisting.

873 - if (suggestUseCallback && fn.type === 'Variable') {

874 - suggest = [

875 - {

876 - desc: `Wrap the '${fn.name.name}' definition into its own useCallback() Hook.`,

877 - fix(fixer) {

878 - return [

879 - // TODO: also add an import?

880 - fixer.insertTextBefore(fn.node.init, 'useCallback('),

881 - // TODO: ideally we'd gather deps here but it would require

882 - // restructuring the rule code. This will cause a new lint

883 - // error to appear immediately for useCallback. Note we're

884 - // not adding [] because would that changes semantics.

885 - fixer.insertTextAfter(fn.node.init, ')'),

886 - ];

860 + const constructionType =

861 + depType === 'function' ? 'definition' : 'initialization';

862 +

863 + const defaultAdvice = `wrap the ${constructionType} of '${construction.name.name}' in its own ${wrapperHook}() Hook.`;

864 +

865 + const advice = isUsedOutsideOfHook

866 + ? `To fix this, ${defaultAdvice}`

867 + : `Move it inside the ${reactiveHookName} callback. Alternatively, ${defaultAdvice}`;

868 +

869 + const causation =

870 + depType === 'conditional' || depType === 'logical expression'

871 + ? 'could make'

872 + : 'makes';

873 +

874 + const message =

875 + `The '${construction.name.name}' ${depType} ${causation} the dependencies of ` +

876 + `${reactiveHookName} Hook (at line ${declaredDependenciesNode.loc.start.line}) ` +

877 + `change on every render. ${advice}`;

878 +

879 + let suggest;

880 + // Only handle the simple case of variable assignments.

881 + // Wrapping function declarations can mess up hoisting.

882 + if (

883 + isUsedOutsideOfHook &&

884 + construction.type === 'Variable' &&

885 + // Objects may be mutated ater construction, which would make this

886 + // fix unsafe. Functions _probably_ won't be mutated, so we'll

887 + // allow this fix for them.

888 + depType === 'function'

889 + ) {

890 + suggest = [

891 + {

892 + desc: `Wrap the ${constructionType} of '${construction.name.name}' in its own ${wrapperHook}() Hook.`,

893 + fix(fixer) {

894 + const [before, after] =

895 + wrapperHook === 'useMemo'

896 + ? [`useMemo(() => { return `, '; })']

897 + : ['useCallback(', ')'];

898 + return [

899 + // TODO: also add an import?

900 + fixer.insertTextBefore(construction.node.init, before),

901 + // TODO: ideally we'd gather deps here but it would require

902 + // restructuring the rule code. This will cause a new lint

903 + // error to appear immediately for useCallback. Note we're

904 + // not adding [] because would that changes semantics.

905 + fixer.insertTextAfter(construction.node.init, after),

906 + ];

907 + },

908 },

888 - },

889 - ];

890 - }

891 - // TODO: What if the function needs to change on every render anyway?

892 - // Should we suggest removing effect deps as an appropriate fix too?

893 - reportProblem({

894 - // TODO: Why not report this at the dependency site?

895 - node: fn.node,

896 - message,

897 - suggest,

898 - });

899 - });

909 + ];

910 + }

911 + // TODO: What if the function needs to change on every render anyway?

912 + // Should we suggest removing effect deps as an appropriate fix too?

913 + reportProblem({

914 + // TODO: Why not report this at the dependency site?

915 + node: construction.node,

916 + message,

917 + suggest,

918 + });

919 + },

920 + );

921 return;

922 }

923

@@ -1381,50 +1402,116 @@ function collectRecommendations({

1402 };

1403 }

1404

1384 -// Finds functions declared as dependencies

1405 +// If the node will result in constructing a referentially unique value, return

1406 +// its human readable type name, else return null.

1407 +function getConstructionExpressionType(node) {

1408 + switch (node.type) {

1409 + case 'ObjectExpression':

1410 + return 'object';

1411 + case 'ArrayExpression':

1412 + return 'array';

1413 + case 'ArrowFunctionExpression':

1414 + case 'FunctionExpression':

1415 + return 'function';

1416 + case 'ClassExpression':

1417 + return 'class';

1418 + case 'ConditionalExpression':

1419 + if (

1420 + getConstructionExpressionType(node.consequent) != null ||

1421 + getConstructionExpressionType(node.alternate) != null

1422 + ) {

1423 + return 'conditional';

1424 + }

1425 + return null;

1426 + case 'LogicalExpression':

1427 + if (

1428 + getConstructionExpressionType(node.left) != null ||

1429 + getConstructionExpressionType(node.right) != null

1430 + ) {

1431 + return 'logical expression';

1432 + }

1433 + return null;

1434 + case 'JSXFragment':

1435 + return 'JSX fragment';

1436 + case 'JSXElement':

1437 + return 'JSX element';

1438 + case 'AssignmentExpression':

1439 + if (getConstructionExpressionType(node.right) != null) {

1440 + return 'assignment expression';

1441 + }

1442 + return null;

1443 + case 'NewExpression':

1444 + return 'object construction';

1445 + case 'Literal':

1446 + if (node.value instanceof RegExp) {

1447 + return 'regular expression';

1448 + }

1449 + return null;

1450 + case 'TypeCastExpression':

1451 + return getConstructionExpressionType(node.expression);

1452 + case 'TSAsExpression':

1453 + return getConstructionExpressionType(node.expression);

1454 + }

1455 + return null;

1456 +}

1457 +

1458 +// Finds variables declared as dependencies

1459 // that would invalidate on every render.

1386 -function scanForDeclaredBareFunctions({

1460 +function scanForConstructions({

1461 declaredDependencies,

1462 declaredDependenciesNode,

1463 componentScope,

1464 scope,

1465 }) {

1392 - const bareFunctions = declaredDependencies

1466 + const constructions = declaredDependencies

1467 .map(({key}) => {

1394 - const fnRef = componentScope.set.get(key);

1395 - if (fnRef == null) {

1468 + const ref = componentScope.variables.find(v => v.name === key);

1469 + if (ref == null) {

1470 return null;

1471 }

1398 - const fnNode = fnRef.defs[0];

1399 - if (fnNode == null) {

1472 +

1473 + const node = ref.defs[0];

1474 + if (node == null) {

1475 return null;

1476 }

1477 // const handleChange = function () {}

1478 // const handleChange = () => {}

1479 + // const foo = {}

1480 + // const foo = []

1481 + // etc.

1482 if (

1405 - fnNode.type === 'Variable' &&

1406 - fnNode.node.type === 'VariableDeclarator' &&

1407 - fnNode.node.init != null &&

1408 - (fnNode.node.init.type === 'ArrowFunctionExpression' ||

1409 - fnNode.node.init.type === 'FunctionExpression')

1483 + node.type === 'Variable' &&

1484 + node.node.type === 'VariableDeclarator' &&

1485 + node.node.id.type === 'Identifier' && // Ensure this is not destructed assignment

1486 + node.node.init != null

1487 ) {

1411 - return fnRef;

1488 + const constantExpressionType = getConstructionExpressionType(

1489 + node.node.init,

1490 + );

1491 + if (constantExpressionType != null) {

1492 + return [ref, constantExpressionType];

1493 + }

1494 }

1495 // function handleChange() {}

1496 if (

1415 - fnNode.type === 'FunctionName' &&

1416 - fnNode.node.type === 'FunctionDeclaration'

1497 + node.type === 'FunctionName' &&

1498 + node.node.type === 'FunctionDeclaration'

1499 ) {

1418 - return fnRef;

1500 + return [ref, 'function'];

1501 + }

1502 +

1503 + // class Foo {}

1504 + if (node.type === 'ClassName' && node.node.type === 'ClassDeclaration') {

1505 + return [ref, 'class'];

1506 }

1507 return null;

1508 })

1509 .filter(Boolean);

1510

1424 - function isUsedOutsideOfHook(fnRef) {

1511 + function isUsedOutsideOfHook(ref) {

1512 let foundWriteExpr = false;

1426 - for (let i = 0; i < fnRef.references.length; i++) {

1427 - const reference = fnRef.references[i];

1513 + for (let i = 0; i < ref.references.length; i++) {

1514 + const reference = ref.references[i];

1515 if (reference.writeExpr) {

1516 if (foundWriteExpr) {

1517 // Two writes to the same function.

@@ -1450,9 +1537,10 @@ function scanForDeclaredBareFunctions({

1537 return false;

1538 }

1539

1453 - return bareFunctions.map(fnRef => ({

1454 - fn: fnRef.defs[0],

1455 - suggestUseCallback: isUsedOutsideOfHook(fnRef),

1540 + return constructions.map(([ref, depType]) => ({

1541 + construction: ref.defs[0],

1542 + depType,

1543 + isUsedOutsideOfHook: isUsedOutsideOfHook(ref),

1544 }));

1545 }

1546