Add jsx-sort-props ignoreTags option #2979
Conversation
fracz
force-pushed
the
jsx-sort-props-ignore-tags
branch
2 times, most recently
from
e4c503e
to
fa43041
Compare
fracz
force-pushed
the
jsx-sort-props-ignore-tags
branch
from
fa43041
to
36b583f
Compare
|
oof, that sounds like a very strange API. The ordering of differently-named props is definitely not supposed to matter, but I see the whimsical (but ruby-like rather than JS-like) approach they're trying for here. |
| @@ -98,6 +98,16 @@ With `reservedFirst: ["key"]`, the following will **not** warn: | |||
| <Hello key={'uuid'} name="John" ref="ref" /> | |||
| ``` | |||
|
|
|||
| ### `ignoreTags` | |||
|
|
|||
| An array of element (tag) names to ignore when checking the properties order. | |||
There was a problem hiding this comment.
these aren't tag names, they're custom component names, so the terminology needs tweaking
There was a problem hiding this comment.
Is it necessary to be a custom component name? The rule is not limited to sort props on the components only. It currently sorts the attributes of "normal" HTML elements, too. If you accept the ignore option, it should be possible to whitelist normal tags, too.
| With `ignoreTags: ["Can"]`, the following will **not** warn: | ||
|
|
||
| ```jsx | ||
| <Can i="create" a="post" /> |
There was a problem hiding this comment.
i think rather than ignoring, it would be better to specify a custom ordering that applies to a specific custom component name. What do you think?
There was a problem hiding this comment.
Well, it makes sense in the case of the CASL's Can component.
I can also think about using this in FontAwesome components (I'd like to have the fixedWitdth after icon because it is better to read then: <FontAwesomeIcon icon="coffee" fixedWidth />.
However, I'm reluctant because it will be less generic (uou don't always know all possible props that can be used with an element). Moreover, this will make the configuration much more complicated.
There was a problem hiding this comment.
This is a very very rare use case; I'm not aware of anything in the entire JS ecosystem besides CASL that has a props API like this. I think it's totally fine to be not generic, since I very much doubt there will ever be a second use case (where the order is sentence-like).
In the case of FontAwesome, just ignoring it wouldn't enforce your preference, it'd just remove enforcement - which is strictly less useful, even if the alternative would require more config.
| @@ -249,6 +249,9 @@ module.exports = { | |||
| }, | |||
| reservedFirst: { | |||
| type: ['array', 'boolean'] | |||
| }, | |||
| ignoreTags: { | |||
| type: 'array' | |||
There was a problem hiding this comment.
this would need to ensure the items are unique, and are valid custom component names
There was a problem hiding this comment.
Agree as for the uniqueness. Not sure about validity of component names - see my comment above.
It does not matter - component works regardless of the order. But in some cases - it is better to read like "an english sentence". |
There was a problem hiding this comment.
See https://github.com/yannickcr/eslint-plugin-react/pull/2979/files#r624281044; ignoring is not very useful, instead, specifying a custom prop ordering for an arbitrary list of custom component names seems better. Given that this CASL thing seems like the only instance of this pattern in existence, I'd prefer something generic that also allows for enforcement, rather than just ignoring.
ljharb
force-pushed
the
master
branch
6 times, most recently
from
59af733
to
865ed16
Compare
ljharb
force-pushed
the
master
branch
4 times, most recently
from
069314a
to
181c68f
Compare
This PR adds an ability to set ignored tags when checking the
jsx-sort-propsrule.Generally, we like the idea of sorting props but in some cases it does not make sense. For example, when we use the
Cancomponent from React CASL:The rule forces us to sort props.
We'd love to have an option to ignore some tags - which I propose here.
Tests and docs provided. The tag name detection code inside the rule has been "borrowed" from jsx-props-no-spreading.