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 shorthand-property-sorting to recommended ESLint rules #1736

Merged
merged 3 commits into from
Oct 25, 2024

Conversation

dddlr
Copy link
Collaborator

@dddlr dddlr commented Oct 25, 2024

What is this change?

Add shorthand-property-sorting to recommended ESLint rules, meaning that it will be turned on by default for all codebases where @compiled/eslint-plugin set up correctly.

Why are we making this change?

After #1700 was merged in, Compiled will now sort shorthand properties to come before longhand properties at buiild-time and runtime. @zesmith2 made a lint rule to enforce that the order of the properties in the source code of Compiled usages matches this ordering. We've resolved all the eslint rule violations in jira and confluence (I think?), so we can just add this as a default recommended eslint rule to remove the need to configure it manually.

Some asks

Some requests from me before I go:

  • If someone could extend shorthand-property-sorting so it handles the different forms of style composition, that would be great 🙏 @pancaspe87 has a link to the internal jira ticket for tracking this. @liamqma gave some examples in AFB-777 create eslint to enforce shorthand sorting #1714 (comment)

  • If someone could quickly verify that the rule does indeed get applied in our products after the Compiled eslint plugin is version bumped, that would be great 🙏


PR checklist

Don't delete me!

I have...

  • Updated or added applicable tests
  • Updated the documentation in website/

Copy link

changeset-bot bot commented Oct 25, 2024

🦋 Changeset detected

Latest commit: 63d5d53

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@compiled/eslint-plugin Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

netlify bot commented Oct 25, 2024

Deploy Preview for compiled-css-in-js canceled.

Name Link
🔨 Latest commit 63d5d53
🔍 Latest deploy log https://app.netlify.com/sites/compiled-css-in-js/deploys/671b272764fec30008615fc8

@dddlr dddlr requested a review from AllySummers October 25, 2024 04:05
zesmith2
zesmith2 previously approved these changes Oct 25, 2024
@AllySummers
Copy link
Collaborator

not sure how you like to handle this in compiled eslint plugin since it's still in 0.x.x (where basically anything goes), but just a note from a (very) opinionated eslint perspective.

this kind of change, if it were following the same semver policy as eslint core (and what many big eslint plugins use) and it wasn't in 0.x.x, it would be considered a breaking change. considering that, it may be appropriate to do at least a minor bump instead of patch?

feel free to ignore if you like to handle this in a different way for this plugin :)

from here: https://github.com/eslint/eslint?tab=readme-ov-file#semantic-versioning-policy

  • Major release (likely to break your lint build)
    • eslint:recommended is updated and may result in new linting errors (e.g., rule additions, most rule option updates).

@AllySummers
Copy link
Collaborator

also i just realised, we should probably update the readme to reflect that this is now in the recommended config?

@dddlr
Copy link
Collaborator Author

dddlr commented Oct 25, 2024

also i just realised, we should probably update the readme to reflect that this is now in the recommended config?

ah you've already added it to the readme :p

@dddlr
Copy link
Collaborator Author

dddlr commented Oct 25, 2024

Screenshot 2024-10-25 at 16 03 58

@dddlr
Copy link
Collaborator Author

dddlr commented Oct 25, 2024

not sure how you like to handle this in compiled eslint plugin since it's still in 0.x.x (where basically anything goes), but just a note from a (very) opinionated eslint perspective.

sure thing, will change to minor

@dddlr dddlr enabled auto-merge (squash) October 25, 2024 05:30
@dddlr dddlr merged commit 6fc00de into master Oct 25, 2024
13 checks passed
@dddlr dddlr deleted the chore/add-shorthand-sorting-to-recommended branch October 25, 2024 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants