-
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
core(fr): add timespan support to css-usage #12728
Conversation
/** @param {LH.Crdp.CSS.StyleSheetAddedEvent} sheet */ | ||
this._onStylesheetAdded = sheet => this._stylesheets.push(sheet); | ||
/** @type {LH.Crdp.CSS.RuleUsage[]|undefined} */ | ||
this._ruleUsage = 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 should always be set by the time we use it, which is why I opted to throw an error on undefined
rather than initialize to []
. I'm open to changing it though.
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.
nice!
const coverageResponse = await session.sendCommand('CSS.stopRuleUsageTracking'); | ||
this._ruleUsage = coverageResponse.ruleUsage; | ||
session.off('CSS.styleSheetAdded', this._onStylesheetAdded); | ||
} |
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.
nit: swap order of stop/start functions
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.
Are you saying both start functions should come before both stop functions?
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 necessarily both, just the ones that are logically paired. it helps with readability when you can move top to bottom and the fns build off each other. so startThing before stopThing is preferred
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.
Pretty sure that's the way I had it, but going start/start/stop/stop looks better imo.
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.
the counter-point to this (that I happen to agree with) is that the version here intermixes bespoke helper functions with the official gatherer API functions whereas before the "private" methods were grouped away from "public" :)
I don't think either is a big deal, and perhaps this is just down to how familiar the gatherer API methods are to the reader, but I did like the flow of the original. 🤷
expect(notApplicableAudits.map(audit => audit.id)).toContain('server-response-time'); | ||
|
||
// TODO(FR-COMPAT): Reduce this number by handling the error, making N/A, or removing timespan support. | ||
expect(erroredAudits.length).toMatchInlineSnapshot(`22`); | ||
expect(erroredAudits.length).toMatchInlineSnapshot(`24`); |
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'm gonna wait to land after #12764 to resolve this conflict.
Some slight restructuring was required because the original gatherer only calculated the rule usage at a single point in time (at the snapshot or end of navigation).
Ref #11313