From 4b02b6611141255a8e8b6d8a997e87c865f06626 Mon Sep 17 00:00:00 2001
From: Chang Yan <cyan.binary@gmail.com>
Date: Thu, 23 Apr 2020 18:50:52 -0700
Subject: [PATCH] [ESLint] Extend isHook to recognize those under PascalCase's
 namespace (#18722)

* Extend Namespace to PascalCase

* Add valid case for jest.useFakeTimer

* format

* format :(

* fix nits
---
 .../__tests__/ESLintRulesOfHooks-test.js      | 100 ++++++++++--------
 .../src/RulesOfHooks.js                       |   5 +-
 2 files changed, 58 insertions(+), 47 deletions(-)

diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
index 73bb86989c8b..12c4f8ffb71f 100644
--- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
+++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
@@ -106,6 +106,7 @@ const tests = {
       ({useHook() { useState(); }});
       const {useHook3 = () => { useState(); }} = {};
       ({useHook = () => { useState(); }} = {});
+      Namespace.useHook = () => { useState(); };
     `,
     `
       // Valid because hooks can call hooks.
@@ -191,29 +192,6 @@ const tests = {
         }
       }
     `,
-    `
-      // Currently valid.
-      // We *could* make this invalid if we want, but it creates false positives
-      // (see the FooStore case).
-      class C {
-        m() {
-          This.useHook();
-          Super.useHook();
-        }
-      }
-    `,
-    `
-      // Valid although we *could* consider these invalid.
-      // But it doesn't bring much benefit since it's an immediate runtime error anyway.
-      // So might as well allow it.
-      Hook.use();
-      Hook._use();
-      Hook.useState();
-      Hook._useState();
-      Hook.use42();
-      Hook.useHook();
-      Hook.use_hook();
-    `,
     `
       // Valid -- this is a regression test.
       jest.useFakeTimers();
@@ -221,17 +199,6 @@ const tests = {
         jest.useRealTimers();
       })
     `,
-    `
-      // Valid because that's a false positive we've seen quite a bit.
-      // This is a regression test.
-      class Foo extends Component {
-        render() {
-          if (cond) {
-            FooStore.useFeatureFlag();
-          }
-        }
-      }
-    `,
     `
       // Valid because they're not matching use[A-Z].
       fooState();
@@ -239,16 +206,8 @@ const tests = {
       _use();
       _useState();
       use_hook();
-    `,
-    `
-      // This is grey area.
-      // Currently it's valid (although React.useCallback would fail here).
-      // We could also get stricter and disallow it, just like we did
-      // with non-namespace use*() top-level calls.
-      const History = require('history-2.1.2');
-      const browserHistory = History.useBasename(History.createHistory)({
-        basename: '/',
-      });
+      // also valid because it's not matching the PascalCase namespace
+      jest.useFakeTimer()
     `,
     `
       // Regression test for some internal code.
@@ -392,6 +351,59 @@ const tests = {
       `,
       errors: [conditionalError('useConditionalHook')],
     },
+    {
+      code: `
+        Hook.use();
+        Hook._use();
+        Hook.useState();
+        Hook._useState();
+        Hook.use42();
+        Hook.useHook();
+        Hook.use_hook();
+      `,
+      errors: [
+        topLevelError('Hook.useState'),
+        topLevelError('Hook.use42'),
+        topLevelError('Hook.useHook'),
+      ],
+    },
+    {
+      code: `
+        class C {
+          m() {
+            This.useHook();
+            Super.useHook();
+          }
+        }
+      `,
+      errors: [classError('This.useHook'), classError('Super.useHook')],
+    },
+    {
+      code: `
+        // This is a false positive (it's valid) that unfortunately 
+        // we cannot avoid. Prefer to rename it to not start with "use"
+        class Foo extends Component {
+          render() {
+            if (cond) {
+              FooStore.useFeatureFlag();
+            }
+          }
+        }
+      `,
+      errors: [classError('FooStore.useFeatureFlag')],
+    },
+    {
+      code: `
+        // Invalid because it's dangerous and might not warn otherwise.
+        // This *must* be invalid.
+        function ComponentWithConditionalHook() {
+          if (cond) {
+            Namespace.useConditionalHook();
+          }
+        }
+      `,
+      errors: [conditionalError('Namespace.useConditionalHook')],
+    },
     {
       code: `
         // Invalid because it's dangerous and might not warn otherwise.
diff --git a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
index 71d2d42c938d..dda5a1ee3c4f 100644
--- a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
+++ b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
@@ -31,10 +31,9 @@ function isHook(node) {
     !node.computed &&
     isHook(node.property)
   ) {
-    // Only consider React.useFoo() to be namespace hooks for now to avoid false positives.
-    // We can expand this check later.
     const obj = node.object;
-    return obj.type === 'Identifier' && obj.name === 'React';
+    const isPascalCaseNameSpace = /^[A-Z].*/;
+    return obj.type === 'Identifier' && isPascalCaseNameSpace.test(obj.name);
   } else {
     return false;
   }