Fixes eslint warning when node type is ChainExpression (#19680)

Pascal Fong Kye · web-flow · commit 1396e4a8f564 · 2020-08-29T21:03:23.000+01:00
* Add babel parser which supports ChainExpression

* Add and fix tests for new babel eslint parser

* extract function to mark node

* refactor for compatibility with eslint v7.7.0+

* Update eslint to v7.7.0
Update hook test since eslint now supports nullish coalescing
diff --git a/package.json b/package.json
@@ -7,6 +7,7 @@
     "@babel/cli": "^7.10.5",
     "@babel/code-frame": "^7.10.4",
     "@babel/core": "^7.11.1",
+    "@babel/eslint-parser": "^7.11.4",
     "@babel/helper-module-imports": "^7.10.4",
     "@babel/parser": "^7.11.3",
     "@babel/plugin-external-helpers": "^7.10.4",
@@ -46,7 +47,7 @@
     "create-react-class": "^15.6.3",
     "danger": "^9.2.10",
     "error-stack-parser": "^2.0.6",
-    "eslint": "^7.0.0",
+    "eslint": "^7.7.0",
     "eslint-config-fbjs": "^1.1.1",
     "eslint-config-prettier": "^6.9.0",
     "eslint-plugin-babel": "^5.3.0",
diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
@@ -7972,6 +7972,11 @@ new ESLintTester({
   parserOptions,
 }).run('react-hooks', ReactHooksESLintRule, tests);
 
+new ESLintTester({
+  parser: require.resolve('@babel/eslint-parser'),
+  parserOptions,
+}).run('react-hooks', ReactHooksESLintRule, tests);
+
 new ESLintTester({
   parser: require.resolve('@typescript-eslint/parser'),
   parserOptions,
diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
@@ -752,9 +752,7 @@ const tests = {
       errors: [
         conditionalError('useState'),
         conditionalError('useState'),
-        // TODO: ideally this *should* warn, but ESLint
-        // doesn't plan full support for ?? until it advances.
-        // conditionalError('useState'),
+        conditionalError('useState'),
       ],
     },
     {
diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
@@ -814,9 +814,10 @@ export default {
           let maybeID = declaredDependencyNode;
           while (
             maybeID.type === 'MemberExpression' ||
-            maybeID.type === 'OptionalMemberExpression'
+            maybeID.type === 'OptionalMemberExpression' ||
+            maybeID.type === 'ChainExpression'
           ) {
-            maybeID = maybeID.object;
+            maybeID = maybeID.object || maybeID.expression.object;
           }
           const isDeclaredInComponent = !componentScope.through.some(
             ref => ref.identifier === maybeID,
@@ -1590,6 +1591,27 @@ function getDependency(node) {
   }
 }
 
+/**
+ * Mark a node as either optional or required.
+ * Note: If the node argument is an OptionalMemberExpression, it doesn't necessarily mean it is optional.
+ * It just means there is an optional member somewhere inside.
+ * This particular node might still represent a required member, so check .optional field.
+ */
+function markNode(node, optionalChains, result) {
+  if (optionalChains) {
+    if (node.optional) {
+      // We only want to consider it optional if *all* usages were optional.
+      if (!optionalChains.has(result)) {
+        // Mark as (maybe) optional. If there's a required usage, this will be overridden.
+        optionalChains.set(result, true);
+      }
+    } else {
+      // Mark as required.
+      optionalChains.set(result, false);
+    }
+  }
+}
+
 /**
  * Assuming () means the passed node.
  * (foo) -> 'foo'
@@ -1609,30 +1631,20 @@ function analyzePropertyChain(node, optionalChains) {
     const object = analyzePropertyChain(node.object, optionalChains);
     const property = analyzePropertyChain(node.property, null);
     const result = `${object}.${property}`;
-    if (optionalChains) {
-      // Mark as required.
-      optionalChains.set(result, false);
-    }
+    markNode(node, optionalChains, result);
     return result;
   } else if (node.type === 'OptionalMemberExpression' && !node.computed) {
     const object = analyzePropertyChain(node.object, optionalChains);
     const property = analyzePropertyChain(node.property, null);
     const result = `${object}.${property}`;
-    if (optionalChains) {
-      // Note: OptionalMemberExpression doesn't necessarily mean this node is optional.
-      // It just means there is an optional member somewhere inside.
-      // This particular node might still represent a required member, so check .optional field.
-      if (node.optional) {
-        // We only want to consider it optional if *all* usages were optional.
-        if (!optionalChains.has(result)) {
-          // Mark as (maybe) optional. If there's a required usage, this will be overridden.
-          optionalChains.set(result, true);
-        }
-      } else {
-        // Mark as required.
-        optionalChains.set(result, false);
-      }
-    }
+    markNode(node, optionalChains, result);
+    return result;
+  } else if (node.type === 'ChainExpression' && !node.computed) {
+    const expression = node.expression;
+    const object = analyzePropertyChain(expression.object, optionalChains);
+    const property = analyzePropertyChain(expression.property, null);
+    const result = `${object}.${property}`;
+    markNode(expression, optionalChains, result);
     return result;
   } else {
     throw new Error(`Unsupported node type: ${node.type}`);
diff --git a/scripts/eslint-rules/no-production-logging.js b/scripts/eslint-rules/no-production-logging.js
@@ -9,75 +9,77 @@
 
 'use strict';
 
-module.exports = function(context) {
-  function isInDEVBlock(node) {
-    let done = false;
-    while (!done) {
-      let parent = node.parent;
-      if (!parent) {
-        return false;
-      }
-      if (
-        parent.type === 'IfStatement' &&
-        node === parent.consequent &&
-        parent.test.type === 'Identifier' &&
-        // This is intentionally strict so we can
-        // see blocks of DEV-only code at once.
-        parent.test.name === '__DEV__'
-      ) {
-        return true;
+module.exports = {
+  meta: {
+    fixable: 'code',
+  },
+  create: function(context) {
+    function isInDEVBlock(node) {
+      let done = false;
+      while (!done) {
+        let parent = node.parent;
+        if (!parent) {
+          return false;
+        }
+        if (
+          parent.type === 'IfStatement' &&
+          node === parent.consequent &&
+          parent.test.type === 'Identifier' &&
+          // This is intentionally strict so we can
+          // see blocks of DEV-only code at once.
+          parent.test.name === '__DEV__'
+        ) {
+          return true;
+        }
+        node = parent;
       }
-      node = parent;
     }
-  }
 
-  function reportWrapInDEV(node) {
-    context.report({
-      node: node,
-      message: `Wrap console.{{identifier}}() in an "if (__DEV__) {}" check`,
-      data: {
-        identifier: node.property.name,
-      },
-      fix: function(fixer) {
-        return [
-          fixer.insertTextBefore(node.parent, `if (__DEV__) {`),
-          fixer.insertTextAfter(node.parent, '}'),
-        ];
-      },
-    });
-  }
+    function reportWrapInDEV(node) {
+      context.report({
+        node: node,
+        message: `Wrap console.{{identifier}}() in an "if (__DEV__) {}" check`,
+        data: {
+          identifier: node.property.name,
+        },
+        fix: function(fixer) {
+          return [
+            fixer.insertTextBefore(node.parent, `if (__DEV__) {`),
+            fixer.insertTextAfter(node.parent, '}'),
+          ];
+        },
+      });
+    }
 
-  function reportUnexpectedConsole(node) {
-    context.report({
-      node: node,
-      message: `Unexpected use of console`,
-    });
-  }
+    function reportUnexpectedConsole(node) {
+      context.report({
+        node: node,
+        message: `Unexpected use of console`,
+      });
+    }
 
-  return {
-    meta: {
-      fixable: 'code',
-    },
-    MemberExpression: function(node) {
-      if (
-        node.object.type === 'Identifier' &&
-        node.object.name === 'console' &&
-        node.property.type === 'Identifier'
-      ) {
-        switch (node.property.name) {
-          case 'error':
-          case 'warn': {
-            if (!isInDEVBlock(node)) {
-              reportWrapInDEV(node);
+    return {
+      MemberExpression: function(node) {
+        if (
+          node.object.type === 'Identifier' &&
+          node.object.name === 'console' &&
+          node.property.type === 'Identifier'
+        ) {
+          switch (node.property.name) {
+            case 'error':
+            case 'warn': {
+              if (!isInDEVBlock(node)) {
+                reportWrapInDEV(node);
+              }
+              break;
+            }
+            default: {
+              reportUnexpectedConsole(node);
+              break;
             }
-            break;
-          }
-          default: {
-            reportUnexpectedConsole(node);
-            break;
           }
         }
-      }
-    },
-  };
+      },
+    };
+  },
 };
diff --git a/yarn.lock b/yarn.lock
