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

fix #1402 , fix #1216 #1403

Merged
merged 3 commits into from
May 29, 2022
Merged

fix #1402 , fix #1216 #1403

merged 3 commits into from
May 29, 2022

Conversation

romainmenke
Copy link
Contributor

@romainmenke romainmenke commented May 28, 2022

Now correctly de-dupes selector lists.


This minification still feels a bit wonky to me.
Doesn't have sufficient test coverage and it relies on an implementation detail of postcss-selector-parser.

I would personally remove it to prevent bugs.

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2022

Codecov Report

Merging #1403 (4012ea8) into master (4dc2fc5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1403   +/-   ##
=======================================
  Coverage   97.61%   97.61%           
=======================================
  Files         122      122           
  Lines        9992     9993    +1     
=======================================
+ Hits         9754     9755    +1     
  Misses        238      238           
Impacted Files Coverage Δ
packages/postcss-minify-selectors/src/index.js 99.23% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dc2fc5...4012ea8. Read the comment docs.

Copy link
Collaborator

@ludofischer ludofischer left a comment

Choose a reason for hiding this comment

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

Thank you! It's rare that someone is able to submit a working PR to cssnano. I agree that the code is wonky, but if we deactivated a plugin for that reason we would deactivate most of cssnano. Have you ever looked into swc or Parcel CSS? It looks like they've made a lot of progress building a CSS minifier from scratch, but I don't know if they're less buggy than the current JavaScript CSS minifiers.

@ludofischer ludofischer merged commit f6c29fb into cssnano:master May 29, 2022
@romainmenke romainmenke deleted the fix-selector-minification--courageous-crab-e4943e0397 branch May 29, 2022 15:54
@romainmenke
Copy link
Contributor Author

Thank you! It's rare that someone is able to submit a working PR to cssnano

You're welcome :)
Inner workings here are similar to some things in postcss-preset-env.
So felt right at home

I agree that the code is wonky, but if we deactivated a plugin for that reason we would deactivate most of cssnano. Have you ever looked into swc or Parcel CSS

True, that is the tradeoff with a tool that minifies to equivalent but not equal CSS.
I do like the level of control that is exposed and don't (personally) have a need to switch to swc or parcel.


Thank you for reviewing and merging this so quickly 🎉

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.

None yet

3 participants