foo is enough for both foo.bar and foo.baz

facebook · Jan 11, 2019 · ade085e · ade085e
1 parent 35e19ed
commit ade085e
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 7 deletions.
diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js
@@ -133,6 +133,15 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, {
         }, [foo]);
       }
     `,
+    `
+      // Regression test
+      function MyComponent({ foo }) {
+        useEffect(() => {
+          console.log(foo.length);
+          console.log(foo.slice(0));
+        }, [foo]);
+      }
+    `,
     `
       // Regression test
       function MyComponent({ history }) {
@@ -169,6 +178,19 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, {
         }, [props.foo, props.bar, local]);
       }
     `,
+    {
+      code: `
+        // TODO: we might want to warn "props.foo"
+        // is extraneous because we already have "props".
+        function MyComponent(props) {
+          const local = 42;
+          useEffect(() => {
+            console.log(props.foo);
+            console.log(props.bar);
+          }, [props, props.foo]);
+        }
+      `,
+    },
     {
       code: `
         function MyComponent(props) {
@@ -492,6 +514,19 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, {
         missingError('local'),
       ],
     },
+    {
+      code: `
+        function MyComponent(props) {
+          const local = 42;
+          useEffect(() => {
+            console.log(props.foo);
+            console.log(props.bar);
+            console.log(local);
+          }, [props]);
+        }
+      `,
+      errors: [missingError('local')],
+    },
     {
       code: `
         function MyComponent(props) {

diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js
@@ -272,6 +272,8 @@ export default {
         }
       }
 
+      let usedDependencies = new Set();
+
       // Loop through all our dependencies to make sure they have been declared.
       // If the dependency has not been declared we need to report some errors.
       for (const [dependency, dependencyNodes] of dependencies) {
@@ -281,11 +283,10 @@ export default {
         let candidate = dependency;
         while (candidate) {
           if (declaredDependencies.has(candidate)) {
-            // Yay! Our dependency has been declared. Delete it from
-            // `declaredDependencies` since we will report all remaining unused
-            // declared dependencies.
+            // Yay! Our dependency has been declared.
+            // Record this so we don't report is unused.
             isDeclared = true;
-            declaredDependencies.delete(candidate);
+            usedDependencies.add(candidate);
             break;
           }
           const lastDotAccessIndex = candidate.lastIndexOf('.');
@@ -316,11 +317,15 @@ export default {
 
       // Loop through all the unused declared dependencies and report a warning
       // so the programmer removes the unused dependency from their list.
-      for (const declaredDependencyNode of declaredDependencies.values()) {
+      for (const [dependency, node] of declaredDependencies) {
+        if (usedDependencies.has(dependency)) {
+          continue;
+        }
+
         context.report(
-          declaredDependencyNode,
+          node,
           `React Hook ${context.getSource(reactiveHook)} has an extra ` +
-            `dependency "${context.getSource(declaredDependencyNode)}" which ` +
+            `dependency "${context.getSource(node)}" which ` +
             'is not used in its callback. Removing this dependency may mean ' +
             'the hook needs to execute fewer times.',
         );