Repository: yannickcr/eslint-plugin-react
Pull request #293 (2015-11-05 11:19:54)

jsx-key
Implementation of proposal for reporting missing `key` props in array literals (#40, #67).

This draft has no options, and just reports on JSX elements in array literals and as the only expression in an arrow function body.

Let me know if you want to add an option to ignore arrow functions and just do array validation.

benmosher: A thought: this would be considered a warning:

```jsx
const props = { key: "x", foo: "bar" }
return [<Hello {...props} />]
```

I think that is an acceptable (and unlikely) edge case such that it would not be worth covering.
yannickcr: > Let me know if you want to add an option to ignore arrow functions and just do array validation.

For what cases would you would disable the rule for arrow functions ? Is there a false positive risk ?

> I think that is an acceptable (and unlikely) edge case such that it would not be worth covering

Agreed to not handle this case for now.

Beside that, it's looks good to me. Can you squash your commits ?




Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 01e19cd2..015017ed 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,22 +3,6 @@ All notable changes to this project will be documented in this file.
 This project adheres to [Semantic Versioning](http://semver.org/).
 This change log adheres to standards from [Keep a CHANGELOG](http://keepachangelog.com).
 
-## [3.8.0] - 2015-11-07
-### Added
-* Add ignoreStateless option to `no-multi-comp` ([#290][])
-
-### Fixed
-* Fix classes with properties to always be marked as components ([#291][])
-* Fix ES5 class detection when using `createClass` by itself ([#297][])
-* Fix direct props detection ([#298][])
-* Ignore functions containing the keyword `this` during component detection
-
-[3.8.0]: https://github.com/yannickcr/eslint-plugin-react/compare/v3.7.1...v3.8.0
-[#290]: https://github.com/yannickcr/eslint-plugin-react/issues/290
-[#291]: https://github.com/yannickcr/eslint-plugin-react/issues/291
-[#297]: https://github.com/yannickcr/eslint-plugin-react/issues/297
-[#298]: https://github.com/yannickcr/eslint-plugin-react/issues/298
-
 ## [3.7.1] - 2015-11-05
 ### Fixed
 * Fix `sort-comp` crash on stateless components ([#285][])
diff --git a/README.md b/README.md
index 89dab748..af9c59e4 100644
--- a/README.md
+++ b/README.md
@@ -52,6 +52,7 @@ Finally, enable all of the rules that you would like to use.
     "react/jsx-closing-bracket-location": 1,
     "react/jsx-curly-spacing": 1,
     "react/jsx-indent-props": 1,
+    "react/jsx-key": 1,
     "react/jsx-max-props-per-line": 1,
     "react/jsx-no-bind": 1,
     "react/jsx-no-duplicate-props": 1,
@@ -88,6 +89,7 @@ Finally, enable all of the rules that you would like to use.
 * [jsx-closing-bracket-location](docs/rules/jsx-closing-bracket-location.md): Validate closing bracket location in JSX
 * [jsx-curly-spacing](docs/rules/jsx-curly-spacing.md): Enforce or disallow spaces inside of curly braces in JSX attributes
 * [jsx-indent-props](docs/rules/jsx-indent-props.md): Validate props indentation in JSX
+* [jsx-key](docs/rules/jsx-key.md): Validate JSX has key prop when in array or iterator
 * [jsx-max-props-per-line](docs/rules/jsx-max-props-per-line.md): Limit maximum of props on a single line in JSX
 * [jsx-no-bind](docs/rules/jsx-no-bind.md): Prevent usage of `.bind()` and arrow functions in JSX props
 * [jsx-no-duplicate-props](docs/rules/jsx-no-duplicate-props.md): Prevent duplicate props in JSX
diff --git a/docs/rules/jsx-key.md b/docs/rules/jsx-key.md
new file mode 100644
index 00000000..8746105b
--- /dev/null
+++ b/docs/rules/jsx-key.md
@@ -0,0 +1,30 @@
+# Detect missing `key` prop (jsx-key)
+
+Warn if an element that likely requires a `key` prop--namely, one present in an
+array literal or an arrow function expression.
+
+## Rule Details
+
+The following patterns are considered warnings:
+
+```jsx
+[<Hello />, <Hello />, <Hello />];
+
+data.map(x => <Hello>x</Hello>);
+```
+
+The following patterns are not considered warnings:
+
+```jsx
+[<Hello key="first" />, <Hello key="second" />, <Hello key="third" />];
+
+data.map((x, i) => <Hello key={i}>x</Hello>);
+```
+
+## When not to use
+
+If you are not using JSX then you can disable this rule.
+
+Also, if you have some prevalent situation where you use arrow functions to 
+return JSX that will not be held in an iterable, you may want to disable this 
+rule.
diff --git a/docs/rules/no-multi-comp.md b/docs/rules/no-multi-comp.md
index 51084027..d71296c0 100644
--- a/docs/rules/no-multi-comp.md
+++ b/docs/rules/no-multi-comp.md
@@ -32,41 +32,6 @@ var HelloJohn = React.createClass({
 });
 ```
 
-## Rule Options
-
-```js
-...
-"no-multi-comp": [<enabled>, { "ignoreStateless": <boolean> }]
-...
-```
-
-### `ignoreStateless`
-
-When `true` the rule will ignore stateless components and will allow you to have multiple stateless components, or one statefull component and some stateless components in the same file.
-
-The following patterns are considered okay and do not cause warnings:
-
-```js
-function Hello(props) {
-  return <div>Hello {props.name}</div>;
-}
-function HelloAgain(props) {
-  return <div>Hello again {props.name}</div>;
-}
-```
-
-```js
-function Hello(props) {
-  return <div>Hello {props.name}</div>;
-}
-class HelloJohn extends React.Component {
-  render() {
-    return <Hello name="John" />;
-  }
-}
-module.exports = HelloJohn;
-```
-
 ## When Not To Use It
 
 If you prefer to declare multiple components per files you can disable this rule.
diff --git a/index.js b/index.js
index fdd87f7e..c23a6d60 100644
--- a/index.js
+++ b/index.js
@@ -31,7 +31,8 @@ module.exports = {
     'jsx-closing-bracket-location': require('./lib/rules/jsx-closing-bracket-location'),
     'no-direct-mutation-state': require('./lib/rules/no-direct-mutation-state'),
     'forbid-prop-types': require('./lib/rules/forbid-prop-types'),
-    'prefer-es6-class': require('./lib/rules/prefer-es6-class')
+    'prefer-es6-class': require('./lib/rules/prefer-es6-class'),
+    'jsx-key': require('./lib/rules/jsx-key')
   },
   rulesConfig: {
     'jsx-uses-react': 0,
@@ -63,6 +64,7 @@ module.exports = {
     'jsx-closing-bracket-location': 0,
     'no-direct-mutation-state': 0,
     'forbid-prop-types': 0,
-    'prefer-es6-class': 0
+    'prefer-es6-class': 0,
+    'jsx-key': 0
   }
 };
diff --git a/lib/rules/jsx-key.js b/lib/rules/jsx-key.js
new file mode 100644
index 00000000..3822ca19
--- /dev/null
+++ b/lib/rules/jsx-key.js
@@ -0,0 +1,35 @@
+/**
+ * @fileoverview Report missing `key` props in iterators/collection literals.
+ * @author Ben Mosher
+ */
+'use strict';
+
+// ------------------------------------------------------------------------------
+// Rule Definition
+// ------------------------------------------------------------------------------
+
+module.exports = function(context) {
+
+  function isKeyProp(decl) {
+    if (decl.type === 'JSXSpreadAttribute') {
+      return false;
+    }
+    return (decl.name.name === 'key');
+  }
+
+  return {
+    JSXElement: function(node) {
+      if (node.openingElement.attributes.some(isKeyProp)) {
+        return; // has key prop
+      }
+
+      if (node.parent.type === 'ArrayExpression') {
+        context.report(node, 'Missing "key" prop for element in array');
+      }
+
+      if (node.parent.type === 'ArrowFunctionExpression') {
+        context.report(node, 'Missing "key" prop for element in iterator');
+      }
+    }
+  };
+};
diff --git a/lib/rules/no-multi-comp.js b/lib/rules/no-multi-comp.js
index 179cdc96..d8db19b5 100644
--- a/lib/rules/no-multi-comp.js
+++ b/lib/rules/no-multi-comp.js
@@ -12,20 +12,8 @@ var Components = require('../util/Components');
 
 module.exports = Components.detect(function(context, components) {
 
-  var configuration = context.options[0] || {};
-  var ignoreStateless = configuration.ignoreStateless || false;
-
   var MULTI_COMP_MESSAGE = 'Declare only one React component per file';
 
-  /**
-   * Checks if the component is ignored
-   * @param {Object} component The component being checked.
-   * @returns {Boolean} True if the component is ignored, false if not.
-   */
-  function isIgnored(component) {
-    return ignoreStateless === true && /Function/.test(component.node.type);
-  }
-
   // --------------------------------------------------------------------------
   // Public
   // --------------------------------------------------------------------------
@@ -40,7 +28,7 @@ module.exports = Components.detect(function(context, components) {
       var i = 0;
 
       for (var component in list) {
-        if (!list.hasOwnProperty(component) || isIgnored(list[component]) || ++i === 1) {
+        if (!list.hasOwnProperty(component) || ++i === 1) {
           continue;
         }
         context.report(list[component].node, MULTI_COMP_MESSAGE);
@@ -49,13 +37,4 @@ module.exports = Components.detect(function(context, components) {
   };
 });
 
-module.exports.schema = [{
-  type: 'object',
-  properties: {
-    ignoreStateless: {
-      default: false,
-      type: 'boolean'
-    }
-  },
-  additionalProperties: false
-}];
+module.exports.schema = [];
diff --git a/lib/rules/prop-types.js b/lib/rules/prop-types.js
index df763fd2..8ed31e47 100644
--- a/lib/rules/prop-types.js
+++ b/lib/rules/prop-types.js
@@ -327,7 +327,7 @@ module.exports = Components.detect(function(context, components) {
    * @return {string} the name of the property or undefined if not found
    */
   function getPropertyName(node) {
-    var isDirectProp = /^props(\.|\[)/.test(context.getSource(node));
+    var isDirectProp = /^props\./.test(context.getSource(node));
     var isInClassComponent = context.react.getParentES6Component() || context.react.getParentES5Component();
     var isNotInConstructor = !inConstructor(node);
     if (isDirectProp && isInClassComponent && isNotInConstructor) {
@@ -418,7 +418,7 @@ module.exports = Components.detect(function(context, components) {
           break;
         }
 
-        var isDirectProp = /^props(\.|\[)/.test(context.getSource(node));
+        var isDirectProp = /^props\./.test(context.getSource(node));
 
         usedPropTypes.push({
           name: name,
diff --git a/lib/util/Components.js b/lib/util/Components.js
index b25bde53..c86dc2f6 100644
--- a/lib/util/Components.js
+++ b/lib/util/Components.js
@@ -21,22 +21,16 @@ function Components() {
  * Add a node to the components list, or update it if it's already in the list
  *
  * @param {ASTNode} node The AST node being added.
- * @param {Number} confidence Confidence in the component detection (0=banned, 1=maybe, 2=yes)
+ * @param {Object} props Additional properties to add to the component.
  */
-Components.prototype.add = function(node, confidence) {
+Components.prototype.add = function(node, props) {
   var id = this._getId(node);
   if (this._list[id]) {
-    if (confidence === 0 || this._list[id].confidence === 0) {
-      this._list[id].confidence = 0;
-    } else {
-      this._list[id].confidence = Math.max(this._list[id].confidence, confidence);
-    }
+    this._list[id] = util._extend(this._list[id], props);
     return;
   }
-  this._list[id] = {
-    node: node,
-    confidence: confidence
-  };
+  props.node = node;
+  this._list[id] = props;
 };
 
 /**
@@ -76,7 +70,7 @@ Components.prototype.set = function(node, props) {
 Components.prototype.list = function() {
   var list = {};
   for (var i in this._list) {
-    if (!this._list.hasOwnProperty(i) || this._list[i].confidence < 2) {
+    if (!this._list.hasOwnProperty(i) || !this._list[i].confident) {
       continue;
     }
     list[i] = this._list[i];
@@ -93,7 +87,7 @@ Components.prototype.list = function() {
 Components.prototype.length = function() {
   var length = 0;
   for (var i in this._list) {
-    if (!this._list.hasOwnProperty(i) || this._list[i].confidence < 2) {
+    if (!this._list.hasOwnProperty(i) || !this._list[i].confident) {
       continue;
     }
     length++;
@@ -116,10 +110,7 @@ function componentRule(rule, context) {
      * @returns {Boolean} True if the node is a React ES5 component, false if not
      */
     isES5Component: function(node) {
-      if (!node.parent) {
-        return false;
-      }
-      return /^(React\.)?createClass$/.test(sourceCode.getText(node.parent.callee));
+      return node.parent && sourceCode.getText(node.parent.callee) === 'React.createClass';
     },
 
     /**
@@ -209,7 +200,7 @@ function componentRule(rule, context) {
         scope = scope.upper;
       }
       var node = scope && scope.block;
-      if (!node || !context.react.isES6Component(node)) {
+      if (!node || !node.superClass) {
         return null;
       }
       return node;
@@ -318,7 +309,7 @@ function componentRule(rule, context) {
       if (!context.react.isES6Component(node)) {
         return;
       }
-      components.add(node, 2);
+      components.add(node, {confident: true});
     },
 
     ClassProperty: function(node) {
@@ -326,14 +317,14 @@ function componentRule(rule, context) {
       if (!node) {
         return;
       }
-      components.add(node, 2);
+      components.add(node, {confident: true});
     },
 
     ObjectExpression: function(node) {
       if (!context.react.isES5Component(node)) {
         return;
       }
-      components.add(node, 2);
+      components.add(node, {confident: true});
     },
 
     FunctionExpression: function(node) {
@@ -341,7 +332,8 @@ function componentRule(rule, context) {
       if (!node) {
         return;
       }
-      components.add(node, 1);
+      var component = components.get(node);
+      components.add(node, {confident: component && component.confident || false});
     },
 
     FunctionDeclaration: function(node) {
@@ -349,7 +341,8 @@ function componentRule(rule, context) {
       if (!node) {
         return;
       }
-      components.add(node, 1);
+      var component = components.get(node);
+      components.add(node, {confident: component && component.confident || false});
     },
 
     ArrowFunctionExpression: function(node) {
@@ -358,19 +351,11 @@ function componentRule(rule, context) {
         return;
       }
       if (node.expression && context.react.isReturningJSX(node)) {
-        components.add(node, 2);
+        components.add(node, {confident: true});
       } else {
-        components.add(node, 1);
-      }
-    },
-
-    ThisExpression: function(node) {
-      node = context.react.getParentComponent();
-      if (!node || !/Function/.test(node.type)) {
-        return;
+        var component = components.get(node);
+        components.add(node, {confident: component && component.confident || false});
       }
-      // Ban functions with a ThisExpression
-      components.add(node, 0);
     },
 
     ReturnStatement: function(node) {
@@ -381,7 +366,7 @@ function componentRule(rule, context) {
       if (!node) {
         return;
       }
-      components.add(node, 2);
+      components.add(node, {confident: true});
     }
   };
 
diff --git a/package.json b/package.json
index 8f3916b6..b35e394c 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "eslint-plugin-react",
-  "version": "3.8.0",
+  "version": "3.7.1",
   "author": "Yannick Croissant <yannick.croissant+npm@gmail.com>",
   "description": "React specific linting rules for ESLint",
   "main": "index.js",


Test patch:
diff --git a/tests/lib/rules/display-name.js b/tests/lib/rules/display-name.js
index 3423fa53..33045eaf 100644
--- a/tests/lib/rules/display-name.js
+++ b/tests/lib/rules/display-name.js
@@ -57,16 +57,6 @@ ruleTester.run('display-name', rule, {
       classes: true,
       jsx: true
     }
-  }, {
-    code: [
-      'class Hello extends Greetings {',
-      '  static text = \'Hello World\';',
-      '  render() {',
-      '    return Hello.text;',
-      '  }',
-      '}'
-    ].join('\n'),
-    parser: 'babel-eslint'
   }, {
     code: [
       'class Hello {',
@@ -318,27 +308,6 @@ ruleTester.run('display-name', rule, {
     options: [{
       acceptTranspilerName: true
     }]
-  }, {
-    code: [
-      'export default {',
-      '  renderHello() {',
-      '    let {name} = this.props;',
-      '    return <div>{name}</div>;',
-      '  }',
-      '};'
-    ].join('\n'),
-    parser: 'babel-eslint'
-  }, {
-    code: [
-      'import React, { createClass } from \'react\';',
-      'export default createClass({',
-      '  displayName: \'Foo\',',
-      '  render() {',
-      '    return <h1>foo</h1>;',
-      '  }',
-      '});'
-    ].join('\n'),
-    parser: 'babel-eslint'
   }
 ],
 
@@ -409,7 +378,7 @@ ruleTester.run('display-name', rule, {
   }, {
     code: [
       'module.exports = () => {',
-      '  return <div>Hello {props.name}</div>;',
+      '  return <div>Hello {this.props.name}</div>;',
       '}'
     ].join('\n'),
     parser: 'babel-eslint',
@@ -422,7 +391,7 @@ ruleTester.run('display-name', rule, {
   }, {
     code: [
       'module.exports = function() {',
-      '  return <div>Hello {props.name}</div>;',
+      '  return <div>Hello {this.props.name}</div>;',
       '}'
     ].join('\n'),
     parser: 'babel-eslint',
diff --git a/tests/lib/rules/jsx-key.js b/tests/lib/rules/jsx-key.js
new file mode 100644
index 00000000..f358c52d
--- /dev/null
+++ b/tests/lib/rules/jsx-key.js
@@ -0,0 +1,42 @@
+/**
+ * @fileoverview Report missing `key` props in iterators/collection literals.
+ * @author Ben Mosher
+ */
+'use strict';
+
+// ------------------------------------------------------------------------------
+// Requirements
+// ------------------------------------------------------------------------------
+
+var rule = require('../../../lib/rules/jsx-key');
+var RuleTester = require('eslint').RuleTester;
+
+// ------------------------------------------------------------------------------
+// Tests
+// ------------------------------------------------------------------------------
+
+var ruleTester = new RuleTester();
+ruleTester.run('jsx-key', rule, {
+  valid: [
+    {code: '<App />;', ecmaFeatures: {jsx: true}},
+    {code: '[<App key={0} />, <App key={1} />];', ecmaFeatures: {jsx: true}},
+    {code: '[1, 2, 3].map(x => <App key={x} />);', ecmaFeatures: {jsx: true, arrowFunctions: true}}
+  ],
+  invalid: [
+    {code: '[<App />];',
+     errors: [{message: 'Missing "key" prop for element in array'}],
+     ecmaFeatures: {jsx: true}},
+
+    {code: '[<App {...key} />];',
+     errors: [{message: 'Missing "key" prop for element in array'}],
+     ecmaFeatures: {jsx: true}},
+
+    {code: '[<App key={0}/>, <App />];',
+     errors: [{message: 'Missing "key" prop for element in array'}],
+     ecmaFeatures: {jsx: true}},
+
+    {code: '[1, 2 ,3].map(x => <App />);',
+     errors: [{message: 'Missing "key" prop for element in iterator'}],
+     ecmaFeatures: {jsx: true, arrowFunctions: true}}
+  ]
+});
diff --git a/tests/lib/rules/no-multi-comp.js b/tests/lib/rules/no-multi-comp.js
index 205990e4..9c269871 100644
--- a/tests/lib/rules/no-multi-comp.js
+++ b/tests/lib/rules/no-multi-comp.js
@@ -62,51 +62,6 @@ ruleTester.run('no-multi-comp', rule, {
       classes: true,
       jsx: true
     }
-  }, {
-    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) {',
-      '  return <div>Hello {props.name}</div>;',
-      '}',
-      'function HelloAgain(props) {',
-      '  return <div>Hello again {props.name}</div>;',
-      '}'
-    ].join('\n'),
-    parser: 'babel-eslint',
-    options: [{
-      ignoreStateless: true
-    }]
-  }, {
-    code: [
-      'function Hello(props) {',
-      '  return <div>Hello {props.name}</div>;',
-      '}',
-      'class HelloJohn extends React.Component {',
-      '  render() {',
-      '    return <Hello name="John" />;',
-      '  }',
-      '}'
-    ].join('\r'),
-    ecmaFeatures: {
-      classes: true,
-      jsx: true
-    },
-    options: [{
-      ignoreStateless: true
-    }]
   }],
 
   invalid: [{
@@ -158,38 +113,5 @@ ruleTester.run('no-multi-comp', rule, {
       message: 'Declare only one React component per file',
       line: 11
     }]
-  }, {
-    code: [
-      'function Hello(props) {',
-      '  return <div>Hello {props.name}</div>;',
-      '}',
-      'function HelloAgain(props) {',
-      '  return <div>Hello again {props.name}</div>;',
-      '}'
-    ].join('\n'),
-    parser: 'babel-eslint',
-    errors: [{
-      message: 'Declare only one React component per file',
-      line: 4
-    }]
-  }, {
-    code: [
-      'function Hello(props) {',
-      '  return <div>Hello {props.name}</div>;',
-      '}',
-      'class HelloJohn extends React.Component {',
-      '  render() {',
-      '    return <Hello name="John" />;',
-      '  }',
-      '}'
-    ].join('\r'),
-    ecmaFeatures: {
-      classes: true,
-      jsx: true
-    },
-    errors: [{
-      message: 'Declare only one React component per file',
-      line: 4
-    }]
   }]
 });
diff --git a/tests/lib/rules/prop-types.js b/tests/lib/rules/prop-types.js
index 95c990c2..a44671e1 100644
--- a/tests/lib/rules/prop-types.js
+++ b/tests/lib/rules/prop-types.js
@@ -883,27 +883,6 @@ ruleTester.run('prop-types', rule, {
         '};'
       ].join('\n'),
       parser: 'babel-eslint'
-    }, {
-      code: [
-        'export default {',
-        '  renderHello() {',
-        '    let {name} = this.props;',
-        '    return <div>{name}</div>;',
-        '  }',
-        '};'
-      ].join('\n'),
-      parser: 'babel-eslint'
-    }, {
-      // Reassigned props are ignored
-      code: [
-        'export class Hello extends Component {',
-        '  render() {',
-        '    const props = this.props;',
-        '    return <div>Hello {props.name.firstname} {props[\'name\'].lastname}</div>',
-        '  }',
-        '}'
-      ].join('\n'),
-      parser: 'babel-eslint'
     }
   ],