Repository: yannickcr/eslint-plugin-react
Pull request #978 (2016-11-25 18:27:46)

Add no-array-index-key rule
This rule will warn when using the array index as the `key`. As a first
pass, I decided to implement this for `Array.prototype.map`, since that
is likely the most common case, but it theoretically could be expanded
to find other cases so I kept the naming more generic.

Like many rules, this one is imperfect and is prone to some false
positives and negatives. For instance, if someone defines a `.map()`
function on an object that doesn't have the same signature as
`Array.prototype.map`, this will likely end up warning in those cases.
However, I think the value of this rule outweighs its hypothetical
drawbacks.

EvNaverniouk: Nice. Would be really great if this could also support `React.cloneElement`, ala:

```
[].map((item, i) => {
    return React.cloneElement(someChild, {
        key: i
    })
})
```
lencioni: All feedback has been addressed! PTAL
lencioni: > Also, what about the .forEach method of a Map or Set?

@ljharb What do you mean by this question? Are you referring to the name of the rule and documentation mentioning arrays?
ljharb: @lencioni i mean, `aMapOrSet.forEach(x => { somethingElse.push(<div key={x} />); })` or similar. Will these be caught as well, since there's no way to know if `aMapOrSet` is an array or not?
lencioni: @ljharb Yes, these will be caught as well for precisely the reason you mention. Likewise, so will any instance of any class that has one of these methods on it. This is what I was referring to about potential false positives in my commit message.

