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

[MatthewHerbst]

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]

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

[MatthewHerbst]

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]

RESEVED → RESERVED

[MatthewHerbst]

Woops! Thanks

[ljharb]

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?

[MatthewHerbst]

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;
}

[ljharb]

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.

[MatthewHerbst]

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

[ljharb]

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

[MatthewHerbst]

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

[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

[ljharb]

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.

[MatthewHerbst]

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.

[ljharb]

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

[MatthewHerbst]

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

[MatthewHerbst]

(I don't care either way though imho)

[ljharb]

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

[MatthewHerbst]

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?

[ljharb]

I would think that noSortAlphabetically would be a universal setting

[ljharb]

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)

[MatthewHerbst]

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

[ljharb]

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)

[MatthewHerbst]

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.

[MatthewHerbst]

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

[MatthewHerbst]

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

[ljharb]

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

[ljharb]

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

[MatthewHerbst]

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.)

[MatthewHerbst]

@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?

[ljharb]

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.

[MatthewHerbst]

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.

[MatthewHerbst]

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

[ljharb]

Awesome, LGTM!

[MatthewHerbst]

*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]

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

[ljharb]

@MatthewHerbst no, not necessarily.