-
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(tsc): add type checking to computed artifacts #5051
Conversation
typings/artifacts.d.ts
Outdated
}[] | ||
} | ||
|
||
// TODO(bckenny): all but navigationStart could actually be 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.
these should be updated in a follow up, but it had too many places where we're treating them as always defined. Whether we were correct or not (and we'll get to fix some bugs) remains to be seen.
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 I have any blocking comments, but feels too wrong to approve a 1k-line PR on first review 😝
return artifacts.requestMainResource(artifacts.devtoolsLogs[Audit.DEFAULT_PASS]) | ||
const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS]; | ||
|
||
return artifacts.requestMainResource({devtoolsLog, URL: artifacts.URL}) |
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've been thinking that it might make sense to migrate some of these compute artifacts to just accept same LH.Artifacts/Settings signature so all the metrics and whatnot don't have to do this dance. the metrics all share the same 5-line annoying boilerplate to pluck out the settings in the same order.
This sort of goes in the other direction, so WDYT about that as long-term goal?
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, I thing simplifying it is a good idea. This PR I guess was mostly about making everything explicit so it'll be easier to refactor :)
return Promise.resolve(this._cache.get(artifacts)); | ||
async request(requiredArtifacts) { | ||
const computed = this._cache.get(requiredArtifacts); | ||
if (computed) { |
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.
kind of a pain that TS can't process .has
in the same way, that's kind of the point of .has
:)
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.
microsoft/TypeScript#18781 but they don't sound very enthusiastic :)
@@ -23,86 +25,96 @@ class ManifestValues extends ComputedArtifact { | |||
return ['hasManifest', 'hasParseableManifest']; | |||
} | |||
|
|||
/** @typedef {(val: NonNullable<LH.Artifacts.Manifest['value']>) => boolean} validator */ |
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.
lowercase typedef name? 😮
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.
fixed
@@ -163,7 +172,7 @@ class PageDependencyGraphArtifact extends ComputedArtifact { | |||
let minCandidate = null; | |||
let minDistance = Infinity; | |||
// Find the closest request that finished before this CPU task started | |||
candidates.forEach(candidate => { |
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.
does this really change things type interference wise? or just drive-by perf/style?
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, tsc bug (or limitation). minCadidate
lost its null | NetworkNode
type after the forEach()
and fell back to just null
, and with the if
eliminating falsey values it ended up thinking it was cpuNode.addDependency(never);
const timers = new Map(); | ||
for (const node of cpuNodes) { | ||
for (const evt of node.childEvents) { | ||
if (!evt.args.data) continue; | ||
|
||
const url = evt.args.data.url; |
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.
dataUrl makes me think of a data URI, which other url is this prefix disambiguating?
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.
purely style. I was wondering where url
came from below, then got briefly confused by the various forEach
es. Changed to argsUrl
...wdyt? evtUrl
? Can also revert.
@patrickhulce you can land #5024 and I'll update this |
updated! |
scoped types and rebased :) |
Important first step to refactoring computed artifacts to be just regular requires. Big PR, but almost all type stuff.
Introduces the lie that the computed artifact
request*()
methods are part of theArtifacts
type, which isn't usually true except when callingmyaudit.audit(artifacts, auditContext)
. Follow ups will untangle that.Only real code changes:
computedArtifacts
object realized it also had all the artifacts on it was inMainResource
, whereartifacts.URL
was accessed. Instead,URL
is now passed in as an artifact dependency (as it should be), but that requiresCriticalRequestChains
also needingURL
passed in. This needed a lot of small test tweaks.settings: {}
around.ComputedArtifact.compute_
method now has to be async. It was too annoying dealing with it either being sync (URL
,TraceOfTab
, maybe one more?) or async, so just easier this way. Required a good number of additional async/await scattered around, especially in tests.Pretty much everything else is just adding type annotations or making code a little more explicit for the compiler.