Patch:
diff --git a/README.md b/README.md
index 8cd23d34..a1fb39b0 100644
--- a/README.md
+++ b/README.md
@@ -83,6 +83,7 @@ Finally, enable all of the rules that you would like to use.  Use [our preset](#
 * [react/display-name](docs/rules/display-name.md): Prevent missing `displayName` in a React component definition
 * [react/forbid-component-props](docs/rules/forbid-component-props.md): Forbid certain props on Components
 * [react/forbid-prop-types](docs/rules/forbid-prop-types.md): Forbid certain propTypes
+* [react/no-array-index-key](docs/rules/no-array-index-key.md): Prevent using Array index in `key` props
 * [react/no-children-prop](docs/rules/no-children-prop.md): Prevent passing children as props
 * [react/no-danger](docs/rules/no-danger.md): Prevent usage of dangerous JSX properties
 * [react/no-danger-with-children](docs/rules/no-danger-with-children.md): Prevent problem with children and props.dangerouslySetInnerHTML
diff --git a/docs/rules/no-array-index-key.md b/docs/rules/no-array-index-key.md
new file mode 100644
index 00000000..ba0bde50
--- /dev/null
+++ b/docs/rules/no-array-index-key.md
@@ -0,0 +1,97 @@
+# Prevent usage of Array index in keys
+
+Warn if an element uses an Array index in its `key`.
+
+The `key` is used by React to [identify which items have changed, are added, or are removed and should be stable](https://facebook.github.io/react/docs/lists-and-keys.html#keys).
+
+It's a bad idea to use the array index since it doesn't uniquely identify your elements. In cases where the array is sorted or an element is added to the beginning of the array, the index will be changed even though the element representing that index may be the same. This results in in unnecessary renders.
+
+## Rule Details
+
+The following patterns are considered warnings:
+
+```jsx
+things.map((thing, index) => (
+  <Hello key={index} />
+));
+
+things.map((thing, index) => (
+  React.cloneElement(thing, { key: index })
+));
+
+things.forEach((thing, index) => {
+  otherThings.push(<Hello key={index} />);
+});
+
+things.filter((thing, index) => {
+  otherThings.push(<Hello key={index} />);
+});
+
+things.some((thing, index) => {
+  otherThings.push(<Hello key={index} />);
+});
+
+things.every((thing, index) => {
+  otherThings.push(<Hello key={index} />);
+});
+
+things.find((thing, index) => {
+  otherThings.push(<Hello key={index} />);
+});
+
+things.findIndex((thing, index) => {
+  otherThings.push(<Hello key={index} />);
+});
+
+things.reduce((collection, thing, index) => (
+  collection.concat(<Hello key={index} />)
+), []);
+
+things.reduceRight((collection, thing, index) => (
+  collection.concat(<Hello key={index} />)
+), []);
+```
+
+The following patterns are not considered warnings:
+
+```jsx
+things.map((thing) => (
+  <Hello key={thing.id} />
+));
+
+things.map((thing) => (
+  React.cloneElement(thing, { key: thing.id })
+));
+
+things.forEach((thing) => {
+  otherThings.push(<Hello key={thing.id} />);
+});
+
+things.filter((thing) => {
+  otherThings.push(<Hello key={thing.id} />);
+});
+
+things.some((thing) => {
+  otherThings.push(<Hello key={thing.id} />);
+});
+
+things.every((thing) => {
+  otherThings.push(<Hello key={thing.id} />);
+});
+
+things.find((thing) => {
+  otherThings.push(<Hello key={thing.id} />);
+});
+
+things.findIndex((thing) => {
+  otherThings.push(<Hello key={thing.id} />);
+});
+
+things.reduce((collection, thing) => (
+  collection.concat(<Hello key={thing.id} />)
+), []);
+
+things.reduceRight((collection, thing) => (
+  collection.concat(<Hello key={thing.id} />)
+), []);
+```
diff --git a/index.js b/index.js
index 59a74d5d..63a35ed1 100644
--- a/index.js
+++ b/index.js
@@ -8,6 +8,7 @@ var allRules = {
   'jsx-wrap-multilines': require('./lib/rules/jsx-wrap-multilines'),
   'self-closing-comp': require('./lib/rules/self-closing-comp'),
   'jsx-no-comment-textnodes': require('./lib/rules/jsx-no-comment-textnodes'),
+  'no-array-index-key': require('./lib/rules/no-array-index-key'),
   'no-danger': require('./lib/rules/no-danger'),
   'no-set-state': require('./lib/rules/no-set-state'),
   'no-is-mounted': require('./lib/rules/no-is-mounted'),
diff --git a/lib/rules/no-array-index-key.js b/lib/rules/no-array-index-key.js
new file mode 100644
index 00000000..ecc97264
--- /dev/null
+++ b/lib/rules/no-array-index-key.js
@@ -0,0 +1,196 @@
+/**
+ * @fileoverview Prevent usage of Array index in keys
+ * @author Joe Lencioni
+ */
+'use strict';
+
+// ------------------------------------------------------------------------------
+// Rule Definition
+// ------------------------------------------------------------------------------
+
+module.exports = {
+  meta: {
+    docs: {
+      description: 'Prevent usage of Array index in keys',
+      category: 'Best Practices',
+      recommended: false
+    },
+
+    schema: []
+  },
+
+  create: function(context) {
+    // --------------------------------------------------------------------------
+    // Public
+    // --------------------------------------------------------------------------
+    var indexParamNames = [];
+    var iteratorFunctionsToIndexParamPosition = {
+      every: 1,
+      filter: 1,
+      find: 1,
+      findIndex: 1,
+      forEach: 1,
+      map: 1,
+      reduce: 2,
+      reduceRight: 2,
+      some: 1
+    };
+    var ERROR_MESSAGE = 'Do not use Array index in keys';
+
+    function isArrayIndex(node) {
+      return node.type === 'Identifier'
+        && indexParamNames.indexOf(node.name) !== -1;
+    }
+
+    function getMapIndexParamName(node) {
+      var callee = node.callee;
+      if (callee.type !== 'MemberExpression') {
+        return null;
+      }
+      if (callee.property.type !== 'Identifier') {
+        return null;
+      }
+      if (!iteratorFunctionsToIndexParamPosition.hasOwnProperty(callee.property.name)) {
+        return null;
+      }
+
+      var firstArg = node.arguments[0];
+      if (!firstArg) {
+        return null;
+      }
+
+      var isFunction = [
+        'ArrowFunctionExpression',
+        'FunctionExpression'
+      ].indexOf(firstArg.type) !== -1;
+      if (!isFunction) {
+        return null;
+      }
+
+      var params = firstArg.params;
+
+      var indexParamPosition = iteratorFunctionsToIndexParamPosition[callee.property.name];
+      if (params.length < indexParamPosition + 1) {
+        return null;
+      }
+
+      return params[indexParamPosition].name;
+    }
+
+    function getIdentifiersFromBinaryExpression(side) {
+      if (side.type === 'Identifier') {
+        return side;
+      }
+
+      if (side.type === 'BinaryExpression') {
+        // recurse
+        var left = getIdentifiersFromBinaryExpression(side.left);
+        var right = getIdentifiersFromBinaryExpression(side.right);
+        return [].concat(left, right).filter(Boolean);
+      }
+
+      return null;
+    }
+
+    function checkPropValue(node) {
+      if (isArrayIndex(node)) {
+        // key={bar}
+        context.report({
+          node: node,
+          message: ERROR_MESSAGE
+        });
+        return;
+      }
+
+      if (node.type === 'TemplateLiteral') {
+        // key={`foo-${bar}`}
+        node.expressions.filter(isArrayIndex).forEach(function() {
+          context.report({node: node, message: ERROR_MESSAGE});
+        });
+
+        return;
+      }
+
+      if (node.type === 'BinaryExpression') {
+        // key={'foo' + bar}
+        var identifiers = getIdentifiersFromBinaryExpression(node);
+
+        identifiers.filter(isArrayIndex).forEach(function() {
+          context.report({node: node, message: ERROR_MESSAGE});
+        });
+
+        return;
+      }
+    }
+
+    return {
+      CallExpression: function(node) {
+        if (
+          node.callee
+          && node.callee.type === 'MemberExpression'
+          && ['createElement', 'cloneElement'].indexOf(node.callee.property.name) !== -1
+          && node.arguments.length > 1
+        ) {
+          // React.createElement
+          if (!indexParamNames.length) {
+            return;
+          }
+
+          var props = node.arguments[1];
+
+          if (props.type !== 'ObjectExpression') {
+            return;
+          }
+
+          props.properties.forEach(function (prop) {
+            if (prop.key.name !== 'key') {
+              // { foo: bar }
+              return;
+            }
+
+            checkPropValue(prop.value);
+          });
+
+          return;
+        }
+
+        var mapIndexParamName = getMapIndexParamName(node);
+        if (!mapIndexParamName) {
+          return;
+        }
+
+        indexParamNames.push(mapIndexParamName);
+      },
+
+      JSXAttribute: function(node) {
+        if (node.name.name !== 'key') {
+          // foo={bar}
+          return;
+        }
+
+        if (!indexParamNames.length) {
+          // Not inside a call expression that we think has an index param.
+          return;
+        }
+
+        var value = node.value;
+        if (value.type !== 'JSXExpressionContainer') {
+          // key='foo'
+          return;
+        }
+
+        checkPropValue(value.expression);
+      },
+
+      'CallExpression:exit': function(node) {
+        var mapIndexParamName = getMapIndexParamName(node);
+        if (!mapIndexParamName) {
+          return;
+        }
+
+        indexParamNames.pop();
+      }
+    };
+
+  }
+};


Test patch:
diff --git a/tests/lib/rules/no-array-index-key.js b/tests/lib/rules/no-array-index-key.js
new file mode 100644
index 00000000..ee59d6c3
--- /dev/null
+++ b/tests/lib/rules/no-array-index-key.js
@@ -0,0 +1,254 @@
+/**
+ * @fileoverview Tests for no-array-index-key
+ * @author Joe Lencioni
+ */
+
+'use strict';
+
+// -----------------------------------------------------------------------------
+// Requirements
+// -----------------------------------------------------------------------------
+
+var rule = require('../../../lib/rules/no-array-index-key');
+var RuleTester = require('eslint').RuleTester;
+
+var parserOptions = {
+  ecmaVersion: 6,
+  ecmaFeatures: {
+    jsx: true
+  }
+};
+
+// -----------------------------------------------------------------------------
+// Tests
+// -----------------------------------------------------------------------------
+
+var ruleTester = new RuleTester();
+ruleTester.run('no-array-index-key', rule, {
+  valid: [
+    {code: '<Foo key="foo" />;', parserOptions: parserOptions},
+    {code: '<Foo key={i} />;', parserOptions: parserOptions},
+    {code: '<Foo key={`foo-${i}`} />;', parserOptions: parserOptions},
+    {code: '<Foo key={\'foo-\' + i} />;', parserOptions: parserOptions},
+
+    {
+      code: 'foo.bar((baz, i) => <Foo key={i} />)',
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.bar((bar, i) => <Foo key={`foo-${i}`} />)',
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.bar((bar, i) => <Foo key={\'foo-\' + i} />)',
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.map((baz) => <Foo key={baz.id} />)',
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.map((baz, i) => <Foo key={baz.id} />)',
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.map((baz, i) => <Foo key={\'foo\' + baz.id} />)',
+      parserOptions: parserOptions
+    },
+
+    {
+      code: [
+        'foo.map((item, i) => {',
+        '  return React.cloneElement(someChild, {',
+        '    key: item.id',
+        '  })',
+        '})'
+      ].join('\n'),
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.reduce((a, b) => a.concat(<Foo key={b.id} />), [])',
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.reduce((a, b, i) => a.concat(<Foo key={b.id} />), [])',
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.reduceRight((a, b) => a.concat(<Foo key={b.id} />), [])',
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.reduceRight((a, b, i) => a.concat(<Foo key={b.id} />), [])',
+      parserOptions: parserOptions
+    }
+  ],
+
+  invalid: [
+    {
+      code: 'foo.map((bar, i) => <Foo key={i} />)',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: '[{}, {}].map((bar, i) => <Foo key={i} />)',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.map((bar, anything) => <Foo key={anything} />)',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.map((bar, i) => <Foo key={`foo-${i}`} />)',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.map((bar, i) => <Foo key={\'foo-\' + i} />)',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.map((bar, i) => <Foo key={\'foo-\' + i + \'-bar\'} />)',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: [
+        'foo.map((item, i) => {',
+        '  return React.cloneElement(someChild, {',
+        '    key: i',
+        '  })',
+        '})'
+      ].join('\n'),
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.forEach((bar, i) => { baz.push(<Foo key={i} />); })',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.filter((bar, i) => { baz.push(<Foo key={i} />); })',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.some((bar, i) => { baz.push(<Foo key={i} />); })',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.every((bar, i) => { baz.push(<Foo key={i} />); })',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.find((bar, i) => { baz.push(<Foo key={i} />); })',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.findIndex((bar, i) => { baz.push(<Foo key={i} />); })',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.reduce((a, b, i) => a.concat(<Foo key={i} />), [])',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.reduceRight((a, b, i) => a.concat(<Foo key={i} />), [])',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.map((bar, i) => React.createElement(\'Foo\', { key: i }))',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.map((bar, i) => React.createElement(\'Foo\', { key: `foo-${i}` }))',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.map((bar, i) => React.createElement(\'Foo\', { key: \'foo-\' + i }))',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.map((bar, i) => React.createElement(\'Foo\', { key: \'foo-\' + i + \'-bar\' }))',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.forEach((bar, i) => { baz.push(React.createElement(\'Foo\', { key: i })); })',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.filter((bar, i) => { baz.push(React.createElement(\'Foo\', { key: i })); })',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.some((bar, i) => { baz.push(React.createElement(\'Foo\', { key: i })); })',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.every((bar, i) => { baz.push(React.createElement(\'Foo\', { key: i })); })',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.find((bar, i) => { baz.push(React.createElement(\'Foo\', { key: i })); })',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    },
+
+    {
+      code: 'foo.findIndex((bar, i) => { baz.push(React.createElement(\'Foo\', { key: i })); })',
+      errors: [{message: 'Do not use Array index in keys'}],
+      parserOptions: parserOptions
+    }
+  ]
+});