Skip to content

[feat] Sort key before spread #1087

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

Closed
remcohaszing opened this issue Apr 17, 2025 · 14 comments
Closed

[feat] Sort key before spread #1087

remcohaszing opened this issue Apr 17, 2025 · 14 comments
Labels
Good First Issue Good for newcomers Status: Accepted The issue has been accepted Status: Released The issue has been released

Comments

@remcohaszing
Copy link

Describe the problem

There are already rules to sort props.

These are stylistic rules. So they rightfully don’t sort props before or after prop spread syntax. However, when using the JSX automatic runtime, key is a special attribute in the JSX transform. See the Babel repl and TypeScript playground

If the key prop is before any spread props, it is passed as the key argument of the _jsx / _jsxs / _jsxDev function. But if the key prop is after spread props, The compiler uses createElement instead and passes key as a regular prop.

Describe the solution you'd like

It would be useful to have a rule that warns the user if the key prop appears after JSX spread syntax.

Alternatives considered

It could also be incorporated into @eslint-react/no-implicit-key.

Additional context

The key doesn’t have to be first, only before JSX spread syntax. Other rules can take care of sorting.

@remcohaszing remcohaszing changed the title [feat] Sort key first [feat] Sort key before spread Apr 17, 2025
@Rel1cx Rel1cx added Good First Issue Good for newcomers Status: Triaging Issue is still being evaluated. PRs not yet accepted. labels Apr 17, 2025
@Rel1cx
Copy link
Owner

Rel1cx commented Apr 20, 2025

This feature request has been accepted, I will add a jsx-key-before-spread rule.

@Rel1cx Rel1cx added Status: Accepted The issue has been accepted and removed Status: Triaging Issue is still being evaluated. PRs not yet accepted. labels Apr 20, 2025
@Rel1cx Rel1cx closed this as completed May 4, 2025
@JounQin
Copy link

JounQin commented May 5, 2025

Hmm... Why not just port jsx-sort-props directly?

@remcohaszing
Copy link
Author

There are already multiple rules to sort props. These correctly allow users to sort props before or after JSX spread syntax. This is important, because spread syntax changes the order in which the props override each other. The key prop is a special case. Its meaning changes depending on whether or not it appears after JSX spread syntax.

@JounQin
Copy link

JounQin commented May 5, 2025

Then why it must be implemented here instead of an option of perfectionist/sort-jsx-props nor @stylistic/jsx/jsx-sort-props?

@Rel1cx
Copy link
Owner

Rel1cx commented May 5, 2025

Then why it must be implemented here instead of an option of perfectionist/sort-jsx-props nor @stylistic/jsx/jsx-sort-props?

Because this is the only way to correctly set the severity of the two types of problems mentioned above under the ESLint plugin system, which allows setting jsx-key-before-spread to error, jsx-sort-props to warn, or setting jsx-key-before-spread to warn and override jsx-sort-props to info in the editor (via "eslint.rules.customizations"). And be able to correctly disable each of them in different contexts via eslint-disable comments.

@JounQin
Copy link

JounQin commented May 5, 2025

Because this is the only way to correctly set the severity of the two types of problems mentioned above under the ESLint plugin system

Why it/they should be considered two types of problems at the first place? There are many rules have different options, and different options share same severity, we don't provide different rules for each of them only for different severities?

@Rel1cx
Copy link
Owner

Rel1cx commented May 5, 2025

Because this is the only way to correctly set the severity of the two types of problems mentioned above under the ESLint plugin system

Why it/they should be considered two types of problems at the first place? There are many rules have different options, and different options share same severity, we don't provide different rules for each of them only for different severities?

This is exactly the bad practice, caused by a lack of understanding of the linter tool.

@JounQin
Copy link

JounQin commented May 5, 2025

This is exactly the bad practice, caused by a lack of understanding of the linter tool.

Hah? The current implementation is exactly the bad practice without cooperations with other plugins IMO. But well, you're the boss here.

@Rel1cx
Copy link
Owner

Rel1cx commented May 5, 2025

This is exactly the bad practice, caused by a lack of understanding of the linter tool.

Hah? The current implementation is exactly the bad practice without cooperations with other plugins IMO. But well, you're the boss here.

The project was created with the philosophy of "Rules Over Options", and many of the achievements made so far have benefited from this choice.

@Rel1cx
Copy link
Owner

Rel1cx commented May 5, 2025

This is exactly the bad practice, caused by a lack of understanding of the linter tool.

Hah? The current implementation is exactly the bad practice without cooperations with other plugins IMO. But well, you're the boss here.

The current implementation is exactly the bad practice without cooperations with other plugins IMO.

Each rule should have a single purpose. Make multiple rules work together to achieve more complex behaviors instead of adding endless options to a single rule. IMO, This is how plugins should cooperate.

@Rel1cx Rel1cx added the Status: Released The issue has been released label May 5, 2025
@JounQin
Copy link

JounQin commented May 5, 2025

The project was created with the philosophy of "Rules Over Options", and many of the achievements made so far have benefited from this choice.

That's exactly what I mean "you're the boss here", you can have your own philosophy, that's totally fine, although anyone else like me might disagree.

@merrywhether
Copy link

Each rule should have a single purpose. Make multiple rules work together to achieve more complex behaviors instead of adding endless options to a single rule.

AKA "composition over configuration" 💯

@JounQin
Copy link

JounQin commented May 5, 2025

AKA "composition over configuration" 💯

It's definitely not, every single rule needs to be enabled specifically. 😅

@Rel1cx
Copy link
Owner

Rel1cx commented May 5, 2025

In most scenarios this is a benefit rather than a drawback. When a single rule does too much, the rule itself becomes a "plugin", and its messages and options act as the actual "rule". However, messages and options are not first-class citizens under the ESLint plugin system and config system, but rules are.
A typical example is solid/reactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good for newcomers Status: Accepted The issue has been accepted Status: Released The issue has been released
Projects
None yet
Development

No branches or pull requests

4 participants