Add a `reservedFirst` option to the `jsx-sort-props` rule #1134

MatthewHerbst · 2017-03-31T00:04:25Z

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.

ljharb

I'm pretty skeptical about calling these "reserved" props but still allowing the list to be customized - also, "style" isn't "reserved" by react, nor is "htmlFor" (they're both just properties of HTML elements), so neither of these should be in the default list.

ljharb · 2017-03-31T00:12:32Z

docs/rules/jsx-sort-props.md

+
+#### `reservedList`
+
+Use this to optionally override the default list of reserved props.


i think the defaults should go here, not on line 86

I'm open to that; I was hoping to let people know what the rule is doing without making them read all the options.

ljharb · 2017-03-31T00:13:04Z

lib/rules/jsx-sort-props.js

@@ -14,6 +14,19 @@ function isCallbackPropName(name) {
  return /^on[A-Z]/.test(name);
 }

+var RESEVED_PROPS_LIST = [


RESEVED → RESERVED

Woops! Thanks

ljharb · 2017-03-31T00:15:45Z

lib/rules/jsx-sort-props.js

+  'style'
+];
+
+function isReservedPropName(name, list) {


this is just "is in list" if it takes the list argument; it'd probably be better to define this function so that reservedList is available via closure, and this function would take only the name?

I was playing with making it a "default" option if list wasn't specified; would you prefer that?

Regarding your general comment, I think it's important to allow the user to customize this list since some people may only want certain "reserved" props first. For example, maybe we want key first but all the rest to be alphabetized. I'm open to not allowing that (or, maybe better, to allowing overriding the list with only a subset of the reserved props), but I think without this option this option won't be as useful.

function isReservedProp(name, list) { var listToUse = list || RESERVED_PROPS_LIST; return list.indexOf(name) >= 0; }

I can see an argument for allowing a custom sorting of the reserved list - but by calling it "reserved", that means it's not something the user can change. In other words, "foo" isn't considered "reserved" by React, thus it should never be considered "reserved" by anything else.

(Part of making this trivial functionality a function was just to match the style of the isCallbackPropName function.)

ljharb · 2017-03-31T00:16:19Z

lib/rules/jsx-sort-props.js

@@ -44,6 +57,17 @@ module.exports = {
        // Whether alphabetical sorting should be enforced
        noSortAlphabetically: {
          type: 'boolean'
+        },
+        reservedFirst: {


can we ensure that additionalProperties are disallowed on the object form? (not sure if line 73 covers this or not)

I believe that it (line 73) does do that. http://eslint.org/docs/developer-guide/working-with-rules

MatthewHerbst · 2017-03-31T00:36:43Z

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 · 2017-03-31T00:41:12Z

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 · 2017-03-31T03:19:54Z

@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

ljharb · 2017-03-31T04:41:52Z

docs/rules/jsx-sort-props.md

+
+This can be a boolean or an object option.
+
+When `reservedFirst` is defined, React reserved props (`children`, `dangerouslySetInnerHTML`, `key`, `ref`) must be listed before all other props, but still respecting the alphabetical order:


actually now that i think of it; isn't dangerouslySetInnerHTML a special prop only on DOM nodes?

I think you may want separate lists for DOM nodes, and custom components.

Hmmm. Well, all DOM nodes can take the other props, so I think this is ok; I don't actually know what happens if you set dangerouslySetInnerHTML on a composite component. Either way, I think users not putting dangerouslySetInnerHTML on non-DOM components is something left for a different rule check.

It does nothing on a composite component; what I mean is, it's a normal (non-reserved) prop on them.

Is it? I guess my version of "reserved" are things that have special meaning in React, which I feel like this kind of does, even though it mimicks "real" DOM functionality

(I don't care either way though imho)

ljharb · 2017-03-31T04:43:28Z

docs/rules/jsx-sort-props.md

+
+#### `noSortReservedAlphabetically`
+
+When `true`, alphabetical sorting of reserved props is not enforced, but alphabetical sorting of other props still is, unless `noSortAlphabetically` is set to `true` as well.


to confirm: if noSortAlphabetically is set to true, then this option is ignored?

No, right now setting noSortAlphabetically will allow non-reserved to be listed non-alphabetically, but reserved would still need to be alphabetical. Should setting noSortAlphabetically allow reserved to be non-alphabetical as well?

I would think that noSortAlphabetically would be a universal setting

although, if it is, that probably makes this setting a bit more complex.

Can you elaborate on the use case for wanting alphabetical sorting, but not wanting it to apply universally? (or the inverse)

Oh, I was just trying to match the other options, wasn't trying to create new functionality. I'll update so that setting noSortAlphabetically overrides noSortReservedAlphabetically

I'm still questioning the need for different sorting here tho - can you elaborate on the use case for wanting alphabetical sorting, but not wanting it to apply universally? (or the inverse)

I think it was a pre-mature optionality (I think I just made that phrase up). I'll remove it - I guess I just was trying to allow over-customization, but maybe someone else can add that in the future if they need it and can present a compelling reason.

Plus, actually, removing that significantly simplifies the option, since now it can be <boolean>|<array> rather than <boolean>|{...}

Updated; it's much less complex now, good catch calling me out on that!

ljharb · 2017-03-31T08:16:57Z

docs/rules/jsx-sort-props.md

+
+This can be a boolean or an array option.
+
+When `reservedFirst` is defined, React reserved props (`children`, `dangerouslySetInnerHTML`, `key`, `ref`) must be listed before all other props, but still respecting the alphabetical order:


I still think dangerouslySetInnerHTML should only be considered "reserved" on DOM nodes, even if the schema has a single list.

I also don't see any tests that exercise this prop - please add tests that cover every "reserved prop" :-)

I'll add the tests for them all shortly :)

Regarding dangerouslySetInnerHTML, from playing around with a fiddle, it seems to have no affect at all on composite components - so it seems to be treated as a "normal" prop. I'll add the DOM node check for it.

(Maybe there should be another rule to ensure people don't use dangerouslySetInnerHTML on composite components? Seems like something that could easily cause bugs for people who expect it to work.)

@ljharb tried working on this last night, ran into a small problem: what's the best way to identify if an AST node is a DOM element or not? I saw that void-dom-elements-no-children just creates a map of the ones it cares about, but for this we would care about all of them. Checking if the name starts with an upper-case letter would be a good guess, but would be wrong sometimes. It's not clear to me if node.type or if something in node.attributes can be used for this.

If I do just need to hardcode the list, that would mean we wouldn't catch custom DOM elements - is that ok?

Hmm - React treats all lowercased names as a DOM element, I think; but either way I think node.type will only be a string when it's a DOM element.

React treats all lowercased names as a DOM element

I don't believe that's correct, you can name an imported composite component whatever you want and use that within JSX no problem; there's actually a rule in this repo to discourage that: jsx-pascal-case

(live-edit:) I realized that this rule must have the ability to determine user defined components! I'll utilize the method found in that rule.

~~Here's how React Test Utils does it (it comes down to check ReactElement.isValidElement). I'll work on it tomorrow and see what we have available on node.~~

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 · 2017-04-05T02:17:55Z

@ljharb I've fully updated the changes based on what we've discussed, let me know your thoughts, thanks!

ljharb

Awesome, LGTM!

MatthewHerbst · 2017-04-18T03:52:38Z

*bump @lencioni @yannickcr @EvNaverniouk if any of you have some spare moments, looking for some love <3 thanks!

EvHaus

Makes me cringe a little not seeing props sorted alphabetically consistently. 😄 Otherwise the code change LGTM.

MatthewHerbst · 2017-04-28T01:09:31Z

@ljharb do all 4 reviewers on the PR need to approve it?

ljharb · 2017-04-28T03:46:21Z

@MatthewHerbst no, not necessarily.

ljharb requested changes Mar 31, 2017

View reviewed changes

ljharb reviewed Mar 31, 2017

View reviewed changes

ljharb requested changes Mar 31, 2017

View reviewed changes

ljharb approved these changes Apr 5, 2017

View reviewed changes

ljharb added accepted enhancement labels Apr 5, 2017

ljharb requested review from yannickcr, lencioni and EvHaus April 5, 2017 03:16

EvHaus approved these changes Apr 18, 2017

View reviewed changes

ljharb merged commit dfe190b into jsx-eslint:master Apr 28, 2017

MatthewHerbst deleted the jsx-sort-props/new-rule-option--reservedFirst branch April 28, 2017 04:32

renovate bot mentioned this pull request Feb 2, 2018

Update dependency eslint-plugin-react to v7 JimmyLv/Haiku#34

Open

1 task

This was referenced Oct 25, 2018

Update dependency eslint-plugin-react to v7 efcsydney/efcsydney-roster#502

Merged

Update dependency eslint-plugin-react to v7 CaptainGlac1er/StudentHomeworkProject#38

Open

renovate bot mentioned this pull request Nov 7, 2018

chore(deps): update dependency eslint-plugin-react to v7 vgmtv/rsvg#12

Merged

renovate bot mentioned this pull request Nov 23, 2018

chore(deps): update dependency eslint-plugin-react to v7 JLHwung/tianzige#31

Open

1 task

renovate bot mentioned this pull request Dec 5, 2018

Update dependency eslint-plugin-react to v7 Automattic/jetpack#10827

Closed

1 task

mesosphere-web mentioned this pull request Dec 19, 2018

chore(deps): update dependency eslint-plugin-react to v7 mesosphere/reactjs-components#475

Closed

1 task

ljharb mentioned this pull request Feb 2, 2022

Add ability to ignore certain props from jsx-sort-props #605

Closed


		#### `reservedList`

		Use this to optionally override the default list of reserved props.


		This can be a boolean or an object option.

		When `reservedFirst` is defined, React reserved props (`children`, `dangerouslySetInnerHTML`, `key`, `ref`) must be listed before all other props, but still respecting the alphabetical order:


		#### `noSortReservedAlphabetically`

		When `true`, alphabetical sorting of reserved props is not enforced, but alphabetical sorting of other props still is, unless `noSortAlphabetically` is set to `true` as well.


		This can be a boolean or an array option.

		When `reservedFirst` is defined, React reserved props (`children`, `dangerouslySetInnerHTML`, `key`, `ref`) must be listed before all other props, but still respecting the alphabetical order:

Add a reservedFirst option to the jsx-sort-props rule #1134

Add a reservedFirst option to the jsx-sort-props rule #1134

Conversation

MatthewHerbst commented Mar 31, 2017 • edited

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewHerbst Mar 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewHerbst commented Mar 31, 2017

ljharb commented Mar 31, 2017

MatthewHerbst commented Mar 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewHerbst Mar 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewHerbst Apr 2, 2017 • edited

Choose a reason for hiding this comment

MatthewHerbst commented Apr 5, 2017

ljharb left a comment

Choose a reason for hiding this comment

MatthewHerbst commented Apr 18, 2017

EvHaus left a comment

Choose a reason for hiding this comment

MatthewHerbst commented Apr 28, 2017

ljharb commented Apr 28, 2017

Add a `reservedFirst` option to the `jsx-sort-props` rule #1134

Add a `reservedFirst` option to the `jsx-sort-props` rule #1134

MatthewHerbst commented Mar 31, 2017 •

edited

MatthewHerbst Mar 31, 2017 •

edited

MatthewHerbst Mar 31, 2017 •

edited

MatthewHerbst Apr 2, 2017 •

edited