core: adjustments to stack-pack comments and types #8169
Conversation
| @@ -121,6 +121,7 @@ function minifyScript(filePath) { | |||
| shouldPrintComment: () => false, // Don't include @license or @preserve comments either | |||
| plugins: [ | |||
| 'syntax-object-rest-spread', | |||
| 'syntax-async-generators', | |||
There was a problem hiding this comment.
this change was needed for supporting for await...of in our client minifying pipeline. It feels like we shouldn't need the plugin now that it's stage 4 and shipping everywhere, but I don't care enough to look into babel more :)
note that this isn't transpiling anything (we only minify), it's just so the babel parser understands the syntax and doesn't throw when seeing this.
(no idea if the brfs change is needed, it's just that 10 is the latest version, so might as well)
| iconDataURL: matchedPack.iconDataURL, | ||
| descriptions: matchedPack.descriptions, | ||
| }); | ||
| for (const pageStack of pageStacks) { |
There was a problem hiding this comment.
this is all just whitespace changes due to removing the if (artifacts.Stacks) { check
| const matchedPack = stackPacks.find(pack => pack.id === stackPackToIncl.packId); | ||
| if (!matchedPack) { | ||
| log.warn('StackPacks', | ||
| `'${stackPackToIncl.packId}' stack pack was matched but is not found in stack-packs lib`); |
There was a problem hiding this comment.
except for warning here :)
| * @property {string} icon Essentially an id, useful if no npm name is detected. | ||
| * @property {string} url | ||
| * @property {string|null} npm npm module name, if applicable to library. | ||
| * @property {function(Window): false|{version: string|null} | Promise<false|{version: string|null}>} test Returns false if library is not present, otherwise returns an object that contains the library version (set to null if the version is not detected). |
There was a problem hiding this comment.
this is kinda a confusing type to read :) could we do something like
/** @typedef {false|{version:string|null}} JSLibraryDetectorTestResult */
/** ...
@property {function(Window): (JSLibraryDetectorTestResult|Promise<JSLibraryDetectorTestResult>)}| const passData = await GatherRunner.afterPass(passContext, gathererResults); | ||
|
|
||
| if (isFirstPass) { | ||
| baseArtifacts.Stacks = await stacksGatherer(passContext); |
There was a problem hiding this comment.
any reason for moving this after after pass?
There was a problem hiding this comment.
any reason for moving this after after pass?
so we aren't tracing while it runs. The web manifest one can move out of during the trace too, but it has to be run before gatherer's afterPass, so we'd need a refactor on GatherRunner.afterPass
There was a problem hiding this comment.
oh right, maybe add a comment to this effect? it's hard to remember all the steps that happen inside those when glancing at this section :)
| }); | ||
| } | ||
| } catch (e) {} | ||
| } | ||
|
|
||
| return libraries; |
There was a problem hiding this comment.
does this mean we always returned an empty array before?
seems important enough to warrant a smoketest and separate fix if so
There was a problem hiding this comment.
does this mean we always returned an empty array before?
when it was `JSLibraries? Yes.
seems important enough to warrant a smoketest and separate fix if so
an artifact smoketest would be excellent (we have an audit smoke test with the js-libraries audit in dbw picking up jquery), but not sure what you mean about fix. This still returns an empty array when nothing is found?
There was a problem hiding this comment.
I'm confused given your two answers as they seem mutually exclusive :)
Given that we're not Promise.all and .map'ing I'm surprised that the previous code was working at all, so I asked "does this mean we always returned an empty array before?" to which you say "Yes"
Yet our smoketest for this was still passing and you believe there's no fix necessary, so it seems like it wasn't always returning an empty array.
I guess the answer is that because our evaluateAsync Promise wrapping logic in driver waits for all microtasks to complete, it's sort of coincidence that we were still getting all of the synchronously computed ones by .push'ing to the array that was returned after the fact...
Crazy times 😆
There was a problem hiding this comment.
ohhh, I understand what you were asking. I thought you meant in the case where there were no libraries detected did we always return an array, even if empty :)
Yes, it was buggy but accidentally working except for the workbox test promise, which resolved enough microtasks later (+ whatever else SW infrastructure was doing) that evaluateAsync had already returned the array.
types/lhr.d.ts
Outdated
| iconDataURL: string; | ||
| /** An object containing the descriptions of the audits, keyed by the audits' `id` identifier. */ | ||
| /** A set of descriptions for some of Lighthouse's audits, keyed by the audits' `id`. */ |
There was a problem hiding this comment.
| /** A set of descriptions for some of Lighthouse's audits, keyed by the audits' `id`. */ | |
| /** A set of descriptions for some of Lighthouse's audits, keyed by the audit `id`. */ |
changes:
d.tsfilesartifacts.Stacksalways defined (just init with a[], get to remove the null checks). makes sense cause it's an array (so no results can just be an empty array) and it's a base artifact, so really should always be definedlib/gatherer-stacks.jstolib/stack-collector.jsafter discussing names with @paulirish. Happy to bikeshed more tho :)js-library-detectoroutput (instack-collector.js), and settled on normalizing its various combinations ofundefinedandnullfor missing output to just usingundefinedonce it becomes an artifact.js-library-detectornoticed our incorrect asyncObject.entries().forEach(), which gives us our firstfor await...ofin honor of moving to node 10 :)TODO (tomorrow):