Repository: yannickcr/eslint-plugin-react
Pull request #362 (2015-12-15 19:41:09)

Prevent string literals being used in ref attributes
Initially, the idea in #341 was to check if references in `this.refs` exist. Based on further discussion it was suggested to define a rule to disallow using string literals in reference properties.

yannickcr: Hi, thanks for the rule.

2 minors things though:
* Maybe a less generic rule name (like `no-string-refs` ?), `no-refs` seems to forbid all types of refs.
* Can you add some titles in the documentations? See [other rules documentation](https://github.com/yannickcr/eslint-plugin-react/tree/master/docs/rules) for example.

Beside that I'm totally ok to add this rule :)


Intellicode: Sure! I'll add them tomorrow :)
Intellicode: @yannickcr I've implemented your suggestions, let me know if there is anything else :)
yannickcr: Just missing the title in docs/no-string-refs.md
Can you add it and squash your commits? Then I think we'll be good to go :)

Patch:
diff --git a/README.md b/README.md
index c0e687f9..b0ea9d6a 100644
--- a/README.md
+++ b/README.md
@@ -87,6 +87,7 @@ Finally, enable all of the rules that you would like to use.
     "react/no-direct-mutation-state": 1,
     "react/no-multi-comp": 1,
     "react/no-set-state": 1,
+    "react/no-string-refs": 1,
     "react/no-unknown-property": 1,
     "react/prefer-es6-class": 1,
     "react/prop-types": 1,
@@ -128,8 +129,9 @@ Finally, enable all of the rules that you would like to use.
 * [no-is-mounted](docs/rules/no-is-mounted.md): Prevent usage of `isMounted`
 * [no-multi-comp](docs/rules/no-multi-comp.md): Prevent multiple component definition per file
 * [no-set-state](docs/rules/no-set-state.md): Prevent usage of `setState`
+* [no-string-refs](docs/rules/no-string-refs.md): Prevent using string references in `ref` attribute.
 * [no-unknown-property](docs/rules/no-unknown-property.md): Prevent usage of unknown DOM property
-* [prefer-es6-class](docs/rules/prefer-es6-class.md): Enforce ES5 or ES6 class for React Components
+* [prefer-es6-class](docs/rules/prefer-es6-class.md): Prefer es6 class instead of createClass for React Components
 * [prop-types](docs/rules/prop-types.md): Prevent missing props validation in a React component definition
 * [react-in-jsx-scope](docs/rules/react-in-jsx-scope.md): Prevent missing `React` when using JSX
 * [require-extension](docs/rules/require-extension.md): Restrict file extensions that may be required
diff --git a/docs/rules/no-string-refs.md b/docs/rules/no-string-refs.md
new file mode 100644
index 00000000..97fed0b3
--- /dev/null
+++ b/docs/rules/no-string-refs.md
@@ -0,0 +1,41 @@
+# Prevent using string references (no-string-refs)
+
+Currently, two ways are supported by React to refer to components. The first one, providing a string identifier is considered legacy in the official documentation. Referring to components by setting an property on the `this` object in the reference callback is preferred.
+
+## Rule Details
+
+Invalid:
+
+```js
+var Hello = React.createClass({
+ render: function() {
+  return <div ref="hello">Hello, world.</div>;
+ }
+});
+```
+
+```js
+var Hello = React.createClass({
+  componentDidMount: function() {
+    var component = this.refs.hello;
+    // ...do something with component
+  },
+  render: function() {
+    return <div ref="hello">Hello, world.</div>;
+  }
+});
+```
+
+Valid:
+
+```js
+var Hello = React.createClass({
+  componentDidMount: function() {
+    var component = this.hello;
+    // ...do something with component
+  },
+  render() {
+    return <div ref={c => this.hello = c}>Hello, world.</div>;
+  }
+});
+```
diff --git a/docs/rules/prefer-es6-class.md b/docs/rules/prefer-es6-class.md
index 1c742d02..9f3be1cb 100644
--- a/docs/rules/prefer-es6-class.md
+++ b/docs/rules/prefer-es6-class.md
@@ -1,59 +1,27 @@
-# Enforce ES5 or ES6 class for React Components (prefer-es6-class)
+ES6 class bodies are more terse than traditional object literals. Methods do not require a `function` keyword and no commas are needed to separate them. This refactoring looks as such:
 
-React offers you two way to create traditional components: using the ES5 `React.createClass` method or the new ES6 class system. This rule allow you to enforce one way or another.
-
-## Rule Options
-
-```js
-...
-"prefer-es6-class": [<enabled>, <mode>]
-...
-```
-
-### `always` mode
-
-Will enforce ES6 classes for React Components. This is the default mode.
-
-The following patterns are considered warnings:
+Invalid:
 
 ```js
-var Hello = React.createClass({
-  render: function() {
-    return <div>Hello {this.props.name}</div>;
-  }
+var ExampleComponent = React.createClass({
+ render: function() { 
+  return <div onClick={this._handleClick}>Hello, world.</div>;
+ },
+ _handleClick: function() {
+  console.log(this);
+ }
 });
 ```
 
-The following patterns are not considered warnings:
-
-```js
-class Hello extends React.Component {
-  render() {
-    return <div>Hello {this.props.name}</div>;
-  }
-}
-```
-
-### `never` mode
-
-Will enforce ES5 classes for React Components
-
-The following patterns are considered warnings:
+Valid:
 
 ```js
-class Hello extends React.Component {
-  render() {
-    return <div>Hello {this.props.name}</div>;
-  }
+class ExampleComponent extends React.Component {
+ render() { 
+  return <div onClick={this._handleClick}>Hello, world.</div>;
+ }
+ _handleClick() {
+  console.log(this);
+ }
 }
-```
-
-The following patterns are not considered warnings:
-
-```js
-var Hello = React.createClass({
-  render: function() {
-    return <div>Hello {this.props.name}</div>;
-  }
-});
-```
+```
\ No newline at end of file
diff --git a/index.js b/index.js
index e4827777..59225fd2 100644
--- a/index.js
+++ b/index.js
@@ -36,7 +36,8 @@ module.exports = {
     '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'),
-    'jsx-key': require('./lib/rules/jsx-key')
+    'jsx-key': require('./lib/rules/jsx-key'),
+    'no-string-refs': require('./lib/rules/no-string-refs')
   },
   rulesConfig: {
     'jsx-uses-react': 0,
@@ -73,6 +74,7 @@ module.exports = {
     'no-direct-mutation-state': 0,
     'forbid-prop-types': 0,
     'prefer-es6-class': 0,
-    'jsx-key': 0
+    'jsx-key': 0,
+    'no-string-refs': 0
   }
 };
diff --git a/lib/rules/jsx-key.js b/lib/rules/jsx-key.js
index 566242f1..3822ca19 100644
--- a/lib/rules/jsx-key.js
+++ b/lib/rules/jsx-key.js
@@ -4,69 +4,32 @@
  */
 'use strict';
 
-// var Components = require('../util/Components');
-
 // ------------------------------------------------------------------------------
 // Rule Definition
 // ------------------------------------------------------------------------------
 
 module.exports = function(context) {
 
-  function hasKeyProp(node) {
-    return node.openingElement.attributes.some(function(decl) {
-      if (decl.type === 'JSXSpreadAttribute') {
-        return false;
-      }
-      return (decl.name.name === 'key');
-    });
-  }
-
-  function checkIteratorElement(node) {
-    if (node.type === 'JSXElement' && !hasKeyProp(node)) {
-      context.report(node, 'Missing "key" prop for element in iterator');
+  function isKeyProp(decl) {
+    if (decl.type === 'JSXSpreadAttribute') {
+      return false;
     }
-  }
-
-  function getReturnStatement(body) {
-    return body.filter(function(item) {
-      return item.type === 'ReturnStatement';
-    })[0];
+    return (decl.name.name === 'key');
   }
 
   return {
     JSXElement: function(node) {
-      if (hasKeyProp(node)) {
-        return;
+      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');
       }
-    },
 
-    // Array.prototype.map
-    CallExpression: function (node) {
-      if (node.callee.property.name !== 'map') {
-        return;
-      }
-
-      var fn = node.arguments[0];
-      var isFn = fn.type === 'FunctionExpression';
-      var isArrFn = fn.type === 'ArrowFunctionExpression';
-
-      if (isArrFn && fn.body.type === 'JSXElement') {
-        checkIteratorElement(fn.body);
-      }
-
-      if (isFn || isArrFn) {
-        if (fn.body.type === 'BlockStatement') {
-          checkIteratorElement(
-            getReturnStatement(fn.body.body).argument
-          );
-        }
+      if (node.parent.type === 'ArrowFunctionExpression') {
+        context.report(node, 'Missing "key" prop for element in iterator');
       }
     }
   };
 };
-
-module.exports.schema = [];
diff --git a/lib/rules/no-string-refs.js b/lib/rules/no-string-refs.js
new file mode 100644
index 00000000..e6cd65da
--- /dev/null
+++ b/lib/rules/no-string-refs.js
@@ -0,0 +1,88 @@
+/**
+ * @fileoverview Prevent string definitions for references and prevent referencing this.refs
+ * @author Tom Hastjarjanto
+ */
+'use strict';
+
+var Components = require('../util/Components');
+
+// ------------------------------------------------------------------------------
+// Rule Definition
+// ------------------------------------------------------------------------------
+
+module.exports = Components.detect(function(context, components, utils) {
+  /**
+   * Checks if we are using refs
+   * @param {ASTNode} node The AST node being checked.
+   * @returns {Boolean} True if we are using refs, false if not.
+   */
+  function isRefsUsage(node) {
+    return Boolean(
+      (
+        utils.getParentES6Component() ||
+        utils.getParentES5Component()
+      ) &&
+      node.object.type === 'ThisExpression' &&
+      node.property.name === 'refs'
+    );
+  }
+
+  /**
+   * Checks if we are using a ref attribute
+   * @param {ASTNode} node The AST node being checked.
+   * @returns {Boolean} True if we are using a ref attribute, false if not.
+   */
+  function isRefAttribute(node) {
+    return Boolean(
+      node.type === 'JSXAttribute' &&
+      node.name &&
+      node.name.name === 'ref'
+    );
+  }
+
+  /**
+   * Checks if a node contains a string value
+   * @param {ASTNode} node The AST node being checked.
+   * @returns {Boolean} True if the node contains a string value, false if not.
+   */
+  function containsStringLiteral(node) {
+    return Boolean(
+      node.value &&
+      node.value.type === 'Literal' &&
+      typeof node.value.value === 'string'
+    );
+  }
+
+  /**
+   * Checks if a node contains a string value within a jsx expression
+   * @param {ASTNode} node The AST node being checked.
+   * @returns {Boolean} True if the node contains a string value within a jsx expression, false if not.
+   */
+  function containsStringExpressionContainer(node) {
+    return Boolean(
+      node.value &&
+      node.value.type === 'JSXExpressionContainer' &&
+      node.value.expression &&
+      node.value.expression.type === 'Literal' &&
+      typeof node.value.expression.value === 'string'
+    );
+  }
+
+  return {
+    MemberExpression: function(node) {
+      if (isRefsUsage(node)) {
+        context.report(node, 'Using this.refs is deprecated.');
+      }
+    },
+    JSXAttribute: function(node) {
+      if (
+        isRefAttribute(node) &&
+        (containsStringLiteral(node) || containsStringExpressionContainer(node))
+      ) {
+        context.report(node, 'Using string literals in ref attributes is deprecated.');
+      }
+    }
+  };
+});
+
+module.exports.schema = [];
diff --git a/lib/rules/prefer-es6-class.js b/lib/rules/prefer-es6-class.js
index 618b295e..0a42f3e2 100644
--- a/lib/rules/prefer-es6-class.js
+++ b/lib/rules/prefer-es6-class.js
@@ -1,5 +1,5 @@
 /**
- * @fileoverview Enforce ES5 or ES6 class for React Components
+ * @fileoverview Prefer es6 class instead of createClass for React Component
  * @author Dan Hamilton
  */
 'use strict';
@@ -17,16 +17,11 @@ module.exports = Components.detect(function(context, components, utils) {
     ObjectExpression: function(node) {
       if (utils.isES5Component(node) && configuration === 'always') {
         context.report(node, 'Component should use es6 class instead of createClass');
-      }
-    },
-    ClassDeclaration: function(node) {
-      if (utils.isES6Component(node) && configuration === 'never') {
+      } else if (utils.isES6Component(node) && configuration === 'never') {
         context.report(node, 'Component should use createClass instead of es6 class');
       }
     }
   };
 });
 
-module.exports.schema = [{
-  enum: ['always', 'never']
-}];
+module.exports.schema = [];


Test patch:
diff --git a/tests/lib/rules/jsx-key.js b/tests/lib/rules/jsx-key.js
index 81d6fbe6..f21a4aaf 100644
--- a/tests/lib/rules/jsx-key.js
+++ b/tests/lib/rules/jsx-key.js
@@ -27,11 +27,7 @@ ruleTester.run('jsx-key', rule, {
   valid: [
     {code: '<App />;', parserOptions: parserOptions},
     {code: '[<App key={0} />, <App key={1} />];', parserOptions: parserOptions},
-    {code: '[1, 2, 3].map(function(x) { return <App key={x} /> });', parserOptions: parserOptions},
-    {code: '[1, 2, 3].map(x => <App key={x} />);', parserOptions: parserOptions},
-    {code: '[1, 2, 3].map(x => { return <App key={x} /> });', parserOptions: parserOptions},
-    {code: '[1, 2, 3].foo(x => <App />);', parserOptions: parserOptions},
-    {code: 'var App = () => <div />;', parserOptions: parserOptions}
+    {code: '[1, 2, 3].map(x => <App key={x} />);', parserOptions: parserOptions}
   ],
   invalid: [
     {code: '[<App />];',
@@ -46,15 +42,7 @@ ruleTester.run('jsx-key', rule, {
      errors: [{message: 'Missing "key" prop for element in array'}],
      parserOptions: parserOptions},
 
-    {code: '[1, 2 ,3].map(function(x) { return <App /> });',
-     errors: [{message: 'Missing "key" prop for element in iterator'}],
-     parserOptions: parserOptions},
-
     {code: '[1, 2 ,3].map(x => <App />);',
-     errors: [{message: 'Missing "key" prop for element in iterator'}],
-     parserOptions: parserOptions},
-
-    {code: '[1, 2 ,3].map(x => { return <App /> });',
      errors: [{message: 'Missing "key" prop for element in iterator'}],
      parserOptions: parserOptions}
   ]
diff --git a/tests/lib/rules/no-string-refs.js b/tests/lib/rules/no-string-refs.js
new file mode 100644
index 00000000..29dd02e8
--- /dev/null
+++ b/tests/lib/rules/no-string-refs.js
@@ -0,0 +1,114 @@
+/**
+ * @fileoverview Prevent string definitions for references and prevent referencing this.refs
+ * @author Tom Hastjarjanto
+ */
+'use strict';
+
+// ------------------------------------------------------------------------------
+// Requirements
+// ------------------------------------------------------------------------------
+
+var rule = require('../../../lib/rules/no-string-refs');
+var RuleTester = require('eslint').RuleTester;
+
+require('babel-eslint');
+
+// ------------------------------------------------------------------------------
+// Tests
+// ------------------------------------------------------------------------------
+
+var ruleTester = new RuleTester();
+ruleTester.run('no-refs', rule, {
+
+  valid: [{
+    code: [
+      'var Hello = React.createClass({',
+      '  componentDidMount: function() {',
+      '     var component = this.hello;',
+      '  },',
+      '  render: function() {',
+      '    return <div ref={c => this.hello = c}>Hello {this.props.name}</div>;',
+      '  }',
+      '});'
+    ].join('\n'),
+    parser: 'babel-eslint',
+    ecmaFeatures: {
+      jsx: true
+    }
+  }
+  ],
+
+  invalid: [{
+    code: [
+      'var Hello = React.createClass({',
+      '  componentDidMount: function() {',
+      '     var component = this.refs.hello;',
+      '  },',
+      '  render: function() {',
+      '    return <div>Hello {this.props.name}</div>;',
+      '  }',
+      '});'
+    ].join('\n'),
+    parser: 'babel-eslint',
+    ecmaFeatures: {
+      classes: true,
+      jsx: true
+    },
+    errors: [{
+      message: 'Using this.refs is deprecated.'
+    }]
+  }, {
+    code: [
+      'var Hello = React.createClass({',
+      '  render: function() {',
+      '    return <div ref="hello">Hello {this.props.name}</div>;',
+      '  }',
+      '});'
+    ].join('\n'),
+    parser: 'babel-eslint',
+    ecmaFeatures: {
+      classes: true,
+      jsx: true
+    },
+    errors: [{
+      message: 'Using string literals in ref attributes is deprecated.'
+    }]
+  }, {
+    code: [
+      'var Hello = React.createClass({',
+      '  render: function() {',
+      '    return <div ref={\'hello\'}>Hello {this.props.name}</div>;',
+      '  }',
+      '});'
+    ].join('\n'),
+    parser: 'babel-eslint',
+    ecmaFeatures: {
+      classes: true,
+      jsx: true
+    },
+    errors: [{
+      message: 'Using string literals in ref attributes is deprecated.'
+    }]
+  }, {
+    code: [
+      'var Hello = React.createClass({',
+      '  componentDidMount: function() {',
+      '     var component = this.refs.hello;',
+      '  },',
+      '  render: function() {',
+      '    return <div ref="hello">Hello {this.props.name}</div>;',
+      '  }',
+      '});'
+    ].join('\n'),
+    parser: 'babel-eslint',
+    ecmaFeatures: {
+      classes: true,
+      jsx: true
+    },
+    errors: [{
+      message: 'Using this.refs is deprecated.'
+    }, {
+      message: 'Using string literals in ref attributes is deprecated.'
+    }]
+  }
+]});
diff --git a/tests/lib/rules/prefer-es6-class.js b/tests/lib/rules/prefer-es6-class.js
index d6de9a69..3457f610 100644
--- a/tests/lib/rules/prefer-es6-class.js
+++ b/tests/lib/rules/prefer-es6-class.js
@@ -38,7 +38,8 @@ ruleTester.run('prefer-es6-class', rule, {
       'Hello.displayName = \'Hello\''
     ].join('\n'),
     parserOptions: parserOptions
-  }, {
+  },
+  {
     code: [
       'export default class Hello extends React.Component {',
       '  render() {',
@@ -48,33 +49,15 @@ ruleTester.run('prefer-es6-class', rule, {
       'Hello.displayName = \'Hello\''
     ].join('\n'),
     parserOptions: parserOptions
-  }, {
+  },
+  {
     code: [
       'var Hello = "foo";',
       'module.exports = {};'
     ].join('\n'),
     parserOptions: parserOptions
-  }, {
-    code: [
-      'var Hello = React.createClass({',
-      '  render: function() {',
-      '    return <div>Hello {this.props.name}</div>;',
-      '  }',
-      '});'
-    ].join('\n'),
-    options: ['never'],
-    parserOptions: parserOptions
-  }, {
-    code: [
-      'class Hello extends React.Component {',
-      '  render() {',
-      '    return <div>Hello {this.props.name}</div>;',
-      '  }',
-      '}'
-    ].join('\n'),
-    options: ['always'],
-    parserOptions: parserOptions
-  }],
+  }
+  ],
 
   invalid: [{
     code: [
@@ -89,31 +72,5 @@ ruleTester.run('prefer-es6-class', rule, {
     errors: [{
       message: 'Component should use es6 class instead of createClass'
     }]
-  }, {
-    code: [
-      'var Hello = React.createClass({',
-      '  render: function() {',
-      '    return <div>Hello {this.props.name}</div>;',
-      '  }',
-      '});'
-    ].join('\n'),
-    options: ['always'],
-    parserOptions: parserOptions,
-    errors: [{
-      message: 'Component should use es6 class instead of createClass'
-    }]
-  }, {
-    code: [
-      'class Hello extends React.Component {',
-      '  render() {',
-      '    return <div>Hello {this.props.name}</div>;',
-      '  }',
-      '}'
-    ].join('\n'),
-    options: ['never'],
-    parserOptions: parserOptions,
-    errors: [{
-      message: 'Component should use createClass instead of es6 class'
-    }]
   }
 ]});