core(runner): audits can only use requiredArtifacts #8760
Conversation
patrickhulce
changed the title
| @@ -325,7 +325,12 @@ class Runner { | |||
| ...sharedAuditContext, | |||
| }; | |||
|
|
|||
| const product = await audit.audit(artifacts, auditContext); | |||
| const requiredArtifacts = audit.meta.requiredArtifacts | |||
| * @return {Promise<ViewportMetaResult>} | ||
| */ | ||
| static async compute_({MetaElements}) { | ||
| static async compute_(MetaElements) { |
There was a problem hiding this comment.
a necessary refactor to ensure that they're still cached, also this seems like a bad idea to cache on the entire artifacts object anyway :)
| * @return {Promise<ViewportMetaResult>} | ||
| */ | ||
| static async compute_({MetaElements}) { | ||
| static async compute_(MetaElements) { |
| @@ -73,7 +73,7 @@ class RenderBlockingResources extends Audit { | |||
| // This audit also looks at CSSUsage but has a graceful fallback if it failed, so do not mark | |||
There was a problem hiding this comment.
do you want to change anything about CSSUsage for this audit, since after this PR it won't ever see it unless we get around to optionalArtifacts?
There was a problem hiding this comment.
oh yeah nice catch!
| .reduce((requiredArtifacts, artifactName) => { | ||
| requiredArtifacts[artifactName] = artifacts[artifactName]; | ||
| return requiredArtifacts; | ||
| }, /** @type {LH.Artifacts} */ ({})); |
There was a problem hiding this comment.
this is unfortunate, but fine enough. Maybe add a line about the masquerading type to the above comment as well?
I was hoping at one point that we could type the artifacts input of an audit based on its declared requiredArtifacts, but it would require not having Array<string> for the requiredArtifacts type, was finicky because it was nested on the meta getter, and didn't gain us all that much, but maybe in some future version of typescript...
There was a problem hiding this comment.
yeah I thought about it for about 5 seconds before thinking there was no way it was going to be usable 😆
hopefully one day typescript will let you work your magic here :)
|
ha, fascinating. The Is there an easy check we can add to |
We can add a check but it's non trivial and I'm not sure it's worth it :) I'll bump the error count for now 👍 |
Summary
One more breaking change I want to get in we discussed in the meeting that I misunderstood and thought was already on someone's plate :)
an audit can only use its
requiredArtifactsWith plugins it feels like folks aren't going to remember to use
requiredArtifactsand if we ever want to enforce it, seems like we need to do it now.This also fixes the dozen or so audits that were cheating :)