core: strict typing for audit requested artifacts

[connorjclark]

We pass only the artifacts that an audit declares via .meta.requiredArtifacts. However, the audit function takes the entire artifact type (LH.Artifacts) as a parameter, and assumes you declared things properly. This is a little scary when you consider boolean artifacts, since failure to declare one results in the audit not crashing at runtime, but always acting like it was false.

We could type this fairly easily. It's not as nice as it could be, since there's no direct way to use an array of property names in Pick<T, K> without declaring the array as a const, which is not yet supported in JSDoc. So there's duplication in the declaration - one for the type comment and one for the value itself.

EDIT: found a good way to remove duplication without loss of typing. I keep both implementations in the draft for comparison.

[patrickhulce]

I like the function approach, seems like we could put this onto Audit as .artfiacts(), still have it accept an array and do one massive replace

requiredArtifacts: Audit.artifacts([ ]),

[connorjclark]

@patrickhulce would need to do const requiredArtifacts = ... so it can be used in the jsdoc for .audit

[connorjclark]

@patrickhulce this shows how to pull const requiredArtifacts = ... into meta. But the lack of inferrable type parameters means we are stuck with an additional ugly typeof requiredArtifacts[number]

playground

[connorjclark]

I'm happy with this:

1. static get requiredArtifacts. Deprecate defining from meta, but still support indefinitely.
2. export type Select<T extends typeof _Audit> = Pick<LH.Artifacts, T['requiredArtifacts'][number]>;
3. usage in audit: * @param {LH.Artifacts.Select<typeof FirstCPUIdle>} artifacts

see first-cpu-idle.js

[connorjclark]

Could define this in the Axe audit base class, instead of in each axe audit. sg?

[connorjclark]

this works! And made the global find & replace feasible.

[connorjclark]

@patrickhulce would you be the reviewer for this? It's mostly 140 mechanical changes :)

[patrickhulce]

oof, mostly except for the part where we decide to alter our requiredArtifacts properties to getters forever ;)

I'm happy to review this change in its entirety if we get cursory buy-in to this new approach from the entire team first.

I'm fine with it, personally, but also somewhat skeptical of its usefulness compared to the noise. Is it nice to have it typed? Ya, but generally getting this wrong results in colossal obvious failure when missed, so 🤷‍♂

[patrickhulce]

summoning @GoogleChrome/lighthouse-hackers , OK with

get meta() { return {requiredArtifacts: ['MyArtifact']}; } ->
get requiredArtifacts() { return this.artifacts('MyArtifact'); }

?

[connorjclark]

Let's discuss in Monday eng meeting.