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 jsx-sort-props ignoreTags option #2979

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fracz
Copy link

@fracz fracz commented Apr 30, 2021

This PR adds an ability to set ignored tags when checking the jsx-sort-props rule.

Generally, we like the idea of sorting props but in some cases it does not make sense. For example, when we use the Can component from React CASL:

<Can I="create" a="post">

The rule forces us to sort props.

<Can a="post" I="create">

We'd love to have an option to ignore some tags - which I propose here.

ignoreTags: ["Can"]

Tests and docs provided. The tag name detection code inside the rule has been "borrowed" from jsx-props-no-spreading.

@fracz fracz force-pushed the jsx-sort-props-ignore-tags branch 2 times, most recently from e4c503e to fa43041 Compare April 30, 2021 20:48
@fracz fracz force-pushed the jsx-sort-props-ignore-tags branch from fa43041 to 36b583f Compare April 30, 2021 20:50
@ljharb
Copy link
Member

ljharb commented Apr 30, 2021

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

Choose a reason for hiding this comment

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

these aren't tag names, they're custom component names, so the terminology needs tweaking

Copy link
Author

Choose a reason for hiding this comment

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

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.

Comment on lines +105 to +108
With `ignoreTags: ["Can"]`, the following will **not** warn:

```jsx
<Can i="create" a="post" />
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

this would need to ensure the items are unique, and are valid custom component names

Copy link
Author

Choose a reason for hiding this comment

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

Agree as for the uniqueness. Not sure about validity of component names - see my comment above.

@fracz
Copy link
Author

fracz commented May 1, 2021

The ordering of differently-named props is definitely not supposed to matter

It does not matter - component works regardless of the order. But in some cases - it is better to read like "an english sentence".

@fracz fracz requested a review from ljharb May 1, 2021 11:41
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

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.

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.

2 participants