new-audit(unminified-css): identifies savings from unminified CSS #4127
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paulirish
reviewed
Jan 4, 2018
| for (let i = 0; i < content.length; i++) { | ||
| const char = content.charAt(i); | ||
| const nextChar = content.charAt(i + 1); | ||
| const chars = char + nextChar; |
There was a problem hiding this comment.
might be worth doing perf testing of these.
whats the runtime cost of this audit on a decent size site?
There was a problem hiding this comment.
test1 https://jsperf.com/charat-master/1 proves winner is
const chars = content.substr(i, 2)
const char = chars.charAt(0)
equals is 100x faster than regex for those two lines too 👍
total runtime on sites with lots of css (i.e. that actually trigger savings) was ~50ms
patrickhulce
force-pushed
the
css_minification
branch
from
ee27847 to
6eefc26
Compare
|
I reverted the commit that messes with styles gatherer and just stuck this in full config for now. My plan is to immediately followup with a PR that brings both unused-css and unminified css into the default config 🎉 |
paulirish
approved these changes
Jan 5, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #3459
don't squash this one please as removing no-old-flexbox is a relatively atomic commit :) I'll manually rebase before merge
I'm honestly not a huge fan of this audit, I'm not sure it would pass our criteria and it was quite difficult to find a site where it was triggered at all as the usual suspects were fine (CNN, The Verge, etc).
A relatively bloated Wordpress theme (http://theme.weartstudio.eu/thewire/) finally got it but only with ~40ms estimated savings, the production minified version of bootstrap only saves you 2KB after gzip which is below our regular threshold.
The overall approach is similar to unminified javascript, count the % of the content that is meaningful which in this case excludes whitespace and comments. It's just that the volume of CSS is usually quite a bit lower than JS on sites. In the process I had to revive the styles gatherer and kill the no-old-flexbox which needed parsed content that made it slow.
realization that needs to be addressed: the styles gatherer is basically useless on its own because every time you turn on/off the CSS domain all the stylesheet ids change, so we might as well just base the audit on network records and leave the old flexbox cleanup for another time