@samitouri / QOS-React / commits / 0b8efb229c

Allow omitting constant primitive deps (#14959)

Dan Abramov committed Feb 26, 2019 at 16:12 UTC 0b8efb229c0b8e4b0919d855e926c7528e2246f0

2 files changed +215 -81

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

+191 -78

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

34 {

35 code: `

36 function MyComponent() {

37 - const local = 42;

37 + const local = {};

38 useEffect(() => {

39 console.log(local);

40 });

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

45 code: `

46 function MyComponent() {

47 useEffect(() => {

48 - const local = 42;

48 + const local = {};

49 console.log(local);

50 }, []);

51 }

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

54 {

55 code: `

56 function MyComponent() {

57 - const local = 42;

57 + const local = {};

58 useEffect(() => {

59 console.log(local);

60 }, [local]);

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

78 {

79 code: `

80 function MyComponent() {

81 - const local1 = 42;

81 + const local1 = {};

82 {

83 - const local2 = 42;

83 + const local2 = {};

84 useEffect(() => {

85 console.log(local1);

86 console.log(local2);

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

92 {

93 code: `

94 function MyComponent() {

95 - const local1 = 42;

95 + const local1 = {};

96 {

97 - const local2 = 42;

97 + const local2 = {};

98 useEffect(() => {

99 console.log(local1);

100 console.log(local2);

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

106 {

107 code: `

108 function MyComponent() {

109 - const local1 = 42;

109 + const local1 = {};

110 function MyNestedComponent() {

111 - const local2 = 42;

111 + const local2 = {};

112 useEffect(() => {

113 console.log(local1);

114 console.log(local2);

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

120 {

121 code: `

122 function MyComponent() {

123 - const local = 42;

123 + const local = {};

124 useEffect(() => {

125 console.log(local);

126 console.log(local);

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

140 {

141 code: `

142 function MyComponent() {

143 - const local = 42;

143 + const local = {};

144 useEffect(() => {

145 console.log(local);

146 }, [,,,local,,,]);

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

214 // TODO: we might want to forbid dot-access in deps.

215 code: `

216 function MyComponent(props) {

217 - const local = 42;

217 + const local = {};

218 useEffect(() => {

219 console.log(props.foo);

220 console.log(props.bar);

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

228 // is extraneous because we already have "props".

229 code: `

230 function MyComponent(props) {

231 - const local = 42;

231 + const local = {};

232 useEffect(() => {

233 console.log(props.foo);

234 console.log(props.bar);

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

260 {

261 code: `

262 // Valid because we don't care about hooks outside of components.

263 - const local = 42;

263 + const local = {};

264 useEffect(() => {

265 console.log(local);

266 }, []);

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

269 {

270 code: `

271 // Valid because we don't care about hooks outside of components.

272 - const local1 = 42;

272 + const local1 = {};

273 {

274 - const local2 = 42;

274 + const local2 = {};

275 useEffect(() => {

276 console.log(local1);

277 console.log(local2);

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

455 const myRef = useRef();

456 useEffect(() => {

457 const handleMove = () => {};

458 - myRef.current = 42;

458 + myRef.current = {};

459 return () => {

460 console.log(myRef.current.toString())

461 };

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

472 function useMyThing(myRef) {

473 useEffect(() => {

474 const handleMove = () => {};

475 - myRef.current = 42;

475 + myRef.current = {};

476 return () => {

477 console.log(myRef.current.toString())

478 };

@@ -560,12 +560,42 @@ const tests = {

560 }

561 `,

562 },

563 + {

564 + // Valid because it's a primitive constant

565 + code: `

566 + function MyComponent() {

567 + const local1 = 42;

568 + const local2 = '42';

569 + const local3 = null;

570 + useEffect(() => {

571 + console.log(local1);

572 + console.log(local2);

573 + console.log(local3);

574 + }, []);

575 + }

576 + `,

577 + },

578 + {

579 + // It's not a mistake to specify constant values though.

580 + code: `

581 + function MyComponent() {

582 + const local1 = 42;

583 + const local2 = '42';

584 + const local3 = null;

585 + useEffect(() => {

586 + console.log(local1);

587 + console.log(local2);

588 + console.log(local3);

589 + }, [local1, local2, local3]);

590 + }

591 + `,

592 + },

593 ],

594 invalid: [

595 {

596 code: `

597 function MyComponent() {

568 - const local = 42;

598 + const local = {};

599 useEffect(() => {

600 console.log(local);

601 }, []);

@@ -573,7 +603,55 @@ const tests = {

603 `,

604 output: `

605 function MyComponent() {

576 - const local = 42;

606 + const local = {};

607 + useEffect(() => {

608 + console.log(local);

609 + }, [local]);

610 + }

611 + `,

612 + errors: [

613 + "React Hook useEffect has a missing dependency: 'local'. " +

614 + 'Either include it or remove the dependency array.',

615 + ],

616 + },

617 + {

618 + // Note: we *could* detect it's a primitive and never assigned

619 + // even though it's not a constant -- but we currently don't.

620 + // So this is an error.

621 + code: `

622 + function MyComponent() {

623 + let local = 42;

624 + useEffect(() => {

625 + console.log(local);

626 + }, []);

627 + }

628 + `,

629 + output: `

630 + function MyComponent() {

631 + let local = 42;

632 + useEffect(() => {

633 + console.log(local);

634 + }, [local]);

635 + }

636 + `,

637 + errors: [

638 + "React Hook useEffect has a missing dependency: 'local'. " +

639 + 'Either include it or remove the dependency array.',

640 + ],

641 + },

642 + {

643 + // Regexes are literals but potentially stateful.

644 + code: `

645 + function MyComponent() {

646 + const local = /foo/;

647 + useEffect(() => {

648 + console.log(local);

649 + }, []);

650 + }

651 + `,

652 + output: `

653 + function MyComponent() {

654 + const local = /foo/;

655 useEffect(() => {

656 console.log(local);

657 }, [local]);

@@ -588,7 +666,7 @@ const tests = {

666 code: `

667 // Regression test

668 function MyComponent() {

591 - const local = 42;

669 + const local = {};

670 useEffect(() => {

671 if (true) {

672 console.log(local);

@@ -599,7 +677,7 @@ const tests = {

677 output: `

678 // Regression test

679 function MyComponent() {

602 - const local = 42;

680 + const local = {};

681 useEffect(() => {

682 if (true) {

683 console.log(local);

@@ -616,7 +694,7 @@ const tests = {

694 code: `

695 // Regression test

696 function MyComponent() {

619 - const local = 42;

697 + const local = {};

698 useEffect(() => {

699 try {

700 console.log(local);

@@ -627,7 +705,7 @@ const tests = {

705 output: `

706 // Regression test

707 function MyComponent() {

630 - const local = 42;

708 + const local = {};

709 useEffect(() => {

710 try {

711 console.log(local);

@@ -644,7 +722,7 @@ const tests = {

722 code: `

723 // Regression test

724 function MyComponent() {

647 - const local = 42;

725 + const local = {};

726 useEffect(() => {

727 function inner() {

728 console.log(local);

@@ -656,7 +734,7 @@ const tests = {

734 output: `

735 // Regression test

736 function MyComponent() {

659 - const local = 42;

737 + const local = {};

738 useEffect(() => {

739 function inner() {

740 console.log(local);

@@ -673,9 +751,9 @@ const tests = {

751 {

752 code: `

753 function MyComponent() {

676 - const local1 = 42;

754 + const local1 = {};

755 {

678 - const local2 = 42;

756 + const local2 = {};

757 useEffect(() => {

758 console.log(local1);

759 console.log(local2);

@@ -685,9 +763,9 @@ const tests = {

763 `,

764 output: `

765 function MyComponent() {

688 - const local1 = 42;

766 + const local1 = {};

767 {

690 - const local2 = 42;

768 + const local2 = {};

769 useEffect(() => {

770 console.log(local1);

771 console.log(local2);

@@ -703,8 +781,8 @@ const tests = {

781 {

782 code: `

783 function MyComponent() {

706 - const local1 = 42;

707 - const local2 = 42;

784 + const local1 = {};

785 + const local2 = {};

786 useEffect(() => {

787 console.log(local1);

788 console.log(local2);

@@ -713,8 +791,8 @@ const tests = {

791 `,

792 output: `

793 function MyComponent() {

716 - const local1 = 42;

717 - const local2 = 42;

794 + const local1 = {};

795 + const local2 = {};

796 useEffect(() => {

797 console.log(local1);

798 console.log(local2);

@@ -729,8 +807,8 @@ const tests = {

807 {

808 code: `

809 function MyComponent() {

732 - const local1 = 42;

733 - const local2 = 42;

810 + const local1 = {};

811 + const local2 = {};

812 useEffect(() => {

813 console.log(local1);

814 }, [local1, local2]);

@@ -738,8 +816,8 @@ const tests = {

816 `,

817 output: `

818 function MyComponent() {

741 - const local1 = 42;

742 - const local2 = 42;

819 + const local1 = {};

820 + const local2 = {};

821 useEffect(() => {

822 console.log(local1);

823 }, [local1]);

@@ -755,9 +833,9 @@ const tests = {

833 // Maybe it should not consider local1 unused despite component nesting?

834 code: `

835 function MyComponent() {

758 - const local1 = 42;

836 + const local1 = {};

837 function MyNestedComponent() {

760 - const local2 = 42;

838 + const local2 = {};

839 useEffect(() => {

840 console.log(local1);

841 console.log(local2);

@@ -767,9 +845,9 @@ const tests = {

845 `,

846 output: `

847 function MyComponent() {

770 - const local1 = 42;

848 + const local1 = {};

849 function MyNestedComponent() {

772 - const local2 = 42;

850 + const local2 = {};

851 useEffect(() => {

852 console.log(local1);

853 console.log(local2);

@@ -786,7 +864,7 @@ const tests = {

864 {

865 code: `

866 function MyComponent() {

789 - const local = 42;

867 + const local = {};

868 useEffect(() => {

869 console.log(local);

870 console.log(local);

@@ -795,7 +873,7 @@ const tests = {

873 `,

874 output: `

875 function MyComponent() {

798 - const local = 42;

876 + const local = {};

877 useEffect(() => {

878 console.log(local);

879 console.log(local);

@@ -810,7 +888,7 @@ const tests = {

888 {

889 code: `

890 function MyComponent() {

813 - const local = 42;

891 + const local = {};

892 useEffect(() => {

893 console.log(local);

894 console.log(local);

@@ -819,7 +897,7 @@ const tests = {

897 `,

898 output: `

899 function MyComponent() {

822 - const local = 42;

900 + const local = {};

901 useEffect(() => {

902 console.log(local);

903 console.log(local);

@@ -962,7 +1040,7 @@ const tests = {

1040 {

1041 code: `

1042 function MyComponent() {

965 - const local = 42;

1043 + const local = {};

1044 const dependencies = [local];

1045 useEffect(() => {

1046 console.log(local);

@@ -972,7 +1050,7 @@ const tests = {

1050 // TODO: should this autofix or bail out?

1051 output: `

1052 function MyComponent() {

975 - const local = 42;

1053 + const local = {};

1054 const dependencies = [local];

1055 useEffect(() => {

1056 console.log(local);

@@ -990,7 +1068,7 @@ const tests = {

1068 {

1069 code: `

1070 function MyComponent() {

993 - const local = 42;

1071 + const local = {};

1072 const dependencies = [local];

1073 useEffect(() => {

1074 console.log(local);

@@ -1000,7 +1078,7 @@ const tests = {

1078 // TODO: should this autofix or bail out?

1079 output: `

1080 function MyComponent() {

1003 - const local = 42;

1081 + const local = {};

1082 const dependencies = [local];

1083 useEffect(() => {

1084 console.log(local);

@@ -1018,7 +1096,7 @@ const tests = {

1096 {

1097 code: `

1098 function MyComponent() {

1021 - const local = 42;

1099 + const local = {};

1100 useEffect(() => {

1101 console.log(local);

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

@@ -1026,7 +1104,7 @@ const tests = {

1104 `,

1105 output: `

1106 function MyComponent() {

1029 - const local = 42;

1107 + const local = {};

1108 useEffect(() => {

1109 console.log(local);

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

@@ -1041,7 +1119,7 @@ const tests = {

1119 {

1120 code: `

1121 function MyComponent() {

1044 - const local = 42;

1122 + const local = {};

1123 useEffect(() => {

1124 console.log(local);

1125 }, [computeCacheKey(local)]);

@@ -1051,7 +1129,7 @@ const tests = {

1129 // Maybe bail out?

1130 output: `

1131 function MyComponent() {

1054 - const local = 42;

1132 + const local = {};

1133 useEffect(() => {

1134 console.log(local);

1135 }, [local]);

@@ -1177,7 +1255,7 @@ const tests = {

1255 {

1256 code: `

1257 function MyComponent() {

1180 - const local = 42;

1258 + const local = {};

1259 useEffect(() => {

1260 console.log(local);

1261 }, [local, local]);

@@ -1185,7 +1263,7 @@ const tests = {

1263 `,

1264 output: `

1265 function MyComponent() {

1188 - const local = 42;

1266 + const local = {};

1267 useEffect(() => {

1268 console.log(local);

1269 }, [local]);

@@ -1199,18 +1277,18 @@ const tests = {

1277 {

1278 code: `

1279 function MyComponent() {

1202 - const local1 = 42;

1280 + const local1 = {};

1281 useEffect(() => {

1204 - const local1 = 42;

1282 + const local1 = {};

1283 console.log(local1);

1284 }, [local1]);

1285 }

1286 `,

1287 output: `

1288 function MyComponent() {

1211 - const local1 = 42;

1289 + const local1 = {};

1290 useEffect(() => {

1213 - const local1 = 42;

1291 + const local1 = {};

1292 console.log(local1);

1293 }, []);

1294 }

@@ -1223,13 +1301,13 @@ const tests = {

1301 {

1302 code: `

1303 function MyComponent() {

1226 - const local1 = 42;

1304 + const local1 = {};

1305 useEffect(() => {}, [local1]);

1306 }

1307 `,

1308 output: `

1309 function MyComponent() {

1232 - const local1 = 42;

1310 + const local1 = {};

1311 useEffect(() => {}, []);

1312 }

1313 `,

@@ -1356,7 +1434,7 @@ const tests = {

1434 {

1435 code: `

1436 function MyComponent(props) {

1359 - const local = 42;

1437 + const local = {};

1438 useEffect(() => {

1439 console.log(props.foo);

1440 console.log(props.bar);

@@ -1368,7 +1446,7 @@ const tests = {

1446 // Should it capture by default?

1447 output: `

1448 function MyComponent(props) {

1371 - const local = 42;

1449 + const local = {};

1450 useEffect(() => {

1451 console.log(props.foo);

1452 console.log(props.bar);

@@ -1384,7 +1462,7 @@ const tests = {

1462 {

1463 code: `

1464 function MyComponent(props) {

1387 - const local = 42;

1465 + const local = {};

1466 useEffect(() => {

1467 console.log(props.foo);

1468 console.log(props.bar);

@@ -1394,7 +1472,7 @@ const tests = {

1472 `,

1473 output: `

1474 function MyComponent(props) {

1397 - const local = 42;

1475 + const local = {};

1476 useEffect(() => {

1477 console.log(props.foo);

1478 console.log(props.bar);

@@ -1519,7 +1597,7 @@ const tests = {

1597 {

1598 code: `

1599 function MyComponent() {

1522 - const local = 42;

1600 + const local = {};

1601 useEffect(() => {

1602 console.log(local);

1603 }, [a ? local : b]);

@@ -1528,7 +1606,7 @@ const tests = {

1606 // TODO: should we bail out instead?

1607 output: `

1608 function MyComponent() {

1531 - const local = 42;

1609 + const local = {};

1610 useEffect(() => {

1611 console.log(local);

1612 }, [local]);

@@ -1544,7 +1622,7 @@ const tests = {

1622 {

1623 code: `

1624 function MyComponent() {

1547 - const local = 42;

1625 + const local = {};

1626 useEffect(() => {

1627 console.log(local);

1628 }, [a && local]);

@@ -1553,7 +1631,7 @@ const tests = {

1631 // TODO: should we bail out instead?

1632 output: `

1633 function MyComponent() {

1556 - const local = 42;

1634 + const local = {};

1635 useEffect(() => {

1636 console.log(local);

1637 }, [local]);

@@ -1572,7 +1650,7 @@ const tests = {

1650 const ref = useRef();

1651 const [state, setState] = useState();

1652 useEffect(() => {

1575 - ref.current = 42;

1653 + ref.current = {};

1654 setState(state + 1);

1655 }, []);

1656 }

@@ -1582,7 +1660,7 @@ const tests = {

1660 const ref = useRef();

1661 const [state, setState] = useState();

1662 useEffect(() => {

1585 - ref.current = 42;

1663 + ref.current = {};

1664 setState(state + 1);

1665 }, [state]);

1666 }

@@ -1598,7 +1676,7 @@ const tests = {

1676 const ref = useRef();

1677 const [state, setState] = useState();

1678 useEffect(() => {

1601 - ref.current = 42;

1679 + ref.current = {};

1680 setState(state + 1);

1681 }, [ref]);

1682 }

@@ -1611,7 +1689,7 @@ const tests = {

1689 const ref = useRef();

1690 const [state, setState] = useState();

1691 useEffect(() => {

1614 - ref.current = 42;

1692 + ref.current = {};

1693 setState(state + 1);

1694 }, [ref, state]);

1695 }

@@ -1789,7 +1867,7 @@ const tests = {

1867 let value3;

1868 let asyncValue;

1869 useEffect(() => {

1792 - value = 42;

1870 + value = {};

1871 value2 = 100;

1872 value = 43;

1873 console.log(value2);

@@ -1810,7 +1888,7 @@ const tests = {

1888 let value3;

1889 let asyncValue;

1890 useEffect(() => {

1813 - value = 42;

1891 + value = {};

1892 value2 = 100;

1893 value = 43;

1894 console.log(value2);

@@ -1850,7 +1928,7 @@ const tests = {

1928 let value3;

1929 let asyncValue;

1930 useEffect(() => {

1853 - value = 42;

1931 + value = {};

1932 value2 = 100;

1933 value = 43;

1934 console.log(value2);

@@ -1871,7 +1949,7 @@ const tests = {

1949 let value3;

1950 let asyncValue;

1951 useEffect(() => {

1874 - value = 42;

1952 + value = {};

1953 value2 = 100;

1954 value = 43;

1955 console.log(value2);

@@ -2039,6 +2117,41 @@ const tests = {

2117 `the effect itself and refer to that variable from the cleanup function.`,

2118 ],

2119 },

2120 + {

2121 + // Autofix ignores constant primitives (leaving the ones that are there).

2122 + code: `

2123 + function MyComponent() {

2124 + const local1 = 42;

2125 + const local2 = '42';

2126 + const local3 = null;

2127 + const local4 = {};

2128 + useEffect(() => {

2129 + console.log(local1);

2130 + console.log(local2);

2131 + console.log(local3);

2132 + console.log(local4);

2133 + }, [local1, local3]);

2134 + }

2135 + `,

2136 + output: `

2137 + function MyComponent() {

2138 + const local1 = 42;

2139 + const local2 = '42';

2140 + const local3 = null;

2141 + const local4 = {};

2142 + useEffect(() => {

2143 + console.log(local1);

2144 + console.log(local2);

2145 + console.log(local3);

2146 + console.log(local4);

2147 + }, [local1, local3, local4]);

2148 + }

2149 + `,

2150 + errors: [

2151 + "React Hook useEffect has a missing dependency: 'local4'. " +

2152 + 'Either include it or remove the dependency array.',

2153 + ],

2154 + },

2155 ],

2156 };

2157

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

+24 -3

@@ -577,12 +577,33 @@ function isDefinitelyStaticDependency(reference) {

577 return false;

578 }

579 const def = resolved.defs[0];

580 - if (def == null || def.node.init == null) {

580 + if (def == null) {

581 + return false;

582 + }

583 + // Look for `let stuff = ...`

584 + if (def.node.type !== 'VariableDeclarator') {

585 return false;

586 }

583 - // Look for `let stuff = SomeHook();`

587 const init = def.node.init;

585 - if (init.callee == null) {

588 + if (init == null) {

589 + return false;

590 + }

591 + // Detect primitive constants

592 + // const foo = 42

593 + const declaration = def.node.parent;

594 + if (

595 + declaration.kind === 'const' &&

596 + init.type === 'Literal' &&

597 + (typeof init.value === 'string' ||

598 + typeof init.value === 'number' ||

599 + init.value === null)

600 + ) {

601 + // Definitely static

602 + return true;

603 + }

604 + // Detect known Hook calls

605 + // const [_, setState] = useState()

606 + if (init.type !== 'CallExpression') {

607 return false;

608 }

609 let callee = init.callee;