@samitouri / QOS-React / commits / 197703ecc7

[ESLint] Add more hints to lint messages (#15046)

* A clearer message for props destructuring where applicable * Add line number to the "move function" message * Add a hint for how to fix callbacks from props * Simplify code and harden tests * Collect all dependency references for better warnings * Suggest updater or reducer where appropriate

Dan Abramov committed Mar 7, 2019 at 12:39 UTC 197703ecc776dd5c1a4d956896603bcc67fb9920
2 files changed +744 -44
packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
+546 -22
@@ -849,6 +849,106 @@ const tests = {
849 }
850 `,
851 },
852 + {
853 + code: `
854 + function withFetch(fetchPodcasts) {
855 + return function Podcasts({ id }) {
856 + let [podcasts, setPodcasts] = useState(null);
857 + useEffect(() => {
858 + fetchPodcasts(id).then(setPodcasts);
859 + }, [id]);
860 + }
861 + }
862 + `,
863 + },
864 + {
865 + code: `
866 + function Podcasts({ id }) {
867 + let [podcasts, setPodcasts] = useState(null);
868 + useEffect(() => {
869 + function doFetch({ fetchPodcasts }) {
870 + fetchPodcasts(id).then(setPodcasts);
871 + }
872 + doFetch({ fetchPodcasts: API.fetchPodcasts });
873 + }, [id]);
874 + }
875 + `,
876 + },
877 + {
878 + code: `
879 + function Counter() {
880 + let [count, setCount] = useState(0);
881 +
882 + function increment(x) {
883 + return x + 1;
884 + }
885 +
886 + useEffect(() => {
887 + let id = setInterval(() => {
888 + setCount(increment);
889 + }, 1000);
890 + return () => clearInterval(id);
891 + }, []);
892 +
893 + return <h1>{count}</h1>;
894 + }
895 + `,
896 + },
897 + {
898 + code: `
899 + function Counter() {
900 + let [count, setCount] = useState(0);
901 +
902 + function increment(x) {
903 + return x + 1;
904 + }
905 +
906 + useEffect(() => {
907 + let id = setInterval(() => {
908 + setCount(count => increment(count));
909 + }, 1000);
910 + return () => clearInterval(id);
911 + }, []);
912 +
913 + return <h1>{count}</h1>;
914 + }
915 + `,
916 + },
917 + {
918 + code: `
919 + import increment from './increment';
920 + function Counter() {
921 + let [count, setCount] = useState(0);
922 +
923 + useEffect(() => {
924 + let id = setInterval(() => {
925 + setCount(count => count + increment);
926 + }, 1000);
927 + return () => clearInterval(id);
928 + }, []);
929 +
930 + return <h1>{count}</h1>;
931 + }
932 + `,
933 + },
934 + {
935 + code: `
936 + function withStuff(increment) {
937 + return function Counter() {
938 + let [count, setCount] = useState(0);
939 +
940 + useEffect(() => {
941 + let id = setInterval(() => {
942 + setCount(count => count + increment);
943 + }, 1000);
944 + return () => clearInterval(id);
945 + }, []);
946 +
947 + return <h1>{count}</h1>;
948 + }
949 + }
950 + `,
951 + },
952 ],
953 invalid: [
954 {
@@ -2203,7 +2303,10 @@ const tests = {
2303 `,
2304 errors: [
2305 "React Hook useEffect has a missing dependency: 'state'. " +
2206 - 'Either include it or remove the dependency array.',
2306 + 'Either include it or remove the dependency array. ' +
2307 + `If 'state' is only necessary for calculating the next state, ` +
2308 + `consider refactoring to the setState(state => ...) form which ` +
2309 + `doesn't need to depend on the state from outside.`,
2310 ],
2311 },
2312 {
@@ -2232,7 +2335,10 @@ const tests = {
2335 `,
2336 errors: [
2337 "React Hook useEffect has a missing dependency: 'state'. " +
2235 - 'Either include it or remove the dependency array.',
2338 + 'Either include it or remove the dependency array. ' +
2339 + `If 'state' is only necessary for calculating the next state, ` +
2340 + `consider refactoring to the setState(state => ...) form which ` +
2341 + `doesn't need to depend on the state from outside.`,
2342 ],
2343 },
2344 {
@@ -2447,7 +2553,9 @@ const tests = {
2553 errors: [
2554 "React Hook useEffect has a missing dependency: 'props'. " +
2555 'Either include it or remove the dependency array. ' +
2450 - 'Alternatively, destructure the necessary props outside the callback.',
2556 + `However, the preferred fix is to destructure the 'props' ` +
2557 + `object outside of the useEffect call and refer to specific ` +
2558 + `props directly by their names.`,
2559 ],
2560 },
2561 {
@@ -2478,7 +2586,9 @@ const tests = {
2586 errors: [
2587 "React Hook useEffect has a missing dependency: 'props'. " +
2588 'Either include it or remove the dependency array. ' +
2481 - 'Alternatively, destructure the necessary props outside the callback.',
2589 + `However, the preferred fix is to destructure the 'props' ` +
2590 + `object outside of the useEffect call and refer to specific ` +
2591 + `props directly by their names.`,
2592 ],
2593 },
2594 {
@@ -2529,7 +2639,9 @@ const tests = {
2639 errors: [
2640 "React Hook useEffect has a missing dependency: 'props'. " +
2641 'Either include it or remove the dependency array. ' +
2532 - 'Alternatively, destructure the necessary props outside the callback.',
2642 + `However, the preferred fix is to destructure the 'props' ` +
2643 + `object outside of the useEffect call and refer to specific ` +
2644 + `props directly by their names.`,
2645 ],
2646 },
2647 {
@@ -2556,7 +2668,9 @@ const tests = {
2668 errors: [
2669 "React Hook useEffect has a missing dependency: 'props'. " +
2670 'Either include it or remove the dependency array. ' +
2559 - 'Alternatively, destructure the necessary props outside the callback.',
2671 + `However, the preferred fix is to destructure the 'props' ` +
2672 + `object outside of the useEffect call and refer to specific ` +
2673 + `props directly by their names.`,
2674 ],
2675 },
2676 {
@@ -2583,7 +2697,9 @@ const tests = {
2697 errors: [
2698 "React Hook useEffect has missing dependencies: 'props' and 'skillsCount'. " +
2699 'Either include them or remove the dependency array. ' +
2586 - 'Alternatively, destructure the necessary props outside the callback.',
2700 + `However, the preferred fix is to destructure the 'props' ` +
2701 + `object outside of the useEffect call and refer to specific ` +
2702 + `props directly by their names.`,
2703 ],
2704 },
2705 {
@@ -2638,11 +2754,15 @@ const tests = {
2754 let value;
2755 let value2;
2756 let value3;
2757 + let value4;
2758 let asyncValue;
2759 useEffect(() => {
2643 - value = {};
2760 + if (value4) {
2761 + value = {};
2762 + }
2763 value2 = 100;
2764 value = 43;
2765 + value4 = true;
2766 console.log(value2);
2767 console.log(value3);
2768 setTimeout(() => {
@@ -2659,11 +2779,15 @@ const tests = {
2779 let value;
2780 let value2;
2781 let value3;
2782 + let value4;
2783 let asyncValue;
2784 useEffect(() => {
2664 - value = {};
2785 + if (value4) {
2786 + value = {};
2787 + }
2788 value2 = 100;
2789 value = 43;
2790 + value4 = true;
2791 console.log(value2);
2792 console.log(value3);
2793 setTimeout(() => {
@@ -2673,14 +2797,20 @@ const tests = {
2797 }
2798 `,
2799 errors: [
2800 + // value2
2801 + `Assignments to the 'value2' variable from inside a React useEffect Hook ` +
2802 + `will not persist between re-renders. ` +
2803 + `If it's only needed by this Hook, move the variable inside it. ` +
2804 + `Alternatively, declare a ref with the useRef Hook, ` +
2805 + `and keep the mutable value in its 'current' property.`,
2806 // value
2807 `Assignments to the 'value' variable from inside a React useEffect Hook ` +
2808 `will not persist between re-renders. ` +
2809 `If it's only needed by this Hook, move the variable inside it. ` +
2810 `Alternatively, declare a ref with the useRef Hook, ` +
2811 `and keep the mutable value in its 'current' property.`,
2682 - // value2
2683 - `Assignments to the 'value2' variable from inside a React useEffect Hook ` +
2812 + // value4
2813 + `Assignments to the 'value4' variable from inside a React useEffect Hook ` +
2814 `will not persist between re-renders. ` +
2815 `If it's only needed by this Hook, move the variable inside it. ` +
2816 `Alternatively, declare a ref with the useRef Hook, ` +
@@ -3339,7 +3469,7 @@ const tests = {
3469 `The 'handleNext' function makes the dependencies of ` +
3470 `useEffect Hook (at line 11) change on every render. ` +
3471 `To fix this, move the 'handleNext' function ` +
3342 - `inside the useEffect callback. Alternatively, ` +
3472 + `inside the useEffect callback (at line 9). Alternatively, ` +
3473 `wrap the 'handleNext' definition into its own useCallback() Hook.`,
3474 ],
3475 },
@@ -3378,7 +3508,7 @@ const tests = {
3508 `The 'handleNext' function makes the dependencies of ` +
3509 `useEffect Hook (at line 11) change on every render. ` +
3510 `To fix this, move the 'handleNext' function ` +
3381 - `inside the useEffect callback. Alternatively, ` +
3511 + `inside the useEffect callback (at line 9). Alternatively, ` +
3512 `wrap the 'handleNext' definition into its own useCallback() Hook.`,
3513 ],
3514 },
@@ -3476,15 +3606,15 @@ const tests = {
3606 errors: [
3607 "The 'handleNext1' function makes the dependencies of useEffect Hook " +
3608 "(at line 14) change on every render. To fix this, move the 'handleNext1' " +
3479 - 'function inside the useEffect callback. Alternatively, wrap the ' +
3609 + 'function inside the useEffect callback (at line 12). Alternatively, wrap the ' +
3610 "'handleNext1' definition into its own useCallback() Hook.",
3611 "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " +
3612 "(at line 17) change on every render. To fix this, move the 'handleNext2' " +
3483 - 'function inside the useLayoutEffect callback. Alternatively, wrap the ' +
3613 + 'function inside the useLayoutEffect callback (at line 15). Alternatively, wrap the ' +
3614 "'handleNext2' definition into its own useCallback() Hook.",
3615 "The 'handleNext3' function makes the dependencies of useMemo Hook " +
3616 "(at line 20) change on every render. To fix this, move the 'handleNext3' " +
3487 - 'function inside the useMemo callback. Alternatively, wrap the ' +
3617 + 'function inside the useMemo callback (at line 18). Alternatively, wrap the ' +
3618 "'handleNext3' definition into its own useCallback() Hook.",
3619 ],
3620 },
@@ -3544,15 +3674,15 @@ const tests = {
3674 errors: [
3675 "The 'handleNext1' function makes the dependencies of useEffect Hook " +
3676 "(at line 15) change on every render. To fix this, move the 'handleNext1' " +
3547 - 'function inside the useEffect callback. Alternatively, wrap the ' +
3677 + 'function inside the useEffect callback (at line 12). Alternatively, wrap the ' +
3678 "'handleNext1' definition into its own useCallback() Hook.",
3679 "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " +
3680 "(at line 19) change on every render. To fix this, move the 'handleNext2' " +
3551 - 'function inside the useLayoutEffect callback. Alternatively, wrap the ' +
3681 + 'function inside the useLayoutEffect callback (at line 16). Alternatively, wrap the ' +
3682 "'handleNext2' definition into its own useCallback() Hook.",
3683 "The 'handleNext3' function makes the dependencies of useMemo Hook " +
3684 "(at line 23) change on every render. To fix this, move the 'handleNext3' " +
3555 - 'function inside the useMemo callback. Alternatively, wrap the ' +
3685 + 'function inside the useMemo callback (at line 20). Alternatively, wrap the ' +
3686 "'handleNext3' definition into its own useCallback() Hook.",
3687 ],
3688 },
@@ -3700,6 +3830,47 @@ const tests = {
3830 "'handleNext2' definition into its own useCallback() Hook.",
3831 ],
3832 },
3833 + {
3834 + code: `
3835 + function MyComponent(props) {
3836 + let handleNext = () => {
3837 + console.log('hello');
3838 + };
3839 + if (props.foo) {
3840 + handleNext = () => {
3841 + console.log('hello');
3842 + };
3843 + }
3844 + useEffect(() => {
3845 + return Store.subscribe(handleNext);
3846 + }, [handleNext]);
3847 + }
3848 + `,
3849 + // Normally we'd suggest moving handleNext inside an
3850 + // effect. But it's used more than once.
3851 + // TODO: our autofix here isn't quite sufficient because
3852 + // it only wraps the first definition. But seems ok.
3853 + output: `
3854 + function MyComponent(props) {
3855 + let handleNext = useCallback(() => {
3856 + console.log('hello');
3857 + });
3858 + if (props.foo) {
3859 + handleNext = () => {
3860 + console.log('hello');
3861 + };
3862 + }
3863 + useEffect(() => {
3864 + return Store.subscribe(handleNext);
3865 + }, [handleNext]);
3866 + }
3867 + `,
3868 + errors: [
3869 + "The 'handleNext' function makes the dependencies of useEffect Hook " +
3870 + '(at line 13) change on every render. To fix this, wrap the ' +
3871 + "'handleNext' definition into its own useCallback() Hook.",
3872 + ],
3873 + },
3874 {
3875 code: `
3876 function MyComponent(props) {
@@ -3737,7 +3908,7 @@ const tests = {
3908 `The 'handleNext' function makes the dependencies of ` +
3909 `useEffect Hook (at line 14) change on every render. ` +
3910 `To fix this, move the 'handleNext' function inside ` +
3740 - `the useEffect callback. Alternatively, wrap the ` +
3911 + `the useEffect callback (at line 12). Alternatively, wrap the ` +
3912 `'handleNext' definition into its own useCallback() Hook.`,
3913 ],
3914 },
@@ -3770,13 +3941,260 @@ const tests = {
3941 return <h1>{count}</h1>;
3942 }
3943 `,
3773 - // TODO: ideally this should suggest useState updater form
3774 - // since this code doesn't actually work.
3944 errors: [
3945 "React Hook useEffect has a missing dependency: 'count'. " +
3946 + 'Either include it or remove the dependency array. ' +
3947 + `If 'count' is only necessary for calculating the next state, ` +
3948 + `consider refactoring to the setCount(count => ...) form which ` +
3949 + `doesn't need to depend on the state from outside.`,
3950 + ],
3951 + },
3952 + {
3953 + code: `
3954 + function Counter() {
3955 + let [count, setCount] = useState(0);
3956 + let [increment, setIncrement] = useState(0);
3957 +
3958 + useEffect(() => {
3959 + let id = setInterval(() => {
3960 + setCount(count + increment);
3961 + }, 1000);
3962 + return () => clearInterval(id);
3963 + }, []);
3964 +
3965 + return <h1>{count}</h1>;
3966 + }
3967 + `,
3968 + output: `
3969 + function Counter() {
3970 + let [count, setCount] = useState(0);
3971 + let [increment, setIncrement] = useState(0);
3972 +
3973 + useEffect(() => {
3974 + let id = setInterval(() => {
3975 + setCount(count + increment);
3976 + }, 1000);
3977 + return () => clearInterval(id);
3978 + }, [count, increment]);
3979 +
3980 + return <h1>{count}</h1>;
3981 + }
3982 + `,
3983 + errors: [
3984 + "React Hook useEffect has missing dependencies: 'count' and 'increment'. " +
3985 + 'Either include them or remove the dependency array. ' +
3986 + `If 'count' is only necessary for calculating the next state, ` +
3987 + `consider refactoring to the setCount(count => ...) form which ` +
3988 + `doesn't need to depend on the state from outside.`,
3989 + ],
3990 + },
3991 + {
3992 + code: `
3993 + function Counter() {
3994 + let [count, setCount] = useState(0);
3995 + let [increment, setIncrement] = useState(0);
3996 +
3997 + useEffect(() => {
3998 + let id = setInterval(() => {
3999 + setCount(count => count + increment);
4000 + }, 1000);
4001 + return () => clearInterval(id);
4002 + }, []);
4003 +
4004 + return <h1>{count}</h1>;
4005 + }
4006 + `,
4007 + output: `
4008 + function Counter() {
4009 + let [count, setCount] = useState(0);
4010 + let [increment, setIncrement] = useState(0);
4011 +
4012 + useEffect(() => {
4013 + let id = setInterval(() => {
4014 + setCount(count => count + increment);
4015 + }, 1000);
4016 + return () => clearInterval(id);
4017 + }, [increment]);
4018 +
4019 + return <h1>{count}</h1>;
4020 + }
4021 + `,
4022 + errors: [
4023 + "React Hook useEffect has a missing dependency: 'increment'. " +
4024 + 'Either include it or remove the dependency array. ' +
4025 + `If 'increment' is only necessary for calculating the next state, ` +
4026 + `consider refactoring to the useReducer Hook. This ` +
4027 + `lets you move the calculation of next state outside the effect.`,
4028 + ],
4029 + },
4030 + {
4031 + code: `
4032 + function Counter() {
4033 + let [count, setCount] = useState(0);
4034 + let increment = useCustomHook();
4035 +
4036 + useEffect(() => {
4037 + let id = setInterval(() => {
4038 + setCount(count => count + increment);
4039 + }, 1000);
4040 + return () => clearInterval(id);
4041 + }, []);
4042 +
4043 + return <h1>{count}</h1>;
4044 + }
4045 + `,
4046 + output: `
4047 + function Counter() {
4048 + let [count, setCount] = useState(0);
4049 + let increment = useCustomHook();
4050 +
4051 + useEffect(() => {
4052 + let id = setInterval(() => {
4053 + setCount(count => count + increment);
4054 + }, 1000);
4055 + return () => clearInterval(id);
4056 + }, [increment]);
4057 +
4058 + return <h1>{count}</h1>;
4059 + }
4060 + `,
4061 + // This intentionally doesn't show the reducer message
4062 + // because we don't know if it's safe for it to close over a value.
4063 + // We only show it for state variables (and possibly props).
4064 + errors: [
4065 + "React Hook useEffect has a missing dependency: 'increment'. " +
4066 + 'Either include it or remove the dependency array.',
4067 + ],
4068 + },
4069 + {
4070 + code: `
4071 + function Counter({ step }) {
4072 + let [count, setCount] = useState(0);
4073 +
4074 + function increment(x) {
4075 + return x + step;
4076 + }
4077 +
4078 + useEffect(() => {
4079 + let id = setInterval(() => {
4080 + setCount(count => increment(count));
4081 + }, 1000);
4082 + return () => clearInterval(id);
4083 + }, []);
4084 +
4085 + return <h1>{count}</h1>;
4086 + }
4087 + `,
4088 + output: `
4089 + function Counter({ step }) {
4090 + let [count, setCount] = useState(0);
4091 +
4092 + function increment(x) {
4093 + return x + step;
4094 + }
4095 +
4096 + useEffect(() => {
4097 + let id = setInterval(() => {
4098 + setCount(count => increment(count));
4099 + }, 1000);
4100 + return () => clearInterval(id);
4101 + }, [increment]);
4102 +
4103 + return <h1>{count}</h1>;
4104 + }
4105 + `,
4106 + // This intentionally doesn't show the reducer message
4107 + // because we don't know if it's safe for it to close over a value.
4108 + // We only show it for state variables (and possibly props).
4109 + errors: [
4110 + "React Hook useEffect has a missing dependency: 'increment'. " +
4111 'Either include it or remove the dependency array.',
4112 ],
4113 },
4114 + {
4115 + code: `
4116 + function Counter({ step }) {
4117 + let [count, setCount] = useState(0);
4118 +
4119 + function increment(x) {
4120 + return x + step;
4121 + }
4122 +
4123 + useEffect(() => {
4124 + let id = setInterval(() => {
4125 + setCount(count => increment(count));
4126 + }, 1000);
4127 + return () => clearInterval(id);
4128 + }, [increment]);
4129 +
4130 + return <h1>{count}</h1>;
4131 + }
4132 + `,
4133 + output: `
4134 + function Counter({ step }) {
4135 + let [count, setCount] = useState(0);
4136 +
4137 + function increment(x) {
4138 + return x + step;
4139 + }
4140 +
4141 + useEffect(() => {
4142 + let id = setInterval(() => {
4143 + setCount(count => increment(count));
4144 + }, 1000);
4145 + return () => clearInterval(id);
4146 + }, [increment]);
4147 +
4148 + return <h1>{count}</h1>;
4149 + }
4150 + `,
4151 + errors: [
4152 + `The 'increment' function makes the dependencies of useEffect Hook ` +
4153 + `(at line 14) change on every render. To fix this, move the ` +
4154 + `'increment' function inside the useEffect callback (at line 9). ` +
4155 + `Alternatively, wrap the \'increment\' definition into its own ` +
4156 + `useCallback() Hook.`,
4157 + ],
4158 + },
4159 + {
4160 + code: `
4161 + function Counter({ increment }) {
4162 + let [count, setCount] = useState(0);
4163 +
4164 + useEffect(() => {
4165 + let id = setInterval(() => {
4166 + setCount(count => count + increment);
4167 + }, 1000);
4168 + return () => clearInterval(id);
4169 + }, []);
4170 +
4171 + return <h1>{count}</h1>;
4172 + }
4173 + `,
4174 + output: `
4175 + function Counter({ increment }) {
4176 + let [count, setCount] = useState(0);
4177 +
4178 + useEffect(() => {
4179 + let id = setInterval(() => {
4180 + setCount(count => count + increment);
4181 + }, 1000);
4182 + return () => clearInterval(id);
4183 + }, [increment]);
4184 +
4185 + return <h1>{count}</h1>;
4186 + }
4187 + `,
4188 + errors: [
4189 + "React Hook useEffect has a missing dependency: 'increment'. " +
4190 + 'Either include it or remove the dependency array. ' +
4191 + `If 'increment' is only necessary for calculating the next state, ` +
4192 + `consider refactoring to the useReducer Hook. This lets you move ` +
4193 + `the calculation of next state outside the effect. ` +
4194 + `You can then read 'increment' from the reducer ` +
4195 + `by putting it directly in your component.`,
4196 + ],
4197 + },
4198 {
4199 code: `
4200 function Counter() {
@@ -3849,6 +4267,112 @@ const tests = {
4267 `Either include it or remove the dependency array.`,
4268 ],
4269 },
4270 + {
4271 + code: `
4272 + function Podcasts({ fetchPodcasts, id }) {
4273 + let [podcasts, setPodcasts] = useState(null);
4274 + useEffect(() => {
4275 + fetchPodcasts(id).then(setPodcasts);
4276 + }, [id]);
4277 + }
4278 + `,
4279 + output: `
4280 + function Podcasts({ fetchPodcasts, id }) {
4281 + let [podcasts, setPodcasts] = useState(null);
4282 + useEffect(() => {
4283 + fetchPodcasts(id).then(setPodcasts);
4284 + }, [fetchPodcasts, id]);
4285 + }
4286 + `,
4287 + errors: [
4288 + `React Hook useEffect has a missing dependency: 'fetchPodcasts'. ` +
4289 + `Either include it or remove the dependency array. ` +
4290 + `If specifying 'fetchPodcasts' makes the dependencies change too often, ` +
4291 + `find the parent component that defines it and wrap that definition in useCallback.`,
4292 + ],
4293 + },
4294 + {
4295 + code: `
4296 + function Podcasts({ api: { fetchPodcasts }, id }) {
4297 + let [podcasts, setPodcasts] = useState(null);
4298 + useEffect(() => {
4299 + fetchPodcasts(id).then(setPodcasts);
4300 + }, [id]);
4301 + }
4302 + `,
4303 + output: `
4304 + function Podcasts({ api: { fetchPodcasts }, id }) {
4305 + let [podcasts, setPodcasts] = useState(null);
4306 + useEffect(() => {
4307 + fetchPodcasts(id).then(setPodcasts);
4308 + }, [fetchPodcasts, id]);
4309 + }
4310 + `,
4311 + errors: [
4312 + `React Hook useEffect has a missing dependency: 'fetchPodcasts'. ` +
4313 + `Either include it or remove the dependency array. ` +
4314 + `If specifying 'fetchPodcasts' makes the dependencies change too often, ` +
4315 + `find the parent component that defines it and wrap that definition in useCallback.`,
4316 + ],
4317 + },
4318 + {
4319 + code: `
4320 + function Podcasts({ fetchPodcasts, fetchPodcasts2, id }) {
4321 + let [podcasts, setPodcasts] = useState(null);
4322 + useEffect(() => {
4323 + setTimeout(() => {
4324 + console.log(id);
4325 + fetchPodcasts(id).then(setPodcasts);
4326 + fetchPodcasts2(id).then(setPodcasts);
4327 + });
4328 + }, [id]);
4329 + }
4330 + `,
4331 + output: `
4332 + function Podcasts({ fetchPodcasts, fetchPodcasts2, id }) {
4333 + let [podcasts, setPodcasts] = useState(null);
4334 + useEffect(() => {
4335 + setTimeout(() => {
4336 + console.log(id);
4337 + fetchPodcasts(id).then(setPodcasts);
4338 + fetchPodcasts2(id).then(setPodcasts);
4339 + });
4340 + }, [fetchPodcasts, fetchPodcasts2, id]);
4341 + }
4342 + `,
4343 + errors: [
4344 + `React Hook useEffect has missing dependencies: 'fetchPodcasts' and 'fetchPodcasts2'. ` +
4345 + `Either include them or remove the dependency array. ` +
4346 + `If specifying 'fetchPodcasts' makes the dependencies change too often, ` +
4347 + `find the parent component that defines it and wrap that definition in useCallback.`,
4348 + ],
4349 + },
4350 + {
4351 + code: `
4352 + function Podcasts({ fetchPodcasts, id }) {
4353 + let [podcasts, setPodcasts] = useState(null);
4354 + useEffect(() => {
4355 + console.log(fetchPodcasts);
4356 + fetchPodcasts(id).then(setPodcasts);
4357 + }, [id]);
4358 + }
4359 + `,
4360 + output: `
4361 + function Podcasts({ fetchPodcasts, id }) {
4362 + let [podcasts, setPodcasts] = useState(null);
4363 + useEffect(() => {
4364 + console.log(fetchPodcasts);
4365 + fetchPodcasts(id).then(setPodcasts);
4366 + }, [fetchPodcasts, id]);
4367 + }
4368 + `,
4369 + errors: [
4370 + `React Hook useEffect has a missing dependency: 'fetchPodcasts'. ` +
4371 + `Either include it or remove the dependency array. ` +
4372 + `If specifying 'fetchPodcasts' makes the dependencies change too often, ` +
4373 + `find the parent component that defines it and wrap that definition in useCallback.`,
4374 + ],
4375 + },
4376 ],
4377 };
4378
packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
+198 -22
@@ -35,6 +35,8 @@ export default {
35 const options = {additionalHooks};
36
37 // Should be shared between visitors.
38 + let setStateCallSites = new WeakMap();
39 + let stateVariables = new WeakSet();
40 let staticKnownValueCache = new WeakMap();
41 let functionWithoutCapturedValueCache = new WeakMap();
42 function memoizeWithWeakMap(fn, map) {
@@ -204,19 +206,40 @@ export default {
206 return false;
207 }
208 const id = def.node.id;
207 - if (callee.name === 'useRef' && id.type === 'Identifier') {
209 + const {name} = callee;
210 + if (name === 'useRef' && id.type === 'Identifier') {
211 // useRef() return value is static.
212 return true;
210 - } else if (callee.name === 'useState' || callee.name === 'useReducer') {
213 + } else if (name === 'useState' || name === 'useReducer') {
214 // Only consider second value in initializing tuple static.
215 if (
216 id.type === 'ArrayPattern' &&
217 id.elements.length === 2 &&
215 - Array.isArray(resolved.identifiers) &&
216 - // Is second tuple value the same reference we're checking?
217 - id.elements[1] === resolved.identifiers[0]
218 + Array.isArray(resolved.identifiers)
219 ) {
219 - return true;
220 + // Is second tuple value the same reference we're checking?
221 + if (id.elements[1] === resolved.identifiers[0]) {
222 + if (name === 'useState') {
223 + const references = resolved.references;
224 + for (let i = 0; i < references.length; i++) {
225 + setStateCallSites.set(
226 + references[i].identifier,
227 + id.elements[0],
228 + );
229 + }
230 + }
231 + // Setter is static.
232 + return true;
233 + } else if (id.elements[0] === resolved.identifiers[0]) {
234 + if (name === 'useState') {
235 + const references = resolved.references;
236 + for (let i = 0; i < references.length; i++) {
237 + stateVariables.add(references[i].identifier);
238 + }
239 + }
240 + // State variable itself is dynamic.
241 + return false;
242 + }
243 }
244 }
245 // By default assume it's dynamic.
@@ -365,8 +388,10 @@ export default {
388 memoizedIsFunctionWithoutCapturedValues(resolved);
389 dependencies.set(dependency, {
390 isStatic,
368 - reference,
391 + references: [reference],
392 });
393 + } else {
394 + dependencies.get(dependency).references.push(reference);
395 }
396 }
397 for (const childScope of currentScope.childScopes) {
@@ -514,9 +539,12 @@ export default {
539
540 // Warn about assigning to variables in the outer scope.
541 // Those are usually bugs.
517 - let foundStaleAssignments = false;
542 + let staleAssignments = new Set();
543 function reportStaleAssignment(writeExpr, key) {
519 - foundStaleAssignments = true;
544 + if (staleAssignments.has(key)) {
545 + return;
546 + }
547 + staleAssignments.add(key);
548 context.report({
549 node: writeExpr,
550 message:
@@ -532,15 +560,17 @@ export default {
560
561 // Remember which deps are optional and report bad usage first.
562 const optionalDependencies = new Set();
535 - dependencies.forEach(({isStatic, reference}, key) => {
563 + dependencies.forEach(({isStatic, references}, key) => {
564 if (isStatic) {
565 optionalDependencies.add(key);
566 }
539 - if (reference.writeExpr) {
540 - reportStaleAssignment(reference.writeExpr, key);
541 - }
567 + references.forEach(reference => {
568 + if (reference.writeExpr) {
569 + reportStaleAssignment(reference.writeExpr, key);
570 + }
571 + });
572 });
543 - if (foundStaleAssignments) {
573 + if (staleAssignments.size > 0) {
574 // The intent isn't clear so we'll wait until you fix those first.
575 return;
576 }
@@ -588,8 +618,10 @@ export default {
618 } else {
619 message +=
620 ` To fix this, move the '${fn.name.name}' function ` +
591 - `inside the ${reactiveHookName} callback. Alternatively, ` +
592 - `wrap the '${
621 + `inside the ${reactiveHookName} callback (at line ${
622 + node.loc.start.line
623 + }). ` +
624 + `Alternatively, wrap the '${
625 fn.name.name
626 }' definition into its own useCallback() Hook.`;
627 }
@@ -697,7 +729,7 @@ export default {
729 if (propDep == null) {
730 return;
731 }
700 - const refs = propDep.reference.resolved.references;
732 + const refs = propDep.references;
733 if (!Array.isArray(refs)) {
734 return;
735 }
@@ -724,8 +756,154 @@ export default {
756 }
757 if (isPropsOnlyUsedInMembers) {
758 extraWarning =
727 - ' Alternatively, destructure the necessary props ' +
728 - 'outside the callback.';
759 + ` However, the preferred fix is to destructure the 'props' ` +
760 + `object outside of the ${reactiveHookName} call and ` +
761 + `refer to specific props directly by their names.`;
762 + }
763 + }
764 +
765 + if (!extraWarning && missingDependencies.size > 0) {
766 + // See if the user is trying to avoid specifying a callable prop.
767 + // This usually means they're unaware of useCallback.
768 + let missingCallbackDep = null;
769 + missingDependencies.forEach(missingDep => {
770 + if (missingCallbackDep) {
771 + return;
772 + }
773 + // Is this a variable from top scope?
774 + const topScopeRef = componentScope.set.get(missingDep);
775 + const usedDep = dependencies.get(missingDep);
776 + if (usedDep.references[0].resolved !== topScopeRef) {
777 + return;
778 + }
779 + // Is this a destructured prop?
780 + const def = topScopeRef.defs[0];
781 + if (def == null || def.name == null || def.type !== 'Parameter') {
782 + return;
783 + }
784 + // Was it called in at least one case? Then it's a function.
785 + let isFunctionCall = false;
786 + let id;
787 + for (let i = 0; i < usedDep.references.length; i++) {
788 + id = usedDep.references[i].identifier;
789 + if (
790 + id != null &&
791 + id.parent != null &&
792 + id.parent.type === 'CallExpression' &&
793 + id.parent.callee === id
794 + ) {
795 + isFunctionCall = true;
796 + break;
797 + }
798 + }
799 + if (!isFunctionCall) {
800 + return;
801 + }
802 + // If it's missing (i.e. in component scope) *and* it's a parameter
803 + // then it is definitely coming from props destructuring.
804 + // (It could also be props itself but we wouldn't be calling it then.)
805 + missingCallbackDep = missingDep;
806 + });
807 + if (missingCallbackDep !== null) {
808 + extraWarning =
809 + ` If specifying '${missingCallbackDep}'` +
810 + ` makes the dependencies change too often, ` +
811 + `find the parent component that defines it ` +
812 + `and wrap that definition in useCallback.`;
813 + }
814 + }
815 +
816 + if (!extraWarning && missingDependencies.size > 0) {
817 + let setStateRecommendation = null;
818 + missingDependencies.forEach(missingDep => {
819 + if (setStateRecommendation !== null) {
820 + return;
821 + }
822 + const usedDep = dependencies.get(missingDep);
823 + const references = usedDep.references;
824 + let id;
825 + let maybeCall;
826 + for (let i = 0; i < references.length; i++) {
827 + id = references[i].identifier;
828 + maybeCall = id.parent;
829 + // Try to see if we have setState(someExpr(missingDep)).
830 + while (maybeCall != null && maybeCall !== componentScope.block) {
831 + if (maybeCall.type === 'CallExpression') {
832 + const correspondingStateVariable = setStateCallSites.get(
833 + maybeCall.callee,
834 + );
835 + if (correspondingStateVariable != null) {
836 + if (correspondingStateVariable.name === missingDep) {
837 + // setCount(count + 1)
838 + setStateRecommendation = {
839 + missingDep,
840 + setter: maybeCall.callee.name,
841 + form: 'updater',
842 + };
843 + } else if (stateVariables.has(id)) {
844 + // setCount(count + increment)
845 + setStateRecommendation = {
846 + missingDep,
847 + setter: maybeCall.callee.name,
848 + form: 'reducer',
849 + };
850 + } else {
851 + const resolved = references[i].resolved;
852 + if (resolved != null) {
853 + // If it's a parameter *and* a missing dep,
854 + // it must be a prop or something inside a prop.
855 + // Therefore, recommend an inline reducer.
856 + const def = resolved.defs[0];
857 + if (def != null && def.type === 'Parameter') {
858 + setStateRecommendation = {
859 + missingDep,
860 + setter: maybeCall.callee.name,
861 + form: 'inlineReducer',
862 + };
863 + }
864 + }
865 + }
866 + break;
867 + }
868 + }
869 + maybeCall = maybeCall.parent;
870 + }
871 + if (setStateRecommendation !== null) {
872 + break;
873 + }
874 + }
875 + });
876 + if (setStateRecommendation !== null) {
877 + let suggestion;
878 + switch (setStateRecommendation.form) {
879 + case 'reducer':
880 + suggestion =
881 + 'useReducer Hook. This lets you move the calculation ' +
882 + 'of next state outside the effect.';
883 + break;
884 + case 'inlineReducer':
885 + suggestion =
886 + 'useReducer Hook. This lets you move the calculation ' +
887 + 'of next state outside the effect. You can then ' +
888 + `read '${
889 + setStateRecommendation.missingDep
890 + }' from the reducer ` +
891 + `by putting it directly in your component.`;
892 + break;
893 + case 'updater':
894 + suggestion =
895 + `${setStateRecommendation.setter}(${
896 + setStateRecommendation.missingDep
897 + } => ...) form ` +
898 + `which doesn't need to depend on the state from outside.`;
899 + break;
900 + default:
901 + throw new Error('Unknown case.');
902 + }
903 + extraWarning =
904 + ` If '${setStateRecommendation.missingDep}'` +
905 + ` is only necessary for calculating the next state, ` +
906 + `consider refactoring to the ${suggestion}`;
907 }
908 }
909
@@ -981,9 +1159,7 @@ function scanForDeclaredBareFunctions({
1159 if (currentScope !== scope) {
1160 // This reference is outside the Hook callback.
1161 // It can only be legit if it's the deps array.
984 - if (isAncestorNodeOf(declaredDependenciesNode, reference.identifier)) {
985 - continue;
986 - } else {
1162 + if (!isAncestorNodeOf(declaredDependenciesNode, reference.identifier)) {
1163 return true;
1164 }
1165 }