-
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): fix OptimizedImages type; type check dep audits #5129
Conversation
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
/** @typedef {{isSameOrigin: boolean, isBase64DataUri: boolean, requestId: string, url: string, mimeType: string, resourceSize: number}} SimplifiedNetworkRecord */ | ||
results.push({ | ||
failed: false, | ||
...stats, |
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.
🎉
const failedImages = []; | ||
/** @type {Array<{url: string, fromProtocol: boolean, isCrossOrigin: boolean, totalBytes: number, wastedBytes: 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.
since this is used in multiple audits can we share it? i'm okay either way.
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 almost but not quite the same as we use in other audits, enough to be annoying to redefine. url
has to be a string and there's the extra isCrossOrigin
and fromProtocol
properties. I don't think it's worth defining externally for just this and uses-optimized-images
, though, since it just becomes a ByteEfficiencyResult
to the rest of the pipeline
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 can make the failedImages
type be inferred, though
|
||
for (const record of imageRecords) { | ||
try { | ||
const stats = await this.calculateImageStats(driver, record); |
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.
throwing this out there... maybe we should let these race and promise.all them instead. would probably be faster.
but i cant remember if I've seen this gatherer being particularly slow..
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 can definitely be slow, but in the cases where it's slowest (few cores, slow CPU) asking chrome to simultaneously compress all of them at once isn't going to help :)
I think it's pretty readable as is, Promise.all might mess with try/catch clarity
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.
k sg. then yah nvm. we'll keep it running serially.
Yet another browserify syntax error. It's on object spread again, this time in lexical-scope, called by insert-module-globals. It appears it might be an actual bug, though, and not another parser version that needs to be piped through because you can actually use object spread in some files and not others. e.g. not in |
gave up on fixing object spread for now and filed #5152 :) Have to |
make
OptimizedImages
a discriminated union onfailed
or not. Keeps the artifact type simpler and the use of it really simple in these two audits.The only remaining
// @ts-nocheck
audit ismixed-content.js
, which should make refactors a little more easier.