Repository: yannickcr/eslint-plugin-react
Pull request #584 (2016-05-03 14:49:16)

jsx-curly-spacing rule breaks with comments
When the `jsx-curly-spacing` rule is set to `never` the rule reports an error when there is a comment in the expression. I've added a failing test to illustrate this but I'm not sure how to go about fixing it. Any pointers?

Using https://astexplorer.net/#/Y9uxLiaZ7W it looks like it should be possible to use the `trailingComments` and `leadingComments` attributes on the node to take comments into account when comparing ranges though I'm not sure how to access them in the `isSpaced` function (https://github.com/jkimbo/eslint-plugin-react/blob/2b5a612755723862ec51790b7a6037d3ed68e594/lib/rules/jsx-curly-spacing.js#L37-L45)


Patch:
diff --git a/lib/rules/forbid-prop-types.js b/lib/rules/forbid-prop-types.js
index 16269430..19b10452 100644
--- a/lib/rules/forbid-prop-types.js
+++ b/lib/rules/forbid-prop-types.js
@@ -53,6 +53,9 @@ module.exports = function(context) {
    */
   function checkForbidden(declarations) {
     declarations.forEach(function(declaration) {
+      if (declaration.type !== 'Property') {
+        return;
+      }
       var target;
       var value = declaration.value;
       if (
diff --git a/lib/rules/require-render-return.js b/lib/rules/require-render-return.js
index 1c82cb90..a94ac44f 100644
--- a/lib/rules/require-render-return.js
+++ b/lib/rules/require-render-return.js
@@ -22,6 +22,54 @@ module.exports = Components.detect(function(context, components, utils) {
     });
   }
 
+  /**
+   * Get properties for a given AST node
+   * @param {ASTNode} node The AST node being checked.
+   * @returns {Array} Properties array.
+   */
+  function getComponentProperties(node) {
+    switch (node.type) {
+      case 'ClassDeclaration':
+        return node.body.body;
+      case 'ObjectExpression':
+        return node.properties;
+      default:
+        return [];
+    }
+  }
+
+  /**
+   * Get properties name
+   * @param {Object} node - Property.
+   * @returns {String} Property name.
+   */
+  function getPropertyName(node) {
+    // Special case for class properties
+    // (babel-eslint does not expose property name so we have to rely on tokens)
+    if (node.type === 'ClassProperty') {
+      var tokens = context.getFirstTokens(node, 2);
+      return tokens[1] && tokens[1].type === 'Identifier' ? tokens[1].value : tokens[0].value;
+    }
+
+    return node.key.name;
+  }
+
+  /**
+   * Check if a given AST node has a render method
+   * @param {ASTNode} node The AST node being checked.
+   * @returns {Boolean} True if there is a render method, false if not
+   */
+  function hasRenderMethod(node) {
+    var properties = getComponentProperties(node);
+    for (var i = 0, j = properties.length; i < j; i++) {
+      if (getPropertyName(properties[i]) !== 'render') {
+        continue;
+      }
+      return /FunctionExpression$/.test(properties[i].value.type);
+    }
+    return false;
+  }
+
   return {
     ReturnStatement: function(node) {
       var ancestors = context.getAncestors(node).reverse();
@@ -31,8 +79,8 @@ module.exports = Components.detect(function(context, components, utils) {
           depth++;
         }
         if (
-          (ancestors[i].type !== 'Property' && ancestors[i].type !== 'MethodDefinition') ||
-          ancestors[i].key.name !== 'render' ||
+          !/(MethodDefinition|(Class)?Property)$/.test(ancestors[i].type) ||
+          getPropertyName(ancestors[i]) !== 'render' ||
           depth > 1
         ) {
           continue;
@@ -41,11 +89,19 @@ module.exports = Components.detect(function(context, components, utils) {
       }
     },
 
+    ArrowFunctionExpression: function(node) {
+      if (node.expression === false || getPropertyName(node.parent) !== 'render') {
+        return;
+      }
+      markReturnStatementPresent(node);
+    },
+
     'Program:exit': function() {
       var list = components.list();
       for (var component in list) {
         if (
           !list.hasOwnProperty(component) ||
+          !hasRenderMethod(list[component].node) ||
           list[component].hasReturnStatement ||
           (!utils.isES5Component(list[component].node) && !utils.isES6Component(list[component].node))
         ) {
diff --git a/lib/util/Components.js b/lib/util/Components.js
index 039ab554..a213c293 100644
--- a/lib/util/Components.js
+++ b/lib/util/Components.js
@@ -424,11 +424,11 @@ function componentRule(rule, context) {
     },
 
     ThisExpression: function(node) {
-      node = utils.getParentComponent();
-      if (!node || !/Function/.test(node.type)) {
+      var component = utils.getParentComponent();
+      if (!component || !/Function/.test(component.type) || !node.parent.property) {
         return;
       }
-      // Ban functions with a ThisExpression
+      // Ban functions accessing a property on a ThisExpression
       components.add(node, 0);
     },
 


Test patch:
diff --git a/tests/lib/rules/forbid-prop-types.js b/tests/lib/rules/forbid-prop-types.js
index bf277c94..b265baf6 100644
--- a/tests/lib/rules/forbid-prop-types.js
+++ b/tests/lib/rules/forbid-prop-types.js
@@ -188,6 +188,17 @@ ruleTester.run('forbid-prop-types', rule, {
       '});'
     ].join('\n'),
     parserOptions: parserOptions
+  }, {
+    // Proptypes declared with a spread property
+    code: [
+      'class Test extends react.component {',
+      '  static propTypes = {',
+      '    intl: React.propTypes.number,',
+      '    ...propTypes',
+      '  };',
+      '}'
+    ].join('\n'),
+    parser: 'babel-eslint'
   }],
 
   invalid: [{
diff --git a/tests/lib/rules/jsx-curly-spacing.js b/tests/lib/rules/jsx-curly-spacing.js
index 8341e65b..0c6d1a7c 100644
--- a/tests/lib/rules/jsx-curly-spacing.js
+++ b/tests/lib/rules/jsx-curly-spacing.js
@@ -78,6 +78,11 @@ ruleTester.run('jsx-curly-spacing', rule, {
     ].join('\n'),
     options: ['never'],
     parserOptions: parserOptions
+  }, {
+    code: '<App foo={bar/* comment */} />;',
+    options: ['never'],
+    parserOptions: parserOptions,
+    parser: 'babel-eslint'
   }],
 
   invalid: [{
diff --git a/tests/lib/rules/no-multi-comp.js b/tests/lib/rules/no-multi-comp.js
index 745b208a..bbf2f32d 100644
--- a/tests/lib/rules/no-multi-comp.js
+++ b/tests/lib/rules/no-multi-comp.js
@@ -61,20 +61,6 @@ ruleTester.run('no-multi-comp', rule, {
       '});'
     ].join('\r'),
     parserOptions: parserOptions
-  }, {
-    code: [
-      'export default {',
-      '  renderHello() {',
-      '    let {name} = this.props;',
-      '    return <div>{name}</div>;',
-      '  },',
-      '  renderHello2() {',
-      '    let {name2} = this.props;',
-      '    return <div>{name2}</div>;',
-      '  }',
-      '};'
-    ].join('\n'),
-    parser: 'babel-eslint'
   }, {
     code: [
       'function Hello(props) {',
@@ -179,5 +165,23 @@ ruleTester.run('no-multi-comp', rule, {
       message: 'Declare only one React component per file',
       line: 4
     }]
+  }, {
+    code: [
+      'export default {',
+      '  renderHello(props) {',
+      '    let {name} = props;',
+      '    return <div>{name}</div>;',
+      '  },',
+      '  renderHello2(props) {',
+      '    let {name} = props;',
+      '    return <div>{name}</div>;',
+      '  }',
+      '};'
+    ].join('\n'),
+    parser: 'babel-eslint',
+    errors: [{
+      message: 'Declare only one React component per file',
+      line: 6
+    }]
   }]
 });
diff --git a/tests/lib/rules/require-render-return.js b/tests/lib/rules/require-render-return.js
index d29960b2..8da8fbff 100644
--- a/tests/lib/rules/require-render-return.js
+++ b/tests/lib/rules/require-render-return.js
@@ -37,6 +37,28 @@ ruleTester.run('require-render-return', rule, {
       '}'
     ].join('\n'),
     parserOptions: parserOptions
+  }, {
+    // ES6 class with render property
+    code: [
+      'class Hello extends React.Component {',
+      '  render = () => {',
+      '    return <div>Hello {this.props.name}</div>;',
+      '  }',
+      '}'
+    ].join('\n'),
+    parser: 'babel-eslint',
+    parserOptions: parserOptions
+  }, {
+    // ES6 class with render property (implicit return)
+    code: [
+      'class Hello extends React.Component {',
+      '  render = () => (',
+      '    <div>Hello {this.props.name}</div>',
+      '  )',
+      '}'
+    ].join('\n'),
+    parser: 'babel-eslint',
+    parserOptions: parserOptions
   }, {
     // ES5 class
     code: [
@@ -93,6 +115,23 @@ ruleTester.run('require-render-return', rule, {
       '}'
     ].join('\n'),
     parserOptions: parserOptions
+  }, {
+    // ES6 class without a render method
+    code: 'class Hello extends React.Component {}',
+    parserOptions: parserOptions
+  }, {
+    // ES5 class without a render method
+    code: 'var Hello = React.createClass({});',
+    parserOptions: parserOptions
+  }, {
+    // ES5 class with an imported render method
+    code: [
+      'var render = require(\'./render\');',
+      'var Hello = React.createClass({',
+      '  render',
+      '});'
+    ].join('\n'),
+    parserOptions: parserOptions
   }],
 
   invalid: [{
@@ -133,5 +172,19 @@ ruleTester.run('require-render-return', rule, {
     errors: [{
       message: 'Your render method should have return statement'
     }]
+  }, {
+    // Missing return ES6 class render property
+    code: [
+      'class Hello extends React.Component {',
+      '  render = () => {',
+      '    <div>Hello {this.props.name}</div>',
+      '  }',
+      '}'
+    ].join('\n'),
+    parser: 'babel-eslint',
+    parserOptions: parserOptions,
+    errors: [{
+      message: 'Your render method should have return statement'
+    }]
   }
 ]});