From d3368beeecb2b51f51814f95f977e8c11900f54e Mon Sep 17 00:00:00 2001
From: Ian Obermiller <ian@obermillers.com>
Date: Wed, 18 Mar 2020 12:55:13 -0700
Subject: [PATCH] [eslint-plugin-react-hooks] Disallow hooks in class
 components (#18341)

Per discussion at Facebook, we think hooks have reached a tipping point where it is more valuable to lint against potential hooks in classes than to worry about false positives.

Test plan:
```
# run from repo root
yarn test --watch RuleOfHooks
```
---
 .../__tests__/ESLintRulesOfHooks-test.js      | 94 +++++++++++--------
 .../src/RulesOfHooks.js                       | 11 ++-
 2 files changed, 61 insertions(+), 44 deletions(-)

diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
index ad932fe4d792b..73bb86989c8bd 100644
--- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
+++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
@@ -232,45 +232,6 @@ const tests = {
         }
       }
     `,
-    `
-      // Currently valid because we found this to be a common pattern
-      // for feature flag checks in existing components.
-      // We *could* make it invalid but that produces quite a few false positives.
-      // Why does it make sense to ignore it? Firstly, because using
-      // hooks in a class would cause a runtime error anyway.
-      // But why don't we care about the same kind of false positive in a functional
-      // component? Because even if it was a false positive, it would be confusing
-      // anyway. So it might make sense to rename a feature flag check in that case.
-      class ClassComponentWithFeatureFlag extends React.Component {
-        render() {
-          if (foo) {
-            useFeatureFlag();
-          }
-        }
-      }
-    `,
-    `
-      // Currently valid because we don't check for hooks in classes.
-      // See ClassComponentWithFeatureFlag for rationale.
-      // We *could* make it invalid if we don't regress that false positive.
-      class ClassComponentWithHook extends React.Component {
-        render() {
-          React.useState();
-        }
-      }
-    `,
-    `
-      // Currently valid.
-      // These are variations capturing the current heuristic--
-      // we only allow hooks in PascalCase, useFoo functions,
-      // or classes (due to common false positives and because they error anyway).
-      // We *could* make some of these invalid.
-      // They probably don't matter much.
-      (class {useHook = () => { useState(); }});
-      (class {useHook() { useState(); }});
-      (class {h = () => { useState(); }});
-      (class {i() { useState(); }});
-    `,
     `
       // Valid because they're not matching use[A-Z].
       fooState();
@@ -870,6 +831,52 @@ const tests = {
       `,
       errors: [topLevelError('useBasename')],
     },
+    {
+      code: `
+        class ClassComponentWithFeatureFlag extends React.Component {
+          render() {
+            if (foo) {
+              useFeatureFlag();
+            }
+          }
+        }
+      `,
+      errors: [classError('useFeatureFlag')],
+    },
+    {
+      code: `
+        class ClassComponentWithHook extends React.Component {
+          render() {
+            React.useState();
+          }
+        }
+      `,
+      errors: [classError('React.useState')],
+    },
+    {
+      code: `
+        (class {useHook = () => { useState(); }});
+      `,
+      errors: [classError('useState')],
+    },
+    {
+      code: `
+        (class {useHook() { useState(); }});
+      `,
+      errors: [classError('useState')],
+    },
+    {
+      code: `
+        (class {h = () => { useState(); }});
+      `,
+      errors: [classError('useState')],
+    },
+    {
+      code: `
+        (class {i() { useState(); }});
+      `,
+      errors: [classError('useState')],
+    },
   ],
 };
 
@@ -919,6 +926,15 @@ function topLevelError(hook) {
   };
 }
 
+function classError(hook) {
+  return {
+    message:
+      `React Hook "${hook}" cannot be called in a class component. React Hooks ` +
+      'must be called in a React function component or a custom React ' +
+      'Hook function.',
+  };
+}
+
 // For easier local testing
 if (!process.env.CI) {
   let only = [];
diff --git a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
index 0e5dac014611b..c5b2e73f83ff4 100644
--- a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
+++ b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
@@ -461,11 +461,12 @@ export default {
                 codePathNode.parent.type === 'ClassProperty') &&
               codePathNode.parent.value === codePathNode
             ) {
-              // Ignore class methods for now because they produce too many
-              // false positives due to feature flag checks. We're less
-              // sensitive to them in classes because hooks would produce
-              // runtime errors in classes anyway, and because a use*()
-              // call in a class, if it works, is unambiguously *not* a hook.
+              // Custom message for hooks inside a class
+              const message =
+                `React Hook "${context.getSource(hook)}" cannot be called ` +
+                'in a class component. React Hooks must be called in a ' +
+                'React function component or a custom React Hook function.';
+              context.report({node: hook, message});
             } else if (codePathFunctionName) {
               // Custom message if we found an invalid function name.
               const message =