From e1e45fb3673b7d23b4f89ab9fa9644fe9539eb9c Mon Sep 17 00:00:00 2001
From: Dan Abramov <dan.abramov@gmail.com>
Date: Fri, 1 Mar 2019 19:48:48 +0000
Subject: [PATCH] [ESLint] Suggest to destructure props when they are only used
 as members (#14993)

* Suggest to destructure props when they are only used as members

* Add more tests

* Fix a bug
---
 .../ESLintRuleExhaustiveDeps-test.js          | 184 +++++++++++++++++-
 .../src/ExhaustiveDeps.js                     |  40 ++++
 2 files changed, 223 insertions(+), 1 deletion(-)

diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
index 1cef754c752cf..8dd36917b0679 100644
--- 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
index 7db679d0cc225..9fd60d960e45c 100644
--- 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: