Skip to content

Commit 59ef284

Browse files
gaearongaearon
authored
Warn about dependencies outside of render scope (#14990)
1 parent df7b476 commit 59ef284

File tree

2 files changed

+189
-15
lines changed

2 files changed

+189
-15
lines changed

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

Lines changed: 160 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -592,15 +592,6 @@ const tests = {
592592
}
593593
`,
594594
},
595-
{
596-
// It is valid for effects to over-specify their deps.
597-
// TODO: maybe only allow local ones, and disallow this example.
598-
code: `
599-
function MyComponent() {
600-
useEffect(() => {}, [window]);
601-
}
602-
`,
603-
},
604595
{
605596
// It is valid for effects to over-specify their deps.
606597
code: `
@@ -897,8 +888,6 @@ const tests = {
897888
],
898889
},
899890
{
900-
// TODO: this case is weird.
901-
// Maybe it should not consider local1 unused despite component nesting?
902891
code: `
903892
function MyComponent() {
904893
const local1 = {};
@@ -924,9 +913,10 @@ const tests = {
924913
}
925914
`,
926915
errors: [
927-
// Focus on the more important part first (missing dep)
928916
"React Hook useCallback has a missing dependency: 'local2'. " +
929-
'Either include it or remove the dependency array.',
917+
'Either include it or remove the dependency array. ' +
918+
"Values like 'local1' aren't valid dependencies " +
919+
"because their mutation doesn't re-render the component.",
930920
],
931921
},
932922
{
@@ -990,7 +980,9 @@ const tests = {
990980
`,
991981
errors: [
992982
"React Hook useCallback has an unnecessary dependency: 'window'. " +
993-
'Either exclude it or remove the dependency array.',
983+
'Either exclude it or remove the dependency array. ' +
984+
"Values like 'window' aren't valid dependencies " +
985+
"because their mutation doesn't re-render the component.",
994986
],
995987
},
996988
{
@@ -2530,6 +2522,160 @@ const tests = {
25302522
'Either include it or remove the dependency array.',
25312523
],
25322524
},
2525+
{
2526+
code: `
2527+
function MyComponent() {
2528+
useEffect(() => {
2529+
window.scrollTo(0, 0);
2530+
}, [window]);
2531+
}
2532+
`,
2533+
errors: [
2534+
"React Hook useEffect has an unnecessary dependency: 'window'. " +
2535+
'Either exclude it or remove the dependency array. ' +
2536+
"Values like 'window' aren't valid dependencies " +
2537+
"because their mutation doesn't re-render the component.",
2538+
],
2539+
},
2540+
{
2541+
code: `
2542+
import MutableStore from 'store';
2543+
2544+
function MyComponent() {
2545+
useEffect(() => {
2546+
console.log(MutableStore.hello);
2547+
}, [MutableStore.hello]);
2548+
}
2549+
`,
2550+
output: `
2551+
import MutableStore from 'store';
2552+
2553+
function MyComponent() {
2554+
useEffect(() => {
2555+
console.log(MutableStore.hello);
2556+
}, []);
2557+
}
2558+
`,
2559+
errors: [
2560+
"React Hook useEffect has an unnecessary dependency: 'MutableStore.hello'. " +
2561+
'Either exclude it or remove the dependency array. ' +
2562+
"Values like 'MutableStore.hello' aren't valid dependencies " +
2563+
"because their mutation doesn't re-render the component.",
2564+
],
2565+
},
2566+
{
2567+
code: `
2568+
import MutableStore from 'store';
2569+
let z = {};
2570+
2571+
function MyComponent(props) {
2572+
let x = props.foo;
2573+
{
2574+
let y = props.bar;
2575+
useEffect(() => {
2576+
console.log(MutableStore.hello.world, props.foo, x, y, z, global.stuff);
2577+
}, [MutableStore.hello.world, props.foo, x, y, z, global.stuff]);
2578+
}
2579+
}
2580+
`,
2581+
output: `
2582+
import MutableStore from 'store';
2583+
let z = {};
2584+
2585+
function MyComponent(props) {
2586+
let x = props.foo;
2587+
{
2588+
let y = props.bar;
2589+
useEffect(() => {
2590+
console.log(MutableStore.hello.world, props.foo, x, y, z, global.stuff);
2591+
}, [props.foo, x, y]);
2592+
}
2593+
}
2594+
`,
2595+
errors: [
2596+
'React Hook useEffect has unnecessary dependencies: ' +
2597+
"'MutableStore.hello.world', 'global.stuff', and 'z'. " +
2598+
'Either exclude them or remove the dependency array. ' +
2599+
"Values like 'MutableStore.hello.world' aren't valid dependencies " +
2600+
"because their mutation doesn't re-render the component.",
2601+
],
2602+
},
2603+
{
2604+
code: `
2605+
import MutableStore from 'store';
2606+
let z = {};
2607+
2608+
function MyComponent(props) {
2609+
let x = props.foo;
2610+
{
2611+
let y = props.bar;
2612+
useEffect(() => {
2613+
// nothing
2614+
}, [MutableStore.hello.world, props.foo, x, y, z, global.stuff]);
2615+
}
2616+
}
2617+
`,
2618+
// The output should contain the ones that are inside a component
2619+
// since there are legit reasons to over-specify them for effects.
2620+
output: `
2621+
import MutableStore from 'store';
2622+
let z = {};
2623+
2624+
function MyComponent(props) {
2625+
let x = props.foo;
2626+
{
2627+
let y = props.bar;
2628+
useEffect(() => {
2629+
// nothing
2630+
}, [props.foo, x, y]);
2631+
}
2632+
}
2633+
`,
2634+
errors: [
2635+
'React Hook useEffect has unnecessary dependencies: ' +
2636+
"'MutableStore.hello.world', 'global.stuff', and 'z'. " +
2637+
'Either exclude them or remove the dependency array. ' +
2638+
"Values like 'MutableStore.hello.world' aren't valid dependencies " +
2639+
"because their mutation doesn't re-render the component.",
2640+
],
2641+
},
2642+
{
2643+
code: `
2644+
import MutableStore from 'store';
2645+
let z = {};
2646+
2647+
function MyComponent(props) {
2648+
let x = props.foo;
2649+
{
2650+
let y = props.bar;
2651+
const fn = useCallback(() => {
2652+
// nothing
2653+
}, [MutableStore.hello.world, props.foo, x, y, z, global.stuff]);
2654+
}
2655+
}
2656+
`,
2657+
output: `
2658+
import MutableStore from 'store';
2659+
let z = {};
2660+
2661+
function MyComponent(props) {
2662+
let x = props.foo;
2663+
{
2664+
let y = props.bar;
2665+
const fn = useCallback(() => {
2666+
// nothing
2667+
}, []);
2668+
}
2669+
}
2670+
`,
2671+
errors: [
2672+
'React Hook useCallback has unnecessary dependencies: ' +
2673+
"'MutableStore.hello.world', 'global.stuff', 'props.foo', 'x', 'y', and 'z'. " +
2674+
'Either exclude them or remove the dependency array. ' +
2675+
"Values like 'MutableStore.hello.world' aren't valid dependencies " +
2676+
"because their mutation doesn't re-render the component.",
2677+
],
2678+
},
25332679
],
25342680
};
25352681

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

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ export default {
8686
// scope. We can't enforce this in a lint so we trust that all variables
8787
// declared outside of pure scope are indeed frozen.
8888
const pureScopes = new Set();
89+
let componentScope = null;
8990
{
9091
let currentScope = scope.upper;
9192
while (currentScope) {
@@ -101,6 +102,7 @@ export default {
101102
if (!currentScope) {
102103
return;
103104
}
105+
componentScope = currentScope;
104106
}
105107

106108
// These are usually mistaken. Collect them.
@@ -224,6 +226,7 @@ export default {
224226
);
225227

226228
const declaredDependencies = [];
229+
const externalDependencies = new Set();
227230
if (declaredDependenciesNode.type !== 'ArrayExpression') {
228231
// If the declared dependencies are not an array expression then we
229232
// can't verify that the user provided the correct dependencies. Tell
@@ -298,11 +301,24 @@ export default {
298301
throw error;
299302
}
300303
}
304+
305+
let maybeID = declaredDependencyNode;
306+
while (maybeID.type === 'MemberExpression') {
307+
maybeID = maybeID.object;
308+
}
309+
const isDeclaredInComponent = !componentScope.through.some(
310+
ref => ref.identifier === maybeID,
311+
);
312+
301313
// Add the dependency to our declared dependency map.
302314
declaredDependencies.push({
303315
key: declaredDependency,
304316
node: declaredDependencyNode,
305317
});
318+
319+
if (!isDeclaredInComponent) {
320+
externalDependencies.add(declaredDependency);
321+
}
306322
});
307323
}
308324

@@ -348,6 +364,7 @@ export default {
348364
dependencies,
349365
declaredDependencies,
350366
optionalDependencies,
367+
externalDependencies,
351368
isEffect,
352369
});
353370

@@ -370,6 +387,7 @@ export default {
370387
dependencies,
371388
declaredDependencies: [], // Pretend we don't know
372389
optionalDependencies,
390+
externalDependencies,
373391
isEffect,
374392
}).suggestedDependencies;
375393
}
@@ -423,6 +441,11 @@ export default {
423441
extraWarning =
424442
` Mutable values like '${badRef}' aren't valid dependencies ` +
425443
"because their mutation doesn't re-render the component.";
444+
} else if (externalDependencies.size > 0) {
445+
const dep = Array.from(externalDependencies)[0];
446+
extraWarning =
447+
` Values like '${dep}' aren't valid dependencies ` +
448+
`because their mutation doesn't re-render the component.`;
426449
}
427450
}
428451

@@ -462,6 +485,7 @@ function collectRecommendations({
462485
dependencies,
463486
declaredDependencies,
464487
optionalDependencies,
488+
externalDependencies,
465489
isEffect,
466490
}) {
467491
// Our primary data structure.
@@ -584,7 +608,11 @@ function collectRecommendations({
584608
duplicateDependencies.add(key);
585609
}
586610
} else {
587-
if (isEffect && !key.endsWith('.current')) {
611+
if (
612+
isEffect &&
613+
!key.endsWith('.current') &&
614+
!externalDependencies.has(key)
615+
) {
588616
// Effects are allowed extra "unnecessary" deps.
589617
// Such as resetting scroll when ID changes.
590618
// Consider them legit.

0 commit comments

Comments
 (0)