Warn about dependencies outside of render scope (#14990)

gaearon · web-flow · commit 59ef28437ac3 · 2019-03-01T18:16:17.000Z
diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
@@ -592,15 +592,6 @@ const tests = {
         }
       `,
     },
-    {
-      // It is valid for effects to over-specify their deps.
-      // TODO: maybe only allow local ones, and disallow this example.
-      code: `
-        function MyComponent() {
-          useEffect(() => {}, [window]);
-        }
-      `,
-    },
     {
       // It is valid for effects to over-specify their deps.
       code: `
@@ -897,8 +888,6 @@ const tests = {
       ],
     },
     {
-      // TODO: this case is weird.
-      // Maybe it should not consider local1 unused despite component nesting?
       code: `
         function MyComponent() {
           const local1 = {};
@@ -924,9 +913,10 @@ const tests = {
         }
       `,
       errors: [
-        // Focus on the more important part first (missing dep)
         "React Hook useCallback has a missing dependency: 'local2'. " +
-          'Either include it or remove the dependency array.',
+          'Either include it or remove the dependency array. ' +
+          "Values like 'local1' aren't valid dependencies " +
+          "because their mutation doesn't re-render the component.",
       ],
     },
     {
@@ -990,7 +980,9 @@ const tests = {
       `,
       errors: [
         "React Hook useCallback has an unnecessary dependency: 'window'. " +
-          'Either exclude it or remove the dependency array.',
+          'Either exclude it or remove the dependency array. ' +
+          "Values like 'window' aren't valid dependencies " +
+          "because their mutation doesn't re-render the component.",
       ],
     },
     {
@@ -2530,6 +2522,160 @@ const tests = {
           'Either include it or remove the dependency array.',
       ],
     },
+    {
+      code: `
+        function MyComponent() {
+          useEffect(() => {
+            window.scrollTo(0, 0);
+          }, [window]);
+        }
+      `,
+      errors: [
+        "React Hook useEffect has an unnecessary dependency: 'window'. " +
+          'Either exclude it or remove the dependency array. ' +
+          "Values like 'window' aren't valid dependencies " +
+          "because their mutation doesn't re-render the component.",
+      ],
+    },
+    {
+      code: `
+        import MutableStore from 'store';
+
+        function MyComponent() {
+          useEffect(() => {
+            console.log(MutableStore.hello);
+          }, [MutableStore.hello]);
+        }
+      `,
+      output: `
+        import MutableStore from 'store';
+
+        function MyComponent() {
+          useEffect(() => {
+            console.log(MutableStore.hello);
+          }, []);
+        }
+      `,
+      errors: [
+        "React Hook useEffect has an unnecessary dependency: 'MutableStore.hello'. " +
+          'Either exclude it or remove the dependency array. ' +
+          "Values like 'MutableStore.hello' aren't valid dependencies " +
+          "because their mutation doesn't re-render the component.",
+      ],
+    },
+    {
+      code: `
+        import MutableStore from 'store';
+        let z = {};
+
+        function MyComponent(props) {
+          let x = props.foo;
+          {
+            let y = props.bar;
+            useEffect(() => {
+              console.log(MutableStore.hello.world, props.foo, x, y, z, global.stuff);
+            }, [MutableStore.hello.world, props.foo, x, y, z, global.stuff]);
+          }
+        }
+      `,
+      output: `
+        import MutableStore from 'store';
+        let z = {};
+
+        function MyComponent(props) {
+          let x = props.foo;
+          {
+            let y = props.bar;
+            useEffect(() => {
+              console.log(MutableStore.hello.world, props.foo, x, y, z, global.stuff);
+            }, [props.foo, x, y]);
+          }
+        }
+      `,
+      errors: [
+        'React Hook useEffect has unnecessary dependencies: ' +
+          "'MutableStore.hello.world', 'global.stuff', and 'z'. " +
+          'Either exclude them or remove the dependency array. ' +
+          "Values like 'MutableStore.hello.world' aren't valid dependencies " +
+          "because their mutation doesn't re-render the component.",
+      ],
+    },
+    {
+      code: `
+        import MutableStore from 'store';
+        let z = {};
+
+        function MyComponent(props) {
+          let x = props.foo;
+          {
+            let y = props.bar;
+            useEffect(() => {
+              // nothing
+            }, [MutableStore.hello.world, props.foo, x, y, z, global.stuff]);
+          }
+        }
+      `,
+      // The output should contain the ones that are inside a component
+      // since there are legit reasons to over-specify them for effects.
+      output: `
+        import MutableStore from 'store';
+        let z = {};
+
+        function MyComponent(props) {
+          let x = props.foo;
+          {
+            let y = props.bar;
+            useEffect(() => {
+              // nothing
+            }, [props.foo, x, y]);
+          }
+        }
+      `,
+      errors: [
+        'React Hook useEffect has unnecessary dependencies: ' +
+          "'MutableStore.hello.world', 'global.stuff', and 'z'. " +
+          'Either exclude them or remove the dependency array. ' +
+          "Values like 'MutableStore.hello.world' aren't valid dependencies " +
+          "because their mutation doesn't re-render the component.",
+      ],
+    },
+    {
+      code: `
+        import MutableStore from 'store';
+        let z = {};
+
+        function MyComponent(props) {
+          let x = props.foo;
+          {
+            let y = props.bar;
+            const fn = useCallback(() => {
+              // nothing
+            }, [MutableStore.hello.world, props.foo, x, y, z, global.stuff]);
+          }
+        }
+      `,
+      output: `
+        import MutableStore from 'store';
+        let z = {};
+
+        function MyComponent(props) {
+          let x = props.foo;
+          {
+            let y = props.bar;
+            const fn = useCallback(() => {
+              // nothing
+            }, []);
+          }
+        }
+      `,
+      errors: [
+        'React Hook useCallback has unnecessary dependencies: ' +
+          "'MutableStore.hello.world', 'global.stuff', 'props.foo', 'x', 'y', and 'z'. " +
+          'Either exclude them or remove the dependency array. ' +
+          "Values like 'MutableStore.hello.world' aren't valid dependencies " +
+          "because their mutation doesn't re-render the component.",
+      ],
+    },
   ],
 };
 
diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
@@ -86,6 +86,7 @@ export default {
       // scope. We can't enforce this in a lint so we trust that all variables
       // declared outside of pure scope are indeed frozen.
       const pureScopes = new Set();
+      let componentScope = null;
       {
         let currentScope = scope.upper;
         while (currentScope) {
@@ -101,6 +102,7 @@ export default {
         if (!currentScope) {
           return;
         }
+        componentScope = currentScope;
       }
 
       // These are usually mistaken. Collect them.
@@ -224,6 +226,7 @@ export default {
       );
 
       const declaredDependencies = [];
+      const externalDependencies = new Set();
       if (declaredDependenciesNode.type !== 'ArrayExpression') {
         // If the declared dependencies are not an array expression then we
         // can't verify that the user provided the correct dependencies. Tell
@@ -298,11 +301,24 @@ export default {
               throw error;
             }
           }
+
+          let maybeID = declaredDependencyNode;
+          while (maybeID.type === 'MemberExpression') {
+            maybeID = maybeID.object;
+          }
+          const isDeclaredInComponent = !componentScope.through.some(
+            ref => ref.identifier === maybeID,
+          );
+
           // Add the dependency to our declared dependency map.
           declaredDependencies.push({
             key: declaredDependency,
             node: declaredDependencyNode,
           });
+
+          if (!isDeclaredInComponent) {
+            externalDependencies.add(declaredDependency);
+          }
         });
       }
 
@@ -348,6 +364,7 @@ export default {
         dependencies,
         declaredDependencies,
         optionalDependencies,
+        externalDependencies,
         isEffect,
       });
 
@@ -370,6 +387,7 @@ export default {
           dependencies,
           declaredDependencies: [], // Pretend we don't know
           optionalDependencies,
+          externalDependencies,
           isEffect,
         }).suggestedDependencies;
       }
@@ -423,6 +441,11 @@ export default {
           extraWarning =
             ` Mutable values like '${badRef}' aren't valid dependencies ` +
             "because their mutation doesn't re-render the component.";
+        } else if (externalDependencies.size > 0) {
+          const dep = Array.from(externalDependencies)[0];
+          extraWarning =
+            ` Values like '${dep}' aren't valid dependencies ` +
+            `because their mutation doesn't re-render the component.`;
         }
       }
 
@@ -462,6 +485,7 @@ function collectRecommendations({
   dependencies,
   declaredDependencies,
   optionalDependencies,
+  externalDependencies,
   isEffect,
 }) {
   // Our primary data structure.
@@ -584,7 +608,11 @@ function collectRecommendations({
         duplicateDependencies.add(key);
       }
     } else {
-      if (isEffect && !key.endsWith('.current')) {
+      if (
+        isEffect &&
+        !key.endsWith('.current') &&
+        !externalDependencies.has(key)
+      ) {
         // Effects are allowed extra "unnecessary" deps.
         // Such as resetting scroll when ID changes.
         // Consider them legit.
