Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Apr 28, 2017
Merged

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

merged 1 commit into from Apr 28, 2017

Conversation

MatthewHerbst
Copy link
Contributor

@MatthewHerbst MatthewHerbst commented Mar 31, 2017

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.

Copy link
Member

@ljharb ljharb left a comment

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.


#### `reservedList`

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

@ljharb ljharb Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

var RESEVED_PROPS_LIST = [
Copy link
Member

@ljharb ljharb Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RESEVED → RESERVED

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops! Thanks

'style'
];

function isReservedPropName(name, list) {
Copy link
Member

@ljharb ljharb Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ljharb ljharb Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@ljharb ljharb Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MatthewHerbst
Copy link
Contributor Author

MatthewHerbst commented Mar 31, 2017

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
Copy link
Member

ljharb commented Mar 31, 2017

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
Copy link
Contributor Author

MatthewHerbst commented Mar 31, 2017

@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


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:
Copy link
Member

@ljharb ljharb Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ljharb ljharb Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I don't care either way though imho)


#### `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.
Copy link
Member

@ljharb ljharb Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@ljharb ljharb Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think that noSortAlphabetically would be a universal setting

Copy link
Member

@ljharb ljharb Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ljharb ljharb Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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:
Copy link
Member

@ljharb ljharb Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ljharb ljharb Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Apr 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ljharb ljharb Apr 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@MatthewHerbst MatthewHerbst Apr 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

MatthewHerbst commented Apr 5, 2017

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

ljharb
ljharb approved these changes Apr 5, 2017
Copy link
Member

@ljharb ljharb left a comment

Awesome, LGTM!

@MatthewHerbst
Copy link
Contributor Author

MatthewHerbst commented Apr 18, 2017

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

EvHaus
EvHaus approved these changes Apr 18, 2017
Copy link
Collaborator

@EvHaus EvHaus left a comment

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

@MatthewHerbst
Copy link
Contributor Author

MatthewHerbst commented Apr 28, 2017

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

@ljharb
Copy link
Member

ljharb commented Apr 28, 2017

@MatthewHerbst no, not necessarily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants