-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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(tsc): add type checking to most byte efficiency audits #5072
Conversation
*/ | ||
static mapSheetToResult(stylesheetInfo, pageUrl) { | ||
if (stylesheetInfo.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.
it appears this is a holdover from old code. It doesn't appear to be set anywhere
@@ -161,6 +175,8 @@ class UnusedBytes extends Audit { | |||
wastedMs, | |||
wastedBytes, | |||
}; | |||
|
|||
// @ts-ignore - TODO(bckenny): unify details types. items shouldn't be an indexed 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.
we need a unified theory of details item types rather than some with {[x: string]: DetailsItem}
and some not (if it doesn't have a generic index you can't assign to a type that does). I'd rather punt here than figure that out, though, since we're just going to have to figure it out all over again when we switch to our new details types
@@ -116,6 +120,7 @@ class OffscreenImages extends ByteEfficiencyAudit { | |||
// TODO(phulce): move this to always use lantern | |||
const settings = context.settings; | |||
return artifacts.requestFirstCPUIdle({trace, devtoolsLog, settings}).then(firstInteractive => { | |||
// @ts-ignore - see above TODO. |
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 now this is actually an LH.Artifacts.Metric
, so firstInteractive.timestamp
does exist. When it switches to lantern (see above), it's going to have to switch off of timestamp
anyways, so didn't seem worth fixing
@@ -70,6 +81,7 @@ class RenderBlockingResources extends Audit { | |||
|
|||
const metricSettings = {throttlingMethod: 'simulate'}; | |||
const metricComputationData = {trace, devtoolsLog, simulator, settings: metricSettings}; | |||
// @ts-ignore - TODO(bckenny): allow optional `throttling` settings |
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.
@patrickhulce maybe we want a new type for settings required for metrics rather than the full config settings
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.
Hm, yeah it's bit odd because we're explicitly passing in the simulator to use here. The throttling settings would be required otherwise. We could define the two options in separate types for the metrics?
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 could define the two options in separate types for the metrics?
ah, I see. I think I didn't understand that when you added that in a PR and I rebased on top of that. I think that's exactly what we should do
@@ -91,6 +103,7 @@ class RenderBlockingResources extends Audit { | |||
node.traverse(node => deferredNodeIds.add(node.id)); | |||
|
|||
// "wastedMs" is the download time of the network request, responseReceived - requestSent | |||
// @ts-ignore - TODO(phulce): nodeTiming.startTime/endTime shouldn't be optional by this point? |
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.
nodeTimings
is going to need something to differentiate between the in progress object with properties still being defined (and therefore optional) vs what's exposed externally where presumably most (all?) properties are defined. I added @patrickhulce here but I can also take it eventually :)
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.
heh, didn't see this but yeah bingo
*/ | ||
static async computeWastedCSSBytes(artifacts, context) { | ||
const wastedBytesByUrl = new Map(); | ||
try { | ||
// TODO(phulce): pull this out into computed artifact | ||
const results = await UnusedCSS.audit(artifacts, context); | ||
// @ts-ignore - TODO(bckenny): details types. |
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 could have a more specific type returned from UnusedCSS.audit
to avoid the any
type, but it unfortunately passes through audit
in the super class, making the return type more general as well. We could figure this out with details types or the above TODO about moving to a computed artifact would fix it as well.
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.
yeah let's just do the right computed artifact fix later 👍
@@ -91,6 +103,7 @@ class RenderBlockingResources extends Audit { | |||
node.traverse(node => deferredNodeIds.add(node.id)); | |||
|
|||
// "wastedMs" is the download time of the network request, responseReceived - requestSent | |||
// @ts-ignore - TODO(phulce): nodeTiming.startTime/endTime shouldn't be optional by this point? |
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.
yeah we'll probably need to do an IntermediateNodeTiming
vs. NodeTiming
split, I can do that in followup 👍
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 not a huge fan of the readability of some of the new ts-inferrable approaches, but they weren't winners to begin with and should be so much easier to refactor now, so we can easily improve :D
how hard is that NetworkNode/CPUNode union inference business?
let results = []; | ||
networkRecords.forEach(record => { | ||
// exclude data URIs since their size is reflected in other resources | ||
// exclude unfinished requests since they won't have transfer size information | ||
if (record.scheme === 'data' || !record.finished) return; | ||
if (record.parsedURL.scheme === 'data' || !record.finished) return; |
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.
😆
} | ||
|
||
export interface HeadingsResult { |
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.
so looks like this was just nonsense before? results: number
:)
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.
so looks like this was just nonsense before
ha, yeah :)
which ones are you not feeling? I don't want us to have to bend too far out of our way for the compiler. The ugly casts? Definitely agree that we'll have a lot of room for improvement :) It'll also be easier when writing new code if it's just type checked by default.
I think it should be relatively easy. It'll be more a question of where type definitions will live. Basically (I think) instead of saying "this is some subclass of |
oh, and we have to make I think that should basically be 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.
which ones are you not feeling? I don't want us to have to bend too far out of our way for the compiler. The ugly casts?
mostly the casts for networknode and the rework for render-blocking-resources, += bytes, -= bytes was super clear and now I had to scratch my head a bit. then again I wrote the orig so maybe it was already a head scratcher 😆
LGTM
uses-optimized-images.js
anduses-webp-images.js
only get the surface treatment for now as I noticed I was lazy typing theOptimizedImages
in the error case. Fixing that will make their changes really easy.The only annoying change here is having to explicitly cast
Node
toNetworkNode
when wanting to access its network record. The cast is somewhat reasonable, but since they already have a readonlytype
property to differentiate, we could make them a discriminated union in the future. That way the compiler knows a node could only be a CPU or network node (+ whatever the future brings), and checkingnode.type === 'network'
would establish to it that the following code was dealing with aNetworkNode
.