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

Refactor selector-max-specificity to leverage upstream helper #7689

Conversation

romainmenke
Copy link
Member

@romainmenke romainmenke commented May 11, 2024

Which issue, if any, is this issue related to?

None

Is there anything in the PR that needs further explanation?

While working on selector-max-specificity for the end position fix I noticed that all of the complexity in the rule is for the ignoreSelectors secondary option.

Because this option is present it is needed to manually walk the selector and re-implement large sections of @csstools/selector-specificity so that ignoring selectors works as expected.

I've added an option upstream to customize the specificity calculations.
See : https://github.com/csstools/postcss-plugins/blob/main/packages/selector-specificity/docs/selector-specificity.selectorspecificity.md

Because this is integrated into @csstools/selector-specificity we can offload a lot more complexity to that package.

A good example was all of the logic around :nth-child(n of .foo).
All this logic already existed upstream but couldn't be used before.


A side effect of this change is that this rule is also a bit faster.

Without secondary options.

Main:

node scripts/benchmark-rule.mjs selector-max-specificity '10,10,10'
Warnings: 0
Mean: 370.16875366666665 ms
Deviation: 16.015808005438103 ms

This change:

node scripts/benchmark-rule.mjs selector-max-specificity '10,10,10'
Warnings: 0
Mean: 301.923139 ms
Deviation: 13.039688509741097 ms

With secondary options.

Main:

node scripts/benchmark-rule.mjs selector-max-specificity '["10,10,10", { "ignoreSelectors": "foo" }]'
Warnings: 0
Mean: 373.55708683333336 ms
Deviation: 17.037700655785976 ms

This change:

node scripts/benchmark-rule.mjs selector-max-specificity '["10,10,10", { "ignoreSelectors": "foo" }]'
Warnings: 0
Mean: 317.46817016666665 ms
Deviation: 19.0016953176133 ms

Copy link

changeset-bot bot commented May 11, 2024

⚠️ No Changeset found

Latest commit: 1aaa7ff

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@Mouvedia
Copy link
Member

A side effect of this change is that this rule is also a bit faster.

Be bold and say that's what you intended all along :)

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@romainmenke Thanks for the pull request. This is a great improvement! Overall LGTM, but I've left a few comments, so I'd appreciate if you would check them out.

lib/rules/selector-max-specificity/index.mjs Show resolved Hide resolved
lib/rules/selector-max-specificity/index.mjs Outdated Show resolved Hide resolved
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM 👍🏼

@romainmenke
Copy link
Member Author

Thank you for the reviews 🙇

@romainmenke romainmenke merged commit c72cdc1 into main May 13, 2024
17 checks passed
@romainmenke romainmenke deleted the refactor-selector-max-specificity-to-leverage-upstream-helper--reliable-malayan-tiger-3e7fc7b930 branch May 13, 2024 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants