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.
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.
| const coverageResponse = await session.sendCommand('CSS.stopRuleUsageTracking'); | ||
| this._ruleUsage = coverageResponse.ruleUsage; | ||
| session.off('CSS.styleSheetAdded', this._onStylesheetAdded); | ||
| } |
There was a problem hiding this comment.
nit: swap order of stop/start functions
There was a problem hiding this comment.
Are you saying both start functions should come before both stop functions?
There was a problem hiding this comment.
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.
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.
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.
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