[Fix]: only detect functions that return JSX somewhere as components

jsx-eslint · Jul 10, 2020 · b5e82ea · b5e82ea
1 parent ee4bad3
commit b5e82ea
Show file tree

Hide file tree

Showing 3 changed files with 157 additions and 40 deletions.
diff --git a/lib/util/Components.js b/lib/util/Components.js
@@ -419,33 +419,23 @@ function componentRule(rule, context) {
       );
     },
 
-    /**
-     * Check if the node is returning null
-     *
-     * @param {ASTNode} ASTnode The AST node being checked
-     * @returns {Boolean} True if the node is returning null, false if not
-     */
-    isReturningNull(ASTnode) {
-      const nodeAndProperty = utils.getReturnPropertyAndNode(ASTnode);
-      const property = nodeAndProperty.property;
-      const node = nodeAndProperty.node;
-
-      if (!node) {
-        return false;
-      }
-
-      return node[property] && node[property].value === null;
-    },
-
     /**
      * Check if the node is returning JSX or null
      *
      * @param {ASTNode} ASTNode The AST node being checked
      * @param {Boolean} [strict] If true, in a ternary condition the node must return JSX in both cases
      * @returns {Boolean} True if the node is returning JSX or null, false if not
      */
-    isReturningJSXOrNull(ASTNode, strict) {
-      return utils.isReturningJSX(ASTNode, strict) || utils.isReturningNull(ASTNode);
+    isReturningJSXSomewhere(ASTNode, strict) {
+      let returnStatements = [];
+      // Arrow functions without block statements don't have return statements.
+      if (ASTNode.type === 'ArrowFunctionExpression' && ASTNode.body.type !== 'BlockStatement') {
+        returnStatements.push(ASTNode);
+      } else {
+        returnStatements = astUtil.findAllReturnStatements(ASTNode.body);
+      }
+
+      return returnStatements.some((node) => utils.isReturningJSX(node, strict));
     },
 
     getPragmaComponentWrapper(node) {
@@ -613,7 +603,8 @@ function componentRule(rule, context) {
         case 'AssignmentExpression':
         case 'Property':
         case 'ReturnStatement':
-        case 'ExportDefaultDeclaration': {
+        case 'ExportDefaultDeclaration':
+        case 'ArrowFunctionExpression': {
           return true;
         }
         case 'SequenceExpression': {
@@ -635,19 +626,19 @@ function componentRule(rule, context) {
       if (
         node.type === 'FunctionDeclaration'
         && isFirstLetterCapitalized(node.id.name)
-        && utils.isReturningJSXOrNull(node)
+        && utils.isReturningJSXSomewhere(node)
       ) {
         return node;
       }
 
       if (node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression') {
-        if (node.parent.type === 'VariableDeclarator' && utils.isReturningJSXOrNull(node)) {
+        if (node.parent.type === 'VariableDeclarator' && utils.isReturningJSXSomewhere(node)) {
           if (isFirstLetterCapitalized(node.parent.id.name)) {
             return node;
           }
           return undefined;
         }
-        if (utils.isInAllowedPositionForComponent(node) && utils.isReturningJSXOrNull(node)) {
+        if (utils.isInAllowedPositionForComponent(node) && utils.isReturningJSXSomewhere(node)) {
           return node;
         }
 

diff --git a/lib/util/ast.js b/lib/util/ast.js
@@ -201,7 +201,41 @@ function unwrapTSAsExpression(node) {
   return node;
 }
 
+function findAllReturnStatements(ASTNode) {
+  const returnStatements = [];
+  function findReturnStatements(node) {
+    if (node) {
+      if (Array.isArray(node)) {
+        node.forEach((n) => findReturnStatements(n));
+      } else {
+        // eslint-disable-next-line default-case
+        switch (node.type) {
+          case 'ReturnStatement':
+            returnStatements.push(node);
+            break;
+          case 'IfStatement':
+            findReturnStatements(node.consequent);
+            findReturnStatements(node.alternate);
+            break;
+          case 'SwitchStatement':
+            findReturnStatements(node.cases);
+            break;
+          case 'SwitchCase':
+            findReturnStatements(node.consequent);
+            break;
+          case 'BlockStatement':
+            findReturnStatements(node.body);
+            break;
+        }
+      }
+    }
+  }
+  findReturnStatements(ASTNode);
+  return returnStatements;
+}
+
 module.exports = {
+  findAllReturnStatements,
   findReturnStatement,
   getFirstNodeInLine,
   getPropertyName,

diff --git a/tests/lib/rules/no-this-in-sfc.js b/tests/lib/rules/no-this-in-sfc.js
@@ -151,6 +151,42 @@ ruleTester.run('no-this-in-sfc', rule, {
       };
     };`,
     parser: parsers.BABEL_ESLINT
+  }, {
+    code: `
+      export const prepareLogin = new ValidatedMethod({
+        name: "user.prepare",
+        validate: new SimpleSchema({
+        }).validator(),
+        run({ remember }) {
+            if (Meteor.isServer) {
+                const connectionId = this.connection.id;
+                return Methods.prepareLogin(connectionId, remember);
+            }
+            return null;
+        },
+      });
+    `
+  }, {
+    // functions returning null are not necessarily components.
+    code: `
+    class Foo {
+      bar() {
+        function Bar(){
+          return () => {
+            this.something();
+            return null;
+          }
+        }
+      }
+    }`
+  }, {
+    code: `
+    function Foo(props) {
+      if (this.props.foo) {
+        something();
+      }
+      return null;
+    }`
   }],
   invalid: [{
     code: `
@@ -193,15 +229,6 @@ ruleTester.run('no-this-in-sfc', rule, {
       return null;
     }`,
     errors: [{message: ERROR_MESSAGE}]
-  }, {
-    code: `
-    function Foo(props) {
-      if (this.props.foo) {
-        something();
-      }
-      return null;
-    }`,
-    errors: [{message: ERROR_MESSAGE}]
   }, {
     code: 'const Foo = (props) => <span>{this.props.foo}</span>',
     errors: [{message: ERROR_MESSAGE}]
@@ -219,16 +246,81 @@ ruleTester.run('no-this-in-sfc', rule, {
     errors: [{message: ERROR_MESSAGE}, {message: ERROR_MESSAGE}]
   }, {
     code: `
-    class Foo {
-      bar() {
-        function Bar(){
-          return () => {
-            this.something();
-            return null;
+      export default class {
+        renderFooter = () => {
+          return () => (
+            <div>{this.value}</div>
+          );
+        }
+      }
+    `,
+    errors: [{message: ERROR_MESSAGE}],
+    parser: parsers.BABEL_ESLINT
+  }, {
+    code: `
+      export default class {
+        renderFooter = () => () => (
+          <div>{this.value}</div>
+        );
+      }
+    `,
+    errors: [{message: ERROR_MESSAGE}],
+    parser: parsers.BABEL_ESLINT
+  }, {
+    code: `
+      class Foo {
+        bar() {
+          function Bar(){
+            return () => {
+              this.something();
+              return null || <div />;
+            }
           }
         }
       }
-    }`,
+    `,
+    errors: [{message: ERROR_MESSAGE}]
+  }, {
+    code: `
+      class Foo {
+        bar() {
+          function Bar(){
+            return () => {
+              if (this.something()) {
+                return <div />
+              }
+              return null;
+            }
+          }
+        }
+      }
+    `,
     errors: [{message: ERROR_MESSAGE}]
+  }, {
+    code: `
+      export default class {
+        renderFooter = () => () => this.value ? (
+          <div>{this.value}</div>
+        ) : null;
+      }
+    `,
+    errors: [
+      {message: ERROR_MESSAGE, line: 3},
+      {message: ERROR_MESSAGE, line: 4}
+    ],
+    parser: parsers.BABEL_ESLINT
+  }, {
+    code: `
+      export default class {
+        renderFooter = () => () => this.value && (
+          <div>{this.value}</div>
+        );
+      }
+    `,
+    errors: [
+      {message: ERROR_MESSAGE, line: 3},
+      {message: ERROR_MESSAGE, line: 4}
+    ],
+    parser: parsers.BABEL_ESLINT
   }]
 });