Merge pull request #1509 from jomasti/issue-1435

Add new rule for preventing `this` in SFCs
jsx-eslint · Dec 1, 2017 · cfd1c34 · cfd1c34
2 parents 06e1667 + efbb8a5
commit cfd1c34
Show file tree

Hide file tree

Showing 6 changed files with 406 additions and 12 deletions.
diff --git a/README.md b/README.md
@@ -114,6 +114,7 @@ Enable the rules that you would like to use.
 * [react/no-set-state](docs/rules/no-set-state.md): Prevent usage of `setState`
 * [react/no-typos](docs/rules/no-typos.md): Prevent common casing typos
 * [react/no-string-refs](docs/rules/no-string-refs.md): Prevent using string references in `ref` attribute.
+* [react/no-this-in-sfc](docs/rules/no-this-in-sfc.md): Prevent using `this` in stateless functional components
 * [react/no-unescaped-entities](docs/rules/no-unescaped-entities.md): Prevent invalid characters from appearing in markup
 * [react/no-unknown-property](docs/rules/no-unknown-property.md): Prevent usage of unknown DOM property (fixable)
 * [react/no-unused-prop-types](docs/rules/no-unused-prop-types.md): Prevent definitions of unused prop types

diff --git a/docs/rules/no-this-in-sfc.md b/docs/rules/no-this-in-sfc.md
@@ -0,0 +1,136 @@
+# Prevent `this` from being used in stateless functional components (react/no-this-in-sfc)
+
+When using a stateless functional component (SFC), props/context aren't accessed in the same way as a class component or the `create-react-class` format. Both props and context are passed as separate arguments to the component instead. Also, as the name suggests, a stateless component does not have state on `this.state`.
+
+Attempting to access properties on `this` can be a potential error if someone is unaware of the differences when writing a SFC or missed when converting a class component to a SFC.
+
+
+## Rule Details
+
+The following patterns are considered warnings:
+
+```jsx
+function Foo(props) {
+  return (
+    <div>{this.props.bar}</div>
+  );
+}
+```
+
+```jsx
+function Foo(props) {
+  const { bar } = this.props;
+  return (
+    <div>{bar}</div>
+  );
+}
+```
+
+```jsx
+function Foo(props, context) {
+  return (
+    <div>
+      {this.context.foo ? this.props.bar : ''}
+    </div>
+  );
+}
+```
+
+```jsx
+function Foo(props, context) {
+  const { foo } = this.context;
+  const { bar } = this.props;
+  return (
+    <div>
+      {foo ? bar : ''}
+    </div>
+  );
+}
+```
+
+```jsx
+function Foo(props) {
+  if (this.state.loading) {
+    return <Loader />;
+  }
+  return (
+    <div>
+      {this.props.bar}
+    </div>
+  );
+}
+```
+
+```jsx
+function Foo(props) {
+  const { loading } = this.state;
+  const { bar } = this.props;
+  if (loading) {
+    return <Loader />;
+  }
+  return (
+    <div>
+      {bar}
+    </div>
+  );
+}
+```
+
+The following patterns are **not** considered warnings:
+
+```jsx
+function Foo(props) {
+  return (
+    <div>{props.bar}</div>
+  );
+}
+```
+
+```jsx
+function Foo(props) {
+  const { bar } = props;
+  return (
+    <div>{bar}</div>
+  );
+}
+```
+
+```jsx
+function Foo({ bar }) {
+  return (
+    <div>{bar}</div>
+  );
+}
+```
+
+```jsx
+function Foo(props, context) {
+  return (
+    <div>
+      {context.foo ? props.bar : ''}
+    </div>
+  );
+}
+```
+
+```jsx
+function Foo(props, context) {
+  const { foo } = context;
+  const { bar } = props;
+  return (
+    <div>
+      {foo ? bar : ''}
+    </div>
+  );
+}
+```
+
+```jsx
+function Foo({ bar }, { foo }) {
+  return (
+    <div>
+      {foo ? bar : ''}
+    </div>
+  );
+}
+```
diff --git a/index.js b/index.js
@@ -57,6 +57,7 @@ const allRules = {
   'no-string-refs': require('./lib/rules/no-string-refs'),
   'no-redundant-should-component-update': require('./lib/rules/no-redundant-should-component-update'),
   'no-render-return-value': require('./lib/rules/no-render-return-value'),
+  'no-this-in-sfc': require('./lib/rules/no-this-in-sfc'),
   'no-typos': require('./lib/rules/no-typos'),
   'no-unescaped-entities': require('./lib/rules/no-unescaped-entities'),
   'no-unknown-property': require('./lib/rules/no-unknown-property'),

diff --git a/lib/rules/no-this-in-sfc.js b/lib/rules/no-this-in-sfc.js
@@ -0,0 +1,42 @@
+/**
+ * @fileoverview Report "this" being used in stateless functional components.
+ */
+'use strict';
+
+const Components = require('../util/Components');
+
+// ------------------------------------------------------------------------------
+// Constants
+// ------------------------------------------------------------------------------
+
+const ERROR_MESSAGE = 'Stateless functional components should not use this';
+
+// ------------------------------------------------------------------------------
+// Rule Definition
+// ------------------------------------------------------------------------------
+
+module.exports = {
+  meta: {
+    docs: {
+      description: 'Report "this" being used in stateless components',
+      category: 'Possible Errors',
+      recommended: false
+    },
+    schema: []
+  },
+
+  create: Components.detect((context, components, utils) => ({
+    MemberExpression(node) {
+      const component = components.get(utils.getParentStatelessComponent());
+      if (!component) {
+        return;
+      }
+      if (node.object.type === 'ThisExpression') {
+        context.report({
+          node: node,
+          message: ERROR_MESSAGE
+        });
+      }
+    }
+  }))
+};
diff --git a/lib/util/Components.js b/lib/util/Components.js
@@ -303,14 +303,7 @@ function componentRule(rule, context) {
       return calledOnReact;
     },
 
-    /**
-     * Check if the node is returning JSX
-     *
-     * @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, false if not
-     */
-    isReturningJSX: function(ASTnode, strict) {
+    getReturnPropertyAndNode(ASTnode) {
       let property;
       let node = ASTnode;
       switch (node.type) {
@@ -319,14 +312,36 @@ function componentRule(rule, context) {
           break;
         case 'ArrowFunctionExpression':
           property = 'body';
+          if (node[property] && node[property].type === 'BlockStatement') {
+            node = utils.findReturnStatement(node);
+            property = 'argument';
+          }
           break;
         default:
           node = utils.findReturnStatement(node);
-          if (!node) {
-            return false;
-          }
           property = 'argument';
       }
+      return {
+        node: node,
+        property: property
+      };
+    },
+
+    /**
+     * Check if the node is returning JSX
+     *
+     * @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, false if not
+     */
+    isReturningJSX: function(ASTnode, strict) {
+      const nodeAndProperty = utils.getReturnPropertyAndNode(ASTnode);
+      const node = nodeAndProperty.node;
+      const property = nodeAndProperty.property;
+
+      if (!node) {
+        return false;
+      }
 
       const returnsConditionalJSXConsequent =
         node[property] &&
@@ -356,6 +371,35 @@ 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);
+    },
+
     /**
      * Find a return statment in the current node
      *
@@ -430,7 +474,7 @@ function componentRule(rule, context) {
           return null;
         }
         // Return the node if it is a function that is not a class method and is not inside a JSX Element
-        if (isFunction && !isMethod && !isJSXExpressionContainer) {
+        if (isFunction && !isMethod && !isJSXExpressionContainer && utils.isReturningJSXOrNull(node)) {
           return node;
         }
         scope = scope.upper;