[ESLint] Add more cases to exhaustive-deps rule (#14930)
* Add better message for literal dependencies * Warn about ref.current in cleanup phase * Fix wrong comment * Tweak wording
Dan Abramov committed
Feb 25, 2019 at 16:00 UTC
1bbfbc98d20d1eb0c7c8ed3772f5bdd0c29ac96c
2 files changed
+483
-75
packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
+297
@@ -446,6 +446,120 @@ const tests = {
446
}
447
`,
448
},
449
+ {
450
+ // Valid because we assign ref.current
451
+ // ourselves. Therefore it's likely not
452
+ // a ref managed by React.
453
+ code: `
454
+ function MyComponent() {
455
+ const myRef = useRef();
456
+ useEffect(() => {
457
+ const handleMove = () => {};
458
+ myRef.current = 42;
459
+ return () => {
460
+ console.log(myRef.current.toString())
461
+ };
462
+ }, []);
463
+ return <div />;
464
+ }
465
+ `,
466
+ },
467
+ {
468
+ // Valid because we assign ref.current
469
+ // ourselves. Therefore it's likely not
470
+ // a ref managed by React.
471
+ code: `
472
+ function useMyThing(myRef) {
473
+ useEffect(() => {
474
+ const handleMove = () => {};
475
+ myRef.current = 42;
476
+ return () => {
477
+ console.log(myRef.current.toString())
478
+ };
479
+ }, [myRef]);
480
+ }
481
+ `,
482
+ },
483
+ {
484
+ // Valid because the ref is captured.
485
+ code: `
486
+ function MyComponent() {
487
+ const myRef = useRef();
488
+ useEffect(() => {
489
+ const handleMove = () => {};
490
+ const node = myRef.current;
491
+ node.addEventListener('mousemove', handleMove);
492
+ return () => node.removeEventListener('mousemove', handleMove);
493
+ }, []);
494
+ return <div ref={myRef} />;
495
+ }
496
+ `,
497
+ },
498
+ {
499
+ // Valid because the ref is captured.
500
+ code: `
501
+ function useMyThing(myRef) {
502
+ const myRef = useRef();
503
+ useEffect(() => {
504
+ const handleMove = () => {};
505
+ const node = myRef.current;
506
+ node.addEventListener('mousemove', handleMove);
507
+ return () => node.removeEventListener('mousemove', handleMove);
508
+ }, [myRef]);
509
+ return <div ref={myRef} />;
510
+ }
511
+ `,
512
+ },
513
+ {
514
+ // Valid because it's not an effect.
515
+ code: `
516
+ function useMyThing(myRef) {
517
+ useCallback(() => {
518
+ const handleMouse = () => {};
519
+ myRef.current.addEventListener('mousemove', handleMouse);
520
+ myRef.current.addEventListener('mousein', handleMouse);
521
+ return function() {
522
+ setTimeout(() => {
523
+ myRef.current.removeEventListener('mousemove', handleMouse);
524
+ myRef.current.removeEventListener('mousein', handleMouse);
525
+ });
526
+ }
527
+ }, [myRef]);
528
+ }
529
+ `,
530
+ },
531
+ {
532
+ // Valid because we read ref.current in a function that isn't cleanup.
533
+ code: `
534
+ function useMyThing() {
535
+ const myRef = useRef();
536
+ useEffect(() => {
537
+ const handleMove = () => {
538
+ console.log(myRef.current)
539
+ };
540
+ window.addEventListener('mousemove', handleMove);
541
+ return () => window.removeEventListener('mousemove', handleMove);
542
+ }, []);
543
+ return <div ref={myRef} />;
544
+ }
545
+ `,
546
+ },
547
+ {
548
+ // Valid because we read ref.current in a function that isn't cleanup.
549
+ code: `
550
+ function useMyThing() {
551
+ const myRef = useRef();
552
+ useEffect(() => {
553
+ const handleMove = () => {
554
+ return () => window.removeEventListener('mousemove', handleMove);
555
+ };
556
+ window.addEventListener('mousemove', handleMove);
557
+ return () => {};
558
+ }, []);
559
+ return <div ref={myRef} />;
560
+ }
561
+ `,
562
+ },
563
],
564
invalid: [
565
{
@@ -779,6 +893,53 @@ const tests = {
893
'Either include it or remove the dependency array.',
894
],
895
},
896
+ {
897
+ code: `
898
+ function MyComponent({ foo, bar, baz }) {
899
+ useEffect(() => {
900
+ console.log(foo, bar, baz);
901
+ }, ['foo', 'bar']);
902
+ }
903
+ `,
904
+ output: `
905
+ function MyComponent({ foo, bar, baz }) {
906
+ useEffect(() => {
907
+ console.log(foo, bar, baz);
908
+ }, [bar, baz, foo]);
909
+ }
910
+ `,
911
+ errors: [
912
+ "React Hook useEffect has missing dependencies: 'bar', 'baz', and 'foo'. " +
913
+ 'Either include them or remove the dependency array.',
914
+ "The 'foo' string literal is not a valid dependency because it never changes. " +
915
+ 'Did you mean to include foo in the array instead?',
916
+ "The 'bar' string literal is not a valid dependency because it never changes. " +
917
+ 'Did you mean to include bar in the array instead?',
918
+ ],
919
+ },
920
+ {
921
+ code: `
922
+ function MyComponent({ foo, bar, baz }) {
923
+ useEffect(() => {
924
+ console.log(foo, bar, baz);
925
+ }, [42, false, null]);
926
+ }
927
+ `,
928
+ output: `
929
+ function MyComponent({ foo, bar, baz }) {
930
+ useEffect(() => {
931
+ console.log(foo, bar, baz);
932
+ }, [bar, baz, foo]);
933
+ }
934
+ `,
935
+ errors: [
936
+ "React Hook useEffect has missing dependencies: 'bar', 'baz', and 'foo'. " +
937
+ 'Either include them or remove the dependency array.',
938
+ "The '42' literal is not a valid dependency because it never changes. You can safely remove it.",
939
+ "The 'false' literal is not a valid dependency because it never changes. You can safely remove it.",
940
+ "The 'null' literal is not a valid dependency because it never changes. You can safely remove it.",
941
+ ],
942
+ },
943
{
944
code: `
945
function MyComponent() {
@@ -1742,6 +1903,142 @@ const tests = {
1903
`and keep the mutable value in its 'current' property.`,
1904
],
1905
},
1906
+ {
1907
+ code: `
1908
+ function MyComponent() {
1909
+ const myRef = useRef();
1910
+ useEffect(() => {
1911
+ const handleMove = () => {};
1912
+ myRef.current.addEventListener('mousemove', handleMove);
1913
+ return () => myRef.current.removeEventListener('mousemove', handleMove);
1914
+ }, []);
1915
+ return <div ref={myRef} />;
1916
+ }
1917
+ `,
1918
+ output: `
1919
+ function MyComponent() {
1920
+ const myRef = useRef();
1921
+ useEffect(() => {
1922
+ const handleMove = () => {};
1923
+ myRef.current.addEventListener('mousemove', handleMove);
1924
+ return () => myRef.current.removeEventListener('mousemove', handleMove);
1925
+ }, []);
1926
+ return <div ref={myRef} />;
1927
+ }
1928
+ `,
1929
+ errors: [
1930
+ `Accessing 'myRef.current' during the effect cleanup ` +
1931
+ `will likely read a different ref value because by this time React ` +
1932
+ `has already updated the ref. If this ref is managed by React, store ` +
1933
+ `'myRef.current' in a variable inside ` +
1934
+ `the effect itself and refer to that variable from the cleanup function.`,
1935
+ ],
1936
+ },
1937
+ {
1938
+ code: `
1939
+ function useMyThing(myRef) {
1940
+ useEffect(() => {
1941
+ const handleMove = () => {};
1942
+ myRef.current.addEventListener('mousemove', handleMove);
1943
+ return () => myRef.current.removeEventListener('mousemove', handleMove);
1944
+ }, [myRef]);
1945
+ }
1946
+ `,
1947
+ output: `
1948
+ function useMyThing(myRef) {
1949
+ useEffect(() => {
1950
+ const handleMove = () => {};
1951
+ myRef.current.addEventListener('mousemove', handleMove);
1952
+ return () => myRef.current.removeEventListener('mousemove', handleMove);
1953
+ }, [myRef]);
1954
+ }
1955
+ `,
1956
+ errors: [
1957
+ `Accessing 'myRef.current' during the effect cleanup ` +
1958
+ `will likely read a different ref value because by this time React ` +
1959
+ `has already updated the ref. If this ref is managed by React, store ` +
1960
+ `'myRef.current' in a variable inside ` +
1961
+ `the effect itself and refer to that variable from the cleanup function.`,
1962
+ ],
1963
+ },
1964
+ {
1965
+ code: `
1966
+ function useMyThing(myRef) {
1967
+ useEffect(() => {
1968
+ const handleMouse = () => {};
1969
+ myRef.current.addEventListener('mousemove', handleMouse);
1970
+ myRef.current.addEventListener('mousein', handleMouse);
1971
+ return function() {
1972
+ setTimeout(() => {
1973
+ myRef.current.removeEventListener('mousemove', handleMouse);
1974
+ myRef.current.removeEventListener('mousein', handleMouse);
1975
+ });
1976
+ }
1977
+ }, [myRef]);
1978
+ }
1979
+ `,
1980
+ output: `
1981
+ function useMyThing(myRef) {
1982
+ useEffect(() => {
1983
+ const handleMouse = () => {};
1984
+ myRef.current.addEventListener('mousemove', handleMouse);
1985
+ myRef.current.addEventListener('mousein', handleMouse);
1986
+ return function() {
1987
+ setTimeout(() => {
1988
+ myRef.current.removeEventListener('mousemove', handleMouse);
1989
+ myRef.current.removeEventListener('mousein', handleMouse);
1990
+ });
1991
+ }
1992
+ }, [myRef]);
1993
+ }
1994
+ `,
1995
+ errors: [
1996
+ `Accessing 'myRef.current' during the effect cleanup ` +
1997
+ `will likely read a different ref value because by this time React ` +
1998
+ `has already updated the ref. If this ref is managed by React, store ` +
1999
+ `'myRef.current' in a variable inside ` +
2000
+ `the effect itself and refer to that variable from the cleanup function.`,
2001
+ ],
2002
+ },
2003
+ {
2004
+ code: `
2005
+ function useMyThing(myRef, active) {
2006
+ useEffect(() => {
2007
+ const handleMove = () => {};
2008
+ if (active) {
2009
+ myRef.current.addEventListener('mousemove', handleMove);
2010
+ return function() {
2011
+ setTimeout(() => {
2012
+ myRef.current.removeEventListener('mousemove', handleMove);
2013
+ });
2014
+ }
2015
+ }
2016
+ }, [myRef, active]);
2017
+ }
2018
+ `,
2019
+ output: `
2020
+ function useMyThing(myRef, active) {
2021
+ useEffect(() => {
2022
+ const handleMove = () => {};
2023
+ if (active) {
2024
+ myRef.current.addEventListener('mousemove', handleMove);
2025
+ return function() {
2026
+ setTimeout(() => {
2027
+ myRef.current.removeEventListener('mousemove', handleMove);
2028
+ });
2029
+ }
2030
+ }
2031
+ }, [myRef, active]);
2032
+ }
2033
+ `,
2034
+ errors: [
2035
+ `Accessing 'myRef.current' during the effect cleanup ` +
2036
+ `will likely read a different ref value because by this time React ` +
2037
+ `has already updated the ref. If this ref is managed by React, store ` +
2038
+ `'myRef.current' in a variable inside ` +
2039
+ `the effect itself and refer to that variable from the cleanup function.`,
2040
+ ],
2041
+ },
2042
],
2043
};
2044
packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
+186
-75
@@ -9,62 +9,6 @@
9
10
'use strict';
11
12
-// const [state, setState] = useState() / React.useState()
13
-// ^^^ true for this reference
14
-// const [state, dispatch] = useReducer() / React.useReducer()
15
-// ^^^ true for this reference
16
-// const ref = useRef()
17
-// ^^ true for this reference
18
-// False for everything else.
19
-function isDefinitelyStaticDependency(reference) {
20
- // This function is written defensively because I'm not sure about corner cases.
21
- // TODO: we can strengthen this if we're sure about the types.
22
- const resolved = reference.resolved;
23
- if (resolved == null || !Array.isArray(resolved.defs)) {
24
- return false;
25
- }
26
- const def = resolved.defs[0];
27
- if (def == null || def.node.init == null) {
28
- return false;
29
- }
30
- // Look for `let stuff = SomeHook();`
31
- const init = def.node.init;
32
- if (init.callee == null) {
33
- return false;
34
- }
35
- let callee = init.callee;
36
- // Step into `= React.something` initializer.
37
- if (
38
- callee.type === 'MemberExpression' &&
39
- callee.object.name === 'React' &&
40
- callee.property != null &&
41
- !callee.computed
42
- ) {
43
- callee = callee.property;
44
- }
45
- if (callee.type !== 'Identifier') {
46
- return;
47
- }
48
- const id = def.node.id;
49
- if (callee.name === 'useRef' && id.type === 'Identifier') {
50
- // useRef() return value is static.
51
- return true;
52
- } else if (callee.name === 'useState' || callee.name === 'useReducer') {
53
- // Only consider second value in initializing tuple static.
54
- if (
55
- id.type === 'ArrayPattern' &&
56
- id.elements.length === 2 &&
57
- Array.isArray(reference.resolved.identifiers) &&
58
- // Is second tuple value the same reference we're checking?
59
- id.elements[1] === reference.resolved.identifiers[0]
60
- ) {
61
- return true;
62
- }
63
- }
64
- // By default assume it's dynamic.
65
- return false;
66
-}
67
-
12
export default {
13
meta: {
14
fixable: 'code',
@@ -118,6 +62,8 @@ export default {
62
63
// Get the reactive hook node.
64
const reactiveHook = node.parent.callee;
65
+ const reactiveHookName = getNodeWithoutReactNamespace(reactiveHook).name;
66
+ const isEffect = reactiveHookName.endsWith('Effect');
67
68
// Get the declared dependencies for this reactive hook. If there is no
69
// second argument then the reactive callback will re-run on every render.
@@ -157,6 +103,26 @@ export default {
103
}
104
}
105
106
+ // These are usually mistaken. Collect them.
107
+ const currentRefsInEffectCleanup = new Map();
108
+
109
+ // Is this reference inside a cleanup function for this effect node?
110
+ // We can check by traversing scopes upwards from the reference, and checking
111
+ // if the last "return () => " we encounter is located directly inside the effect.
112
+ function isInsideEffectCleanup(reference) {
113
+ let curScope = reference.from;
114
+ let isInReturnedFunction = false;
115
+ while (curScope.block !== node) {
116
+ if (curScope.type === 'function') {
117
+ isInReturnedFunction =
118
+ curScope.block.parent != null &&
119
+ curScope.block.parent.type === 'ReturnStatement';
120
+ }
121
+ curScope = curScope.upper;
122
+ }
123
+ return isInReturnedFunction;
124
+ }
125
+
126
// Get dependencies from all our resolved references in pure scopes.
127
// Key is dependency string, value is whether it's static.
128
const dependencies = new Map();
@@ -172,9 +138,9 @@ export default {
138
if (!pureScopes.has(reference.resolved.scope)) {
139
continue;
140
}
141
+
142
// Narrow the scope of a dependency if it is, say, a member expression.
143
// Then normalize the narrowed dependency.
177
-
144
const referenceNode = fastFindReferenceWithParent(
145
node,
146
reference.identifier,
@@ -182,6 +148,25 @@ export default {
148
const dependencyNode = getDependency(referenceNode);
149
const dependency = toPropertyAccessString(dependencyNode);
150
151
+ // Accessing ref.current inside effect cleanup is bad.
152
+ if (
153
+ // We're in an effect...
154
+ isEffect &&
155
+ // ... and this look like accessing .current...
156
+ dependencyNode.type === 'Identifier' &&
157
+ dependencyNode.parent.type === 'MemberExpression' &&
158
+ !dependencyNode.parent.computed &&
159
+ dependencyNode.parent.property.type === 'Identifier' &&
160
+ dependencyNode.parent.property.name === 'current' &&
161
+ // ...in a cleanup function or below...
162
+ isInsideEffectCleanup(reference)
163
+ ) {
164
+ currentRefsInEffectCleanup.set(dependency, {
165
+ reference,
166
+ dependencyNode,
167
+ });
168
+ }
169
+
170
// Add the dependency to a map so we can make sure it is referenced
171
// again in our dependencies array. Remember whether it's static.
172
if (!dependencies.has(dependency)) {
@@ -197,6 +182,47 @@ export default {
182
}
183
}
184
185
+ // Warn about accessing .current in cleanup effects.
186
+ currentRefsInEffectCleanup.forEach(
187
+ ({reference, dependencyNode}, dependency) => {
188
+ const references = reference.resolved.references;
189
+ // Is React managing this ref or us?
190
+ // Let's see if we can find a .current assignment.
191
+ let foundCurrentAssignment = false;
192
+ for (let i = 0; i < references.length; i++) {
193
+ const {identifier} = references[i];
194
+ const {parent} = identifier;
195
+ if (
196
+ parent != null &&
197
+ // ref.current
198
+ parent.type === 'MemberExpression' &&
199
+ !parent.computed &&
200
+ parent.property.type === 'Identifier' &&
201
+ parent.property.name === 'current' &&
202
+ // ref.current = <something>
203
+ parent.parent.type === 'AssignmentExpression' &&
204
+ parent.parent.left === parent
205
+ ) {
206
+ foundCurrentAssignment = true;
207
+ break;
208
+ }
209
+ }
210
+ // We only want to warn about React-managed refs.
211
+ if (foundCurrentAssignment) {
212
+ return;
213
+ }
214
+ context.report({
215
+ node: dependencyNode.parent.property,
216
+ message:
217
+ `Accessing '${dependency}.current' during the effect cleanup ` +
218
+ `will likely read a different ref value because by this time React ` +
219
+ `has already updated the ref. If this ref is managed by React, store ` +
220
+ `'${dependency}.current' in a variable inside ` +
221
+ `the effect itself and refer to that variable from the cleanup function.`,
222
+ });
223
+ },
224
+ );
225
+
226
const declaredDependencies = [];
227
if (declaredDependenciesNode.type !== 'ArrayExpression') {
228
// If the declared dependencies are not an array expression then we
@@ -234,13 +260,39 @@ export default {
260
declaredDependency = toPropertyAccessString(declaredDependencyNode);
261
} catch (error) {
262
if (/Unsupported node type/.test(error.message)) {
237
- context.report({
238
- node: declaredDependencyNode,
239
- message:
240
- `React Hook ${context.getSource(reactiveHook)} has a ` +
241
- `complex expression in the dependency array. ` +
242
- 'Extract it to a separate variable so it can be statically checked.',
243
- });
263
+ if (declaredDependencyNode.type === 'Literal') {
264
+ if (typeof declaredDependencyNode.value === 'string') {
265
+ context.report({
266
+ node: declaredDependencyNode,
267
+ message:
268
+ `The ${
269
+ declaredDependencyNode.raw
270
+ } string literal is not a valid dependency ` +
271
+ `because it never changes. Did you mean to ` +
272
+ `include ${
273
+ declaredDependencyNode.value
274
+ } in the array instead?`,
275
+ });
276
+ } else {
277
+ context.report({
278
+ node: declaredDependencyNode,
279
+ message:
280
+ `The '${
281
+ declaredDependencyNode.raw
282
+ }' literal is not a valid dependency ` +
283
+ 'because it never changes. You can safely remove it.',
284
+ });
285
+ }
286
+ } else {
287
+ context.report({
288
+ node: declaredDependencyNode,
289
+ message:
290
+ `React Hook ${context.getSource(reactiveHook)} has a ` +
291
+ `complex expression in the dependency array. ` +
292
+ 'Extract it to a separate variable so it can be statically checked.',
293
+ });
294
+ }
295
+
296
return;
297
} else {
298
throw error;
@@ -456,13 +508,7 @@ function toPropertyAccessString(node) {
508
}
509
}
510
459
-// What's the index of callback that needs to be analyzed for a given Hook?
460
-// -1 if it's not a Hook we care about (e.g. useState).
461
-// 0 for useEffect/useMemo/useCallback(fn).
462
-// 1 for useImperativeHandle(ref, fn).
463
-// For additionally configured Hooks, assume that they're like useEffect (0).
464
-function getReactiveHookCallbackIndex(node, options) {
465
- let isOnReactObject = false;
511
+function getNodeWithoutReactNamespace(node, options) {
512
if (
513
node.type === 'MemberExpression' &&
514
node.object.type === 'Identifier' &&
@@ -470,11 +516,20 @@ function getReactiveHookCallbackIndex(node, options) {
516
node.property.type === 'Identifier' &&
517
!node.computed
518
) {
473
- node = node.property;
474
- isOnReactObject = true;
519
+ return node.property;
520
}
521
+ return node;
522
+}
523
+
524
+// What's the index of callback that needs to be analyzed for a given Hook?
525
+// -1 if it's not a Hook we care about (e.g. useState).
526
+// 0 for useEffect/useMemo/useCallback(fn).
527
+// 1 for useImperativeHandle(ref, fn).
528
+// For additionally configured Hooks, assume that they're like useEffect (0).
529
+function getReactiveHookCallbackIndex(calleeNode, options) {
530
+ let node = getNodeWithoutReactNamespace(calleeNode);
531
if (node.type !== 'Identifier') {
477
- return;
532
+ return null;
533
}
534
switch (node.name) {
535
case 'useEffect':
@@ -487,7 +542,7 @@ function getReactiveHookCallbackIndex(node, options) {
542
// useImperativeHandle(ref, fn)
543
return 1;
544
default:
490
- if (!isOnReactObject && options && options.additionalHooks) {
545
+ if (node === calleeNode && options && options.additionalHooks) {
546
// Allow the user to provide a regular expression which enables the lint to
547
// target custom reactive hooks.
548
let name;
@@ -507,6 +562,62 @@ function getReactiveHookCallbackIndex(node, options) {
562
}
563
}
564
565
+// const [state, setState] = useState() / React.useState()
566
+// ^^^ true for this reference
567
+// const [state, dispatch] = useReducer() / React.useReducer()
568
+// ^^^ true for this reference
569
+// const ref = useRef()
570
+// ^^^ true for this reference
571
+// False for everything else.
572
+function isDefinitelyStaticDependency(reference) {
573
+ // This function is written defensively because I'm not sure about corner cases.
574
+ // TODO: we can strengthen this if we're sure about the types.
575
+ const resolved = reference.resolved;
576
+ if (resolved == null || !Array.isArray(resolved.defs)) {
577
+ return false;
578
+ }
579
+ const def = resolved.defs[0];
580
+ if (def == null || def.node.init == null) {
581
+ return false;
582
+ }
583
+ // Look for `let stuff = SomeHook();`
584
+ const init = def.node.init;
585
+ if (init.callee == null) {
586
+ return false;
587
+ }
588
+ let callee = init.callee;
589
+ // Step into `= React.something` initializer.
590
+ if (
591
+ callee.type === 'MemberExpression' &&
592
+ callee.object.name === 'React' &&
593
+ callee.property != null &&
594
+ !callee.computed
595
+ ) {
596
+ callee = callee.property;
597
+ }
598
+ if (callee.type !== 'Identifier') {
599
+ return false;
600
+ }
601
+ const id = def.node.id;
602
+ if (callee.name === 'useRef' && id.type === 'Identifier') {
603
+ // useRef() return value is static.
604
+ return true;
605
+ } else if (callee.name === 'useState' || callee.name === 'useReducer') {
606
+ // Only consider second value in initializing tuple static.
607
+ if (
608
+ id.type === 'ArrayPattern' &&
609
+ id.elements.length === 2 &&
610
+ Array.isArray(reference.resolved.identifiers) &&
611
+ // Is second tuple value the same reference we're checking?
612
+ id.elements[1] === reference.resolved.identifiers[0]
613
+ ) {
614
+ return true;
615
+ }
616
+ }
617
+ // By default assume it's dynamic.
618
+ return false;
619
+}
620
+
621
/**
622
* ESLint won't assign node.parent to references from context.getScope()
623
*