[ESLint] Suggest to destructure props when they are only used as memb…

…ers (#14993) * Suggest to destructure props when they are only used as members * Add more tests * Fix a bug
facebook · Mar 1, 2019 · e1e45fb · e1e45fb
1 parent 59ef284
commit e1e45fb
Show file tree

Hide file tree

Showing 2 changed files with 223 additions and 1 deletion.
diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
@@ -2224,7 +2224,189 @@ const tests = {
         }
       `,
       errors: [
-        // TODO: make this message clearer since it's not obvious why.
+        "React Hook useEffect has a missing dependency: 'props'. " +
+          'Either include it or remove the dependency array. ' +
+          'Alternatively, destructure the necessary props outside the callback.',
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          useEffect(() => {
+           function play() {
+              props.onPlay();
+            }
+            function pause() {
+              props.onPause();
+            }
+          }, []);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          useEffect(() => {
+           function play() {
+              props.onPlay();
+            }
+            function pause() {
+              props.onPause();
+            }
+          }, [props]);
+        }
+      `,
+      errors: [
+        "React Hook useEffect has a missing dependency: 'props'. " +
+          'Either include it or remove the dependency array. ' +
+          'Alternatively, destructure the necessary props outside the callback.',
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          useEffect(() => {
+            if (props.foo.onChange) {
+              props.foo.onChange();
+            }
+          }, []);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          useEffect(() => {
+            if (props.foo.onChange) {
+              props.foo.onChange();
+            }
+          }, [props.foo]);
+        }
+      `,
+      errors: [
+        "React Hook useEffect has a missing dependency: 'props.foo'. " +
+          'Either include it or remove the dependency array.',
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          useEffect(() => {
+            props.onChange();
+            if (props.foo.onChange) {
+              props.foo.onChange();
+            }
+          }, []);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          useEffect(() => {
+            props.onChange();
+            if (props.foo.onChange) {
+              props.foo.onChange();
+            }
+          }, [props]);
+        }
+      `,
+      errors: [
+        "React Hook useEffect has a missing dependency: 'props'. " +
+          'Either include it or remove the dependency array. ' +
+          'Alternatively, destructure the necessary props outside the callback.',
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          const [skillsCount] = useState();
+          useEffect(() => {
+            if (skillsCount === 0 && !props.isEditMode) {
+              props.toggleEditMode();
+            }
+          }, [skillsCount, props.isEditMode, props.toggleEditMode]);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          const [skillsCount] = useState();
+          useEffect(() => {
+            if (skillsCount === 0 && !props.isEditMode) {
+              props.toggleEditMode();
+            }
+          }, [skillsCount, props.isEditMode, props.toggleEditMode, props]);
+        }
+      `,
+      errors: [
+        "React Hook useEffect has a missing dependency: 'props'. " +
+          'Either include it or remove the dependency array. ' +
+          'Alternatively, destructure the necessary props outside the callback.',
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          const [skillsCount] = useState();
+          useEffect(() => {
+            if (skillsCount === 0 && !props.isEditMode) {
+              props.toggleEditMode();
+            }
+          }, []);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          const [skillsCount] = useState();
+          useEffect(() => {
+            if (skillsCount === 0 && !props.isEditMode) {
+              props.toggleEditMode();
+            }
+          }, [props, skillsCount]);
+        }
+      `,
+      errors: [
+        "React Hook useEffect has missing dependencies: 'props' and 'skillsCount'. " +
+          'Either include them or remove the dependency array. ' +
+          'Alternatively, destructure the necessary props outside the callback.',
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          useEffect(() => {
+            externalCall(props);
+            props.onChange();
+          }, []);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          useEffect(() => {
+            externalCall(props);
+            props.onChange();
+          }, [props]);
+        }
+      `,
+      // Don't suggest to destructure props here since you can't.
+      errors: [
+        "React Hook useEffect has a missing dependency: 'props'. " +
+          'Either include it or remove the dependency array.',
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          useEffect(() => {
+            props.onChange();
+            externalCall(props);
+          }, []);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          useEffect(() => {
+            props.onChange();
+            externalCall(props);
+          }, [props]);
+        }
+      `,
+      // Don't suggest to destructure props here since you can't.
+      errors: [
         "React Hook useEffect has a missing dependency: 'props'. " +
           'Either include it or remove the dependency array.',
       ],

diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
@@ -449,6 +449,46 @@ export default {
         }
       }
 
+      // `props.foo()` marks `props` as a dependency because it has
+      // a `this` value. This warning can be confusing.
+      // So if we're going to show it, append a clarification.
+      if (!extraWarning && missingDependencies.has('props')) {
+        let propDep = dependencies.get('props');
+        if (propDep == null) {
+          return;
+        }
+        const refs = propDep.reference.resolved.references;
+        if (!Array.isArray(refs)) {
+          return;
+        }
+        let isPropsOnlyUsedInMembers = true;
+        for (let i = 0; i < refs.length; i++) {
+          const ref = refs[i];
+          const id = fastFindReferenceWithParent(
+            componentScope.block,
+            ref.identifier,
+          );
+          if (!id) {
+            isPropsOnlyUsedInMembers = false;
+            break;
+          }
+          const parent = id.parent;
+          if (parent == null) {
+            isPropsOnlyUsedInMembers = false;
+            break;
+          }
+          if (parent.type !== 'MemberExpression') {
+            isPropsOnlyUsedInMembers = false;
+            break;
+          }
+        }
+        if (isPropsOnlyUsedInMembers) {
+          extraWarning =
+            ' Alternatively, destructure the necessary props ' +
+            'outside the callback.';
+        }
+      }
+
       context.report({
         node: declaredDependenciesNode,
         message: