Repository: yannickcr/eslint-plugin-react
Pull request #1134 (2017-03-31 00:04:25)

Add a `reservedFirst` option to the `jsx-sort-props` rule
This PR is in response to reading some of the comments in #807 plus real-world needs by my company: we always put the `key` prop first, and then alphabetize the rest of them.

First time writing an ESLint rule option, so please let me know what could be better.

Thinking about this option, I intentionally placed it before `shorthandFirst` so that it trumps that option. This seemed appropriate to me based on thinking about props as reserved or not, and considering that there are no reserved shorthand props.

For "reserved," I am considering all props that are specifically related to React. So for example, DOM properties are not reserved since they are related to JSX, not React.

-------------------------------------------------------

This adds a new option, `reservedFirst`, to the `jsx-sort-props` rule. This option can be enabled with a boolean, or an array configuration. Using the array configuration allows customizing the list of reserved props that are checked.

MatthewHerbst: As alluded to in one of my reply-comments, would your concern be alleviated if the customized list could only be a subset of the full list rather than accepting arbitrary values? I do think having some customization of the option is important for people who might want to use it.

Noted on `style`/`htmlFor`, I'll update, thanks.
ljharb: Yes, I think that if the list must always contain a non-empty subset of the default list, that would alleviate my concerns there (although I don't really see the utility).
MatthewHerbst: @ljharb I've updated based on the discussion. Now the customized reserved list may only be a subset of the actual list. The length of the check got me to move it into its own function to make the core `create` function easier to read. Let me know your thoughts; thanks for being flexible on this, really appreciated

Patch:
diff --git a/docs/rules/jsx-sort-props.md b/docs/rules/jsx-sort-props.md
index 81d1a7bf..71b055b3 100644
--- a/docs/rules/jsx-sort-props.md
+++ b/docs/rules/jsx-sort-props.md
@@ -28,7 +28,8 @@ The following patterns are considered okay and do not cause warnings:
   "shorthandFirst": <boolean>,
   "shorthandLast": <boolean>,
   "ignoreCase": <boolean>,
-  "noSortAlphabetically": <boolean>
+  "noSortAlphabetically": <boolean>,
+  "reservedFirst": <boolean>|<array<string>>,
 }]
 ...
 ```
@@ -75,6 +76,26 @@ When `true`, alphabetical order is not enforced:
 <Hello tel={5555555} name="John" />
 ```
 
+### `reservedFirst`
+
+This can be a boolean or an array option.
+
+When `reservedFirst` is defined, React reserved props (`children`, `dangerouslySetInnerHTML` - **only for DOM components**, `key`, and `ref`) must be listed before all other props, but still respecting the alphabetical order:
+
+```jsx
+<Hello key={0} ref="John" name="John">
+  <div dangerouslySetInnerHTML={{__html: 'ESLint Plugin React!'}} ref="dangerDiv" />
+</Hello>
+```
+
+If given as an array, the array's values will override the default list of reserved props. **Note**: the values in the array may only be a **subset** of React reserved props.
+
+With `reservedFirst: [2, ["key"]]`, the following will not warn:
+
+```jsx
+<Hello key={'uuid'} name="John" ref="ref" />
+```
+
 ## When not to use
 
 This rule is a formatting preference and not following it won't negatively affect the quality of your code. If alphabetizing props isn't a part of your coding standards, then you can leave this rule off.
diff --git a/docs/rules/no-unused-prop-types.md b/docs/rules/no-unused-prop-types.md
index 5fa3698d..6a4014e6 100644
--- a/docs/rules/no-unused-prop-types.md
+++ b/docs/rules/no-unused-prop-types.md
@@ -20,7 +20,7 @@ var Hello = React.createClass({
   propTypes: {
     firstname: React.PropTypes.string.isRequired,
     middlename: React.PropTypes.string.isRequired, // middlename is never used below
-    lastname: React.PropTypes.string.isRequired
+	lastname: React.PropTypes.string.isRequired
   },
   render: function() {
     return <div>Hello {this.props.firstname} {this.props.lastname}</div>;
diff --git a/lib/rules/jsx-sort-props.js b/lib/rules/jsx-sort-props.js
index 335675bf..953352b6 100644
--- a/lib/rules/jsx-sort-props.js
+++ b/lib/rules/jsx-sort-props.js
@@ -4,6 +4,7 @@
  */
 'use strict';
 
+var elementType = require('jsx-ast-utils/elementType');
 var propName = require('jsx-ast-utils/propName');
 
 // ------------------------------------------------------------------------------
@@ -14,6 +15,72 @@ function isCallbackPropName(name) {
   return /^on[A-Z]/.test(name);
 }
 
+var COMPAT_TAG_REGEX = /^[a-z]|\-/;
+function isDOMComponent(node) {
+  var name = elementType(node);
+
+  // Get namespace if the type is JSXNamespacedName or JSXMemberExpression
+  if (name.indexOf(':') > -1) {
+    name = name.substring(0, name.indexOf(':'));
+  } else if (name.indexOf('.') > -1) {
+    name = name.substring(0, name.indexOf('.'));
+  }
+
+  return COMPAT_TAG_REGEX.test(name);
+}
+
+var RESERVED_PROPS_LIST = [
+  'children',
+  'dangerouslySetInnerHTML',
+  'key',
+  'ref'
+];
+
+function isReservedPropName(name, list) {
+  return list.indexOf(name) >= 0;
+}
+
+/**
+ * Checks if the `reservedFirst` option is valid
+ * @param {Object} context The context of the rule
+ * @param {Boolean|Array<String>} reservedFirst The `reservedFirst` option
+ * @return {?Function} If an error is detected, a function to generate the error message, otherwise, `undefined`
+ */
+// eslint-disable-next-line consistent-return
+function validateReservedFirstConfig(context, reservedFirst) {
+  if (reservedFirst) {
+    if (Array.isArray(reservedFirst)) {
+      // Only allow a subset of reserved words in customized lists
+      // eslint-disable-next-line consistent-return
+      var nonReservedWords = reservedFirst.filter(function(word) {
+        if (!isReservedPropName(word, RESERVED_PROPS_LIST)) {
+          return true;
+        }
+      });
+
+      if (reservedFirst.length === 0) {
+        return function(decl) {
+          context.report({
+            node: decl,
+            message: 'A customized reserved first list must not be empty'
+          });
+        };
+      } else if (nonReservedWords.length > 0) {
+        return function(decl) {
+          context.report({
+            node: decl,
+            message: 'A customized reserved first list must only contain a subset of React reserved props.' +
+              ' Remove: {{ nonReservedWords }}',
+            data: {
+              nonReservedWords: nonReservedWords.toString()
+            }
+          });
+        };
+      }
+    }
+  }
+}
+
 module.exports = {
   meta: {
     docs: {
@@ -44,6 +111,9 @@ module.exports = {
         // Whether alphabetical sorting should be enforced
         noSortAlphabetically: {
           type: 'boolean'
+        },
+        reservedFirst: {
+          type: ['array', 'boolean']
         }
       },
       additionalProperties: false
@@ -58,9 +128,19 @@ module.exports = {
     var shorthandFirst = configuration.shorthandFirst || false;
     var shorthandLast = configuration.shorthandLast || false;
     var noSortAlphabetically = configuration.noSortAlphabetically || false;
+    var reservedFirst = configuration.reservedFirst || false;
+    var reservedFirstError = validateReservedFirstConfig(context, reservedFirst);
+    var reservedList = Array.isArray(reservedFirst) ? reservedFirst : RESERVED_PROPS_LIST;
 
     return {
       JSXOpeningElement: function(node) {
+        // `dangerouslySetInnerHTML` is only "reserved" on DOM components
+        if (reservedFirst && !isDOMComponent(node)) {
+          reservedList = reservedList.filter(function(prop) {
+            return prop !== 'dangerouslySetInnerHTML';
+          });
+        }
+
         node.attributes.reduce(function(memo, decl, idx, attrs) {
           if (decl.type === 'JSXSpreadAttribute') {
             return attrs[idx + 1];
@@ -70,15 +150,45 @@ module.exports = {
           var currentPropName = propName(decl);
           var previousValue = memo.value;
           var currentValue = decl.value;
-          var previousIsCallback = isCallbackPropName(previousPropName);
-          var currentIsCallback = isCallbackPropName(currentPropName);
 
           if (ignoreCase) {
             previousPropName = previousPropName.toLowerCase();
             currentPropName = currentPropName.toLowerCase();
           }
 
+          if (reservedFirst) {
+            if (reservedFirstError) {
+              reservedFirstError(decl);
+              return memo;
+            }
+
+            var previousIsReserved = isReservedPropName(previousPropName, reservedList);
+            var currentIsReserved = isReservedPropName(currentPropName, reservedList);
+
+            if (previousIsReserved && currentIsReserved) {
+              if (!noSortAlphabetically && currentPropName < previousPropName) {
+                context.report({
+                  node: decl,
+                  message: 'Props should be sorted alphabetically'
+                });
+                return memo;
+              }
+              return decl;
+            }
+            if (!previousIsReserved && currentIsReserved) {
+              context.report({
+                node: decl,
+                message: 'Reserved props must be listed before all other props'
+              });
+              return memo;
+            }
+            return decl;
+          }
+
           if (callbacksLast) {
+            var previousIsCallback = isCallbackPropName(previousPropName);
+            var currentIsCallback = isCallbackPropName(currentPropName);
+
             if (!previousIsCallback && currentIsCallback) {
               // Entering the callback prop section
               return decl;
diff --git a/lib/rules/no-unused-prop-types.js b/lib/rules/no-unused-prop-types.js
index 744f211c..be905493 100644
--- a/lib/rules/no-unused-prop-types.js
+++ b/lib/rules/no-unused-prop-types.js
@@ -18,7 +18,6 @@ var annotations = require('../util/annotations');
 // ------------------------------------------------------------------------------
 
 var DIRECT_PROPS_REGEX = /^props\s*(\.|\[)/;
-var DIRECT_NEXT_PROPS_REGEX = /^nextProps\s*(\.|\[)/;
 
 // ------------------------------------------------------------------------------
 // Rule Definition
@@ -78,24 +77,6 @@ module.exports = {
       return value;
     }
 
-    /**
-     * Check if we are in a class constructor
-     * @return {boolean} true if we are in a class constructor, false if not
-     **/
-    function inComponentWillReceiveProps() {
-      var scope = context.getScope();
-      while (scope) {
-        if (
-          scope.block && scope.block.parent &&
-          scope.block.parent.key && scope.block.parent.key.name === 'componentWillReceiveProps'
-        ) {
-          return true;
-        }
-        scope = scope.upper;
-      }
-      return false;
-    }
-
     /**
      * Checks if we are using a prop
      * @param {ASTNode} node The AST node being checked.
@@ -107,8 +88,7 @@ module.exports = {
         node.object.type === 'ThisExpression' && node.property.name === 'props'
       );
       var isStatelessFunctionUsage = node.object.name === 'props';
-      var isNextPropsUsage = node.object.name === 'nextProps' && inComponentWillReceiveProps();
-      return isClassUsage || isStatelessFunctionUsage || isNextPropsUsage;
+      return isClassUsage || isStatelessFunctionUsage;
     }
 
     /**
@@ -511,17 +491,12 @@ module.exports = {
      */
     function getPropertyName(node) {
       var isDirectProp = DIRECT_PROPS_REGEX.test(sourceCode.getText(node));
-      var isDirectNextProp = DIRECT_NEXT_PROPS_REGEX.test(sourceCode.getText(node));
       var isInClassComponent = utils.getParentES6Component() || utils.getParentES5Component();
       var isNotInConstructor = !inConstructor(node);
-      var isNotInComponentWillReceiveProps = !inComponentWillReceiveProps();
-      if ((isDirectProp || isDirectNextProp)
-        && isInClassComponent
-        && isNotInConstructor
-        && isNotInComponentWillReceiveProps) {
+      if (isDirectProp && isInClassComponent && isNotInConstructor) {
         return void 0;
       }
-      if (!isDirectProp && !isDirectNextProp) {
+      if (!isDirectProp) {
         node = node.parent;
       }
       var property = node.property;


Test patch:
diff --git a/tests/lib/rules/jsx-sort-props.js b/tests/lib/rules/jsx-sort-props.js
index 1e336641..1664baae 100644
--- a/tests/lib/rules/jsx-sort-props.js
+++ b/tests/lib/rules/jsx-sort-props.js
@@ -41,6 +41,16 @@ var expectedShorthandLastError = {
   message: 'Shorthand props must be listed after all other props',
   type: 'JSXAttribute'
 };
+var expectedReservedFirstError = {
+  message: 'Reserved props must be listed before all other props',
+  type: 'JSXAttribute'
+};
+var expectedEmptyReservedFirstError = {
+  message: 'A customized reserved first list must not be empty'
+};
+var expectedInvalidReservedFirstError = {
+  message: 'A customized reserved first list must only contain a subset of React reserved props. Remove: notReserved'
+};
 var callbacksLastArgs = [{
   callbacksLast: true
 }];
@@ -63,6 +73,22 @@ var noSortAlphabeticallyArgs = [{
 var sortAlphabeticallyArgs = [{
   noSortAlphabetically: false
 }];
+var reservedFirstAsBooleanArgs = [{
+  reservedFirst: true
+}];
+var reservedFirstAsArrayArgs = [{
+  reservedFirst: ['children', 'dangerouslySetInnerHTML', 'key']
+}];
+var reservedFirstWithNoSortAlphabeticallyArgs = [{
+  noSortAlphabetically: true,
+  reservedFirst: true
+}];
+var reservedFirstAsEmptyArrayArgs = [{
+  reservedFirst: []
+}];
+var reservedFirstAsInvalidArrayArgs = [{
+  reservedFirst: ['notReserved']
+}];
 
 ruleTester.run('jsx-sort-props', rule, {
   valid: [
@@ -93,7 +119,38 @@ ruleTester.run('jsx-sort-props', rule, {
     },
     // noSortAlphabetically
     {code: '<App a b />;', options: noSortAlphabeticallyArgs, parserOptions: parserOptions},
-    {code: '<App b a />;', options: noSortAlphabeticallyArgs, parserOptions: parserOptions}
+    {code: '<App b a />;', options: noSortAlphabeticallyArgs, parserOptions: parserOptions},
+    // reservedFirst
+    {
+      code: '<App children={<App />} key={0} ref="r" a b c />',
+      options: reservedFirstAsBooleanArgs,
+      parserOptions: parserOptions
+    },
+    {
+      code: '<App children={<App />} key={0} ref="r" a b c dangerouslySetInnerHTML={{__html: "EPR"}} />',
+      options: reservedFirstAsBooleanArgs,
+      parserOptions: parserOptions
+    },
+    {
+      code: '<App children={<App />} key={0} a ref="r" />',
+      options: reservedFirstAsArrayArgs,
+      parserOptions: parserOptions
+    },
+    {
+      code: '<App children={<App />} key={0} a dangerouslySetInnerHTML={{__html: "EPR"}} ref="r" />',
+      options: reservedFirstAsArrayArgs,
+      parserOptions: parserOptions
+    },
+    {
+      code: '<App ref="r" key={0} children={<App />} b a c />',
+      options: reservedFirstWithNoSortAlphabeticallyArgs,
+      parserOptions: parserOptions
+    },
+    {
+      code: '<div ref="r" dangerouslySetInnerHTML={{__html: "EPR"}} key={0} children={<App />} b a c />',
+      options: reservedFirstWithNoSortAlphabeticallyArgs,
+      parserOptions: parserOptions
+    }
   ],
   invalid: [
     {code: '<App b a />;', errors: [expectedError], parserOptions: parserOptions},
@@ -132,6 +189,55 @@ ruleTester.run('jsx-sort-props', rule, {
       options: shorthandLastArgs,
       parserOptions: parserOptions
     },
-    {code: '<App b a />;', errors: [expectedError], options: sortAlphabeticallyArgs, parserOptions: parserOptions}
+    {code: '<App b a />;', errors: [expectedError], options: sortAlphabeticallyArgs, parserOptions: parserOptions},
+    // reservedFirst
+    {
+      code: '<App a key={1} />',
+      options: reservedFirstAsBooleanArgs,
+      errors: [expectedReservedFirstError],
+      parserOptions: parserOptions
+    },
+    {
+      code: '<div a dangerouslySetInnerHTML={{__html: "EPR"}} />',
+      options: reservedFirstAsBooleanArgs,
+      errors: [expectedReservedFirstError],
+      parserOptions: parserOptions
+    },
+    {
+      code: '<App ref="r" key={2} b />',
+      options: reservedFirstAsBooleanArgs,
+      errors: [expectedError],
+      parserOptions: parserOptions
+    },
+    {
+      code: '<App dangerouslySetInnerHTML={{__html: "EPR"}} key={2} b />',
+      options: reservedFirstAsBooleanArgs,
+      errors: [expectedReservedFirstError],
+      parserOptions: parserOptions
+    },
+    {
+      code: '<App key={3} children={<App />} />',
+      options: reservedFirstAsArrayArgs,
+      errors: [expectedError],
+      parserOptions: parserOptions
+    },
+    {
+      code: '<App z ref="r" />',
+      options: reservedFirstWithNoSortAlphabeticallyArgs,
+      errors: [expectedReservedFirstError],
+      parserOptions: parserOptions
+    },
+    {
+      code: '<App key={4} />',
+      options: reservedFirstAsEmptyArrayArgs,
+      errors: [expectedEmptyReservedFirstError],
+      parserOptions: parserOptions
+    },
+    {
+      code: '<App key={5} />',
+      options: reservedFirstAsInvalidArrayArgs,
+      errors: [expectedInvalidReservedFirstError],
+      parserOptions: parserOptions
+    }
   ]
 });
diff --git a/tests/lib/rules/no-unused-prop-types.js b/tests/lib/rules/no-unused-prop-types.js
index 8611e10d..02a60322 100644
--- a/tests/lib/rules/no-unused-prop-types.js
+++ b/tests/lib/rules/no-unused-prop-types.js
@@ -1461,22 +1461,6 @@ ruleTester.run('no-unused-prop-types', rule, {
         '};'
       ].join('\n'),
       parserOptions: parserOptions
-    }, {
-      code: [
-        'class Hello extends Component {',
-        '  componentWillReceiveProps (nextProps) {',
-        '    if (nextProps.foo) {',
-        '      doSomething(this.props.bar);',
-        '    }',
-        '  }',
-        '}',
-
-        'Hello.propTypes = {',
-        '  foo: PropTypes.bool,',
-        '  bar: PropTypes.bool',
-        '};'
-      ].join('\n'),
-      parserOptions: parserOptions
     }
   ],