@samitouri / QOS-React / commits / 59ef28437a

Warn about dependencies outside of render scope (#14990)

Dan Abramov committed Mar 1, 2019 at 18:16 UTC 59ef28437ac30ae26fe53a3be1de50754be8b806

2 files changed +189 -15

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

+160 -14

@@ -592,15 +592,6 @@ const tests = {

592 }

593 `,

594 },

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 - },

595 {

596 // It is valid for effects to over-specify their deps.

597 code: `

@@ -897,8 +888,6 @@ const tests = {

888 ],

889 },

890 {

900 - // TODO: this case is weird.

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

891 code: `

892 function MyComponent() {

893 const local1 = {};

@@ -924,9 +913,10 @@ const tests = {

913 }

914 `,

915 errors: [

927 - // Focus on the more important part first (missing dep)

916 "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.",

920 ],

921 },

922 {

@@ -990,7 +980,9 @@ const tests = {

980 `,

981 errors: [

982 "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.",

986 ],

987 },

988 {

@@ -2530,6 +2522,160 @@ const tests = {

2522 'Either include it or remove the dependency array.',

2523 ],

2524 },

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 + },

2679 ],

2680 };

2681

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

+29 -1

@@ -86,6 +86,7 @@ export default {

86 // scope. We can't enforce this in a lint so we trust that all variables

87 // declared outside of pure scope are indeed frozen.

88 const pureScopes = new Set();

89 + let componentScope = null;

90 {

91 let currentScope = scope.upper;

92 while (currentScope) {

@@ -101,6 +102,7 @@ export default {

102 if (!currentScope) {

103 return;

104 }

105 + componentScope = currentScope;

106 }

107

108 // These are usually mistaken. Collect them.

@@ -224,6 +226,7 @@ export default {

226 );

227

228 const declaredDependencies = [];

229 + const externalDependencies = new Set();

230 if (declaredDependenciesNode.type !== 'ArrayExpression') {

231 // If the declared dependencies are not an array expression then we

232 // can't verify that the user provided the correct dependencies. Tell

@@ -298,11 +301,24 @@ export default {

301 throw error;

302 }

303 }

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 +

313 // Add the dependency to our declared dependency map.

314 declaredDependencies.push({

315 key: declaredDependency,

316 node: declaredDependencyNode,

317 });

318 +

319 + if (!isDeclaredInComponent) {

320 + externalDependencies.add(declaredDependency);

321 + }

322 });

323 }

324

@@ -348,6 +364,7 @@ export default {

364 dependencies,

365 declaredDependencies,

366 optionalDependencies,

367 + externalDependencies,

368 isEffect,

369 });

370

@@ -370,6 +387,7 @@ export default {

387 dependencies,

388 declaredDependencies: [], // Pretend we don't know

389 optionalDependencies,

390 + externalDependencies,

391 isEffect,

392 }).suggestedDependencies;

393 }

@@ -423,6 +441,11 @@ export default {

441 extraWarning =

442 ` Mutable values like '${badRef}' aren't valid dependencies ` +

443 "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.`;

449 }

450 }

451

@@ -462,6 +485,7 @@ function collectRecommendations({

485 dependencies,

486 declaredDependencies,

487 optionalDependencies,

488 + externalDependencies,

489 isEffect,

490 }) {

491 // Our primary data structure.

@@ -584,7 +608,11 @@ function collectRecommendations({

608 duplicateDependencies.add(key);

609 }

610 } else {

587 - if (isEffect && !key.endsWith('.current')) {

611 + if (

612 + isEffect &&

613 + !key.endsWith('.current') &&

614 + !externalDependencies.has(key)

615 + ) {

616 // Effects are allowed extra "unnecessary" deps.

617 // Such as resetting scroll when ID changes.

618 // Consider them legit.