-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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: add CSS usage tracking #1421
Conversation
Adds basic CSS rule usage tracking gatherer and audit. Moves toward parity with Audits 1.0 as per #909.
"name": "Using bytes efficiently", | ||
"audits": { | ||
"unused-css-rules": { | ||
"expectedValue": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense to me, shouldn't the expectedValue be true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't you write this code? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha :P I'm commenting on why I had to set it to false
for it to become an error when the value is true when successful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
followup: all the expectedValue entries here seem to be useless for non-scored sections, the report just wants true
for success and false
for failure, I can toggle this all day with no change in the report, but the rawValue === didPass
@@ -115,9 +115,7 @@ class Styles extends Gatherer { | |||
Promise.all(contentPromises).then(styleHeaders => { | |||
driver.off('CSS.styleSheetAdded', this._onStyleSheetAdded); | |||
driver.off('CSS.styleSheetRemoved', this._onStyleSheetRemoved); | |||
return driver.sendCommand('CSS.disable') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is the right time to pull out domain information when multiple gatherers use the same domain, but for now just manually removing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe just a new cleanup
pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were bound to run into this eventually. Need to come up with a general solution.
At the very least for this PR, can you comment this out and make a note of why they're commented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@patrickhulce if you run the audits panel on the verge.com, does it come back with 4386 unused styles? IOW, does it reach parity or are you trying to do even better than the audits panel?:) |
@ebidel we do a little better than current audits. It returns 2393 unused :) They might restrict to same-origin currently though since the difference appears to be from https://s.ytimg.com/yts/cssbin/www-embed-player-sprite-mode-vfl0Ytuip.css |
@ebidel @brendankenny FYI tentatively removing WIP. I've attempted to address the 3 concerns above by
|
@@ -129,13 +133,17 @@ class Styles extends Gatherer { | |||
afterPass(options) { | |||
return this.endStylesCollect(options.driver) | |||
.then(stylesheets => { | |||
// Want unique stylesheets. Remove those with the same text content. | |||
// Generally want unique stylesheets. Mark those with the same text content. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure where we actually needed to do this? It looks like the only other usage of Styles
is the no flexbox which I would advocate warning even on multiple shadow DOM uses, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shadow DOM is unique here AFAICT. When testing a page with 100 components that all share the same stylesheet, we'll get 100 violations. That ends up being pretty noisy and something I noticed immediately on testing some sites built in Polymer. We should definitely keep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my question is, what audits are noisy? If you use flexbox in 100 style elements in all 100 of your shadow doms, shouldn't you have 100 failures? I couldn't find where else we use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO no, because the author is writing the <style>
once and creating instances of the custom element. IOW, I don't think it's helpful to know you're failing 100 times. You really just want to know the source of failure, then fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough, added the appropriate filter to the usage function
@@ -0,0 +1,113 @@ | |||
/** | |||
* @license | |||
* Copyright 2016 Google Inc. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2017
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
name: 'unused-css-rules', | ||
description: 'Site does not have more than 10% unused CSS', | ||
helpText: 'Remove unused rules from stylesheets to reduce unnecessary ' + | ||
'bytes consumed by network activity.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a learn more link? Maybe [Learn more](https://developers.google.com/speed/docs/insights/OptimizeCSSDelivery)
for now.
cc @kaycebasques for the FYI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
}; | ||
} | ||
|
||
static indexStylesheetsById(styles) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc styles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
}); | ||
|
||
return {unused: unused, total: rules.length}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it's fun to shorten objects with same key/val names: {unused, total: rules.length};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done (by getting rid of entirely)
@@ -129,13 +133,17 @@ class Styles extends Gatherer { | |||
afterPass(options) { | |||
return this.endStylesCollect(options.driver) | |||
.then(stylesheets => { | |||
// Want unique stylesheets. Remove those with the same text content. | |||
// Generally want unique stylesheets. Mark those with the same text content. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shadow DOM is unique here AFAICT. When testing a page with 100 components that all share the same stylesheet, we'll get 100 violations. That ends up being pretty noisy and something I noticed immediately on testing some sites built in Polymer. We should definitely keep.
@@ -35,6 +35,12 @@ function getCSSPropsInStyleSheet(parseTree) { | |||
const results = []; | |||
|
|||
parseTree.traverseByType('declaration', function(node, index, parent) { | |||
if (parent.type === 'arguments') { | |||
// We don't want to return data URI delcarations of the form |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
declarations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -115,9 +115,7 @@ class Styles extends Gatherer { | |||
Promise.all(contentPromises).then(styleHeaders => { | |||
driver.off('CSS.styleSheetAdded', this._onStyleSheetAdded); | |||
driver.off('CSS.styleSheetRemoved', this._onStyleSheetRemoved); | |||
return driver.sendCommand('CSS.disable') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were bound to run into this eventually. Need to come up with a general solution.
At the very least for this PR, can you comment this out and make a note of why they're commented?
@@ -0,0 +1,187 @@ | |||
/** | |||
* Copyright 2016 Google Inc. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2017
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
const Audit = require('../../audits/unused-css-rules.js'); | ||
const assert = require('assert'); | ||
|
||
/* global describe, it, beforeEach */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* eslint-env mocha */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
function generate(content, length) { | ||
const arr = []; | ||
for (let i=0; i<length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing: for (let i = 0; i < length; i++) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Think the CI is failing b/c of some node4/5 es6 features not being supported. |
LGTM from me. |
@brendankenny any more thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good.
JSDoc comments are optional, as I was mostly using that as a way to walk through the PR and we don't really use them right now :)
const styles = artifacts.Styles; | ||
const usage = artifacts.CSSUsage; | ||
|
||
if (typeof styles === 'undefined' || typeof usage === 'undefined') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this first block is no longer necessary because runner now checks that all required artifacts are defined before running an audit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woohoo! done
} | ||
|
||
/** | ||
* @param {!Array.<Object>} styles The output of the Styles gatherer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could do @param {!Array<{header: {styleSheetId: string}}>}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
/** | ||
* @param {!Array.<Object>} styles The output of the Styles gatherer. | ||
* @return {!Object} A map of styleSheetId to stylesheet information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@return {{used: !Array, unused: !Array}}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a subset of the type of each value under the keys of the object. It's basically a map of stylesheet id -> stylesheet + that type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, doing records like that is structural typing, so you really only need to declare what you're using. If we had a compiler looking at this, though, you'd have to cast between calling these functions, so I guess if you really wanted to do this you'd need to use the union of my suggestions for each of these, something like {{used: !Array, unused: !Array, content: string, header: {sourceURL: string, styleSheetId: string}}}
, but, again, may not be worth it if we're not doing typedefs :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, yeah, didn't understand what you meant here. I had this wrong. As you point out, it would be @return Object<{used: !Array, unused: !Array}>
, if you wanted to change it :)
|
||
/** | ||
* Counts the number of unused rules and adds count information to sheets. | ||
* @param {!Array.<RuleUsage>} rules The output of the CSSUsage gatherer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is RuleUsage
defined anywhere? Can also do {!Array<{styleSheetId: string, used: boolean}>}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in the devtools protocol. I saw other usage of devtools protocol classes, but maybe we just had our own corresponding classes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will change
* @return {!Object} A map of styleSheetId to stylesheet information. | ||
*/ | ||
static indexStylesheetsById(styles) { | ||
return styles.reduce((indexed, stylesheet) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like devtools does styleSheet
(e.g. styleSheetId
) but you mostly do stylesheet
. We should settle on one, preferably
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change to theirs
.then(_ => options.driver.sendCommand('CSS.startRuleUsageTracking')); | ||
} | ||
|
||
gatherRuleUsage(driver) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
total nit: any reason not to inline this into afterPass
as the above is inlined into beforePass
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just that I thought it was going to be more complicated when I first started writing the file :) done
return Array.from(map.values()); | ||
return stylesheets.map(stylesheet => { | ||
const idInMap = map.get(stylesheet.content).header.styleSheetId; | ||
stylesheet.isDuplicate = idInMap !== stylesheet.header.styleSheetId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be unused right now?
@ebidel you ok with this no longer removing duplicates per conversation above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really think we should keep it. https://github.com/GoogleChrome/lighthouse/pull/1421/files#r95644056
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
อย่างไรดีวิธีการที่รวดเร็ว
*/ | ||
'use strict'; | ||
|
||
const Audit = require('../../audits/unused-css-rules.js'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will you call this unusedCssAudit
or something? Much easier to follow when reading in the future :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
}); | ||
|
||
describe('#audit', () => { | ||
it('fails when no input present', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can remove this as runner will always call with artifacts now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, yeah I thought we had gotten rid of these but the test file I copied still had it in there so I thought I was misremembering haha
}); | ||
}); | ||
|
||
describe('#audit', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just make these top level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason behind not nesting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it doesn't really matter, just that the whole test is the #audit test :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ทำต่อไปครับ
static indexStylesheetsById(styles) { | ||
return styles.reduce((indexed, stylesheet) => { | ||
indexed[stylesheet.header.styleSheetId] = Object.assign({ | ||
used: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it worth having these as arrays or should you just do counts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's worth removing. Achieving parity w/audits 1.0 meant being able to list each rule selector in devtools (which is where this originally came from), so if it's needed there it will be easier? If we think we're memory constrained I'll remove though.
PTAL :) |
Just thought of a last thing. We should really make a checklist of things new gatherers/audits need... Will you add a smokehouse expectation for this? Since this isn't explicitly DBW, adding it to offline-expectations.js may make the most sense, though the DBW test page will do a better job putting the gatherer and audit through their paces. |
@@ -37,6 +37,10 @@ function filterStylesheetsByUsage(stylesheets, propName, propVal) { | |||
const deepClone = stylesheets.map(sheet => Object.assign({}, sheet)); | |||
|
|||
return deepClone.filter(s => { | |||
if (s.isDuplicate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks 👍
@@ -124,10 +124,12 @@ class UnusedCSSRules extends Audit { | |||
|
|||
const indexedSheets = UnusedCSSRules.indexStylesheetsById(styles); | |||
const unused = UnusedCSSRules.countUnusedRules(usage, indexedSheets); | |||
const unusedRatio = (unused / usage.length) || 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay smoke tests :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Adds basic CSS rule usage tracking gatherer and audit. Moves toward parity with Audits 1.0 as per GoogleChrome#909.
This is causing runtime errors for me. Can someone have a look?
|
I am having the same runtime errors as @XhmikosR. commit 97d48d3 Performed npm installs around 1-16-2017 @ 3pm.
|
@patrickhulce: I figured out that must be the reason since Travis is passing. Thanks for the info! |
R: all
Adds basic CSS rule usage tracking gatherer and audit. Moves toward parity with Audits 1.0 as per #909.
Has a couple of issues that need to be figured out.
It's using the existing URLLIST formatter that the
no-old-flexbox
audit uses but it'd be nice to group these by stylesheet ID, emphasize selector over URL, etc.The Verge gives 4386 unused rules and dumps in a gigantic list, also unable to detect soon-to-be-used rules that still make sense to load with the page and failing for 1 unused rule (which could just be media-query or something) seems harsh and at risk of being ignored as flaky.
I was unable to get the urls of inline styles that were added via script. I wouldn't be concerned but since this is a core behavior of the webpack style-loader it seems reasonable that we'll run into it fairly often, and it'd be nice to have the url of the script that added them at least (not sure how this would play into source maps or if we can leverage)