-
-
Notifications
You must be signed in to change notification settings - Fork 320
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
feat: disable css-declaration-sorter in default preset #1065
Conversation
hm, will be great to test it on popular frameworks, like bootstrap/tailwind |
Definitely should test more. But from my (possibly wrong) understanding of compression I am not surprised it does not reduce the size. I guess there's very small number of cases where sorting could improve the size (same declarations with same properties, but rules cannot be merged for some reason). |
Make sense, maybe we just set it to |
On most framework CSS, css-declaration-sorter has a negative impact on gzip size. Sometimes it is also unsafe. Keep in the advanced preset as it has a positive effect on the Tailwind CSS.
1ce4ad4
to
3b40c97
Compare
I've updated this PRI, disabled in css-declaration-sorter in the default preset instead of removing. But I think this exposes a different bug. I find it's actually pretty strange: Without css-declaration-sorter, a{border-top:1px solid;border-color:purple} turns into a{border-color:currentcolor purple purple;border-top:1px solid purple} Might be related to #1044 |
👋 chiming in, it should reduce the gzip size indeed but I can imagine that it has a negative effect on some stylesheets. I did some testing initially when adding the sorter to cssnano but have no concrete numbers. Would be good to look into, I'll also see if I can find time soon for this. Regarding the upgrade path:
I can backport bug fixes to the 5.x branch, is that something you want? |
The current cssnano uses PostCSS 8, so we would need a css-declaration-sorter release with the PostCSS 8 API compatible with Node.js 10. Right now cssnano is using css-declaration-sorter pinned at |
The gzip compression results surprise me, because I cannot understand how sorting makes the results slightly worse (I could understand if there was no change at all).
The only framework where sorting improves the gzipped output is tailwind. I'm also surprised of the drastic improvements that minification makes on tachyons, as I expected tachyons and tailwind to have a similar structure, but apparently that's not the case. Of course most people would not ship the whole of tailwind so in absolute number of kilobytes, the improvements would me smaller for practical situations. In all other cases there is basically no change, it only gets worse by a minuscule amount. |
What do you think disable this for default preset and set it for |
I do wonder what the effects are on actual CSS bundles instead of framework bundles. 🤔 Could get even complexer with Brotli but not sure about that since it is based on the same principles as gzip afaik. |
@Siilwyn another idea - calculate original gzip/brotli size then order them, calculate gzip/brotli size of ordered and return what is best, it may take some time, but it will be safe to use |
It would be interesting to check with custom CSS, but I think these days the framework code is a big part of the total CSS and it's also time consuming to scrape enough CSS that's not already minified. In my measurements, there's no difference with brotli either. One of the easiest methods to check is running this benchmark https://github.com/GoalSmashers/css-minification-benchmark and disable |
Alright, that should be do-able. I remember |
@ludofischer let's puts benchmark here, it is good work, we should not lost it |
Sounds like a nice weekend task for me. Where would you put this logic in
Definitely, thanks for the investigation good to see! I understand getting unminified CSS is more time consuming, thinking of trying your benchmark out on open source websites like PostCSS and Deno. |
Make sense to put it here, because your module sort of declarations only 😄 Also I think |
Thinking of creating a comparison using stylesheets used in real websites. A quick experiment using the benchmark comparsion you linked @ludofischer gives the following:
I'll also look into @alexander-akait's idea, I am afraid of the performance impact on cssnano though since we'd need to run something like gzip-size on it. |
Good research, can you share example (gist/git)? |
Sometimes I think we should convert all postcss plugin into one package, it will improve perf very well... @ludofischer What do you think? |
I think we've talked about it before. I am in favour, but I thought to try to fix some of the remaining bugs, like the |
Regarding to calc - I think we can keep content as is, if parser can't parse content - I do it postcss/postcss-calc#137, maybe we can merge it |
Could not detect any size reduction with css-declaration-sorter on,
it introduces bugs in certain conditions and it is hard to upgrade
without breaking Node.js 10 compatibility.
Fix #1054
In the test below with
css-size
, it looks likecss-declaration-sorter
increases the gzip size from 22.8kB to 22.9kBmaster:
in this PR: