core: strict typing for audit requested artifacts#9946
Conversation
patrickhulce
left a comment
There was a problem hiding this comment.
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([ ]),
|
@patrickhulce would need to do |
|
@patrickhulce this shows how to pull |
|
I'm happy with this:
see |
| }; | ||
| } | ||
|
|
||
| static get requiredArtifacts() { |
There was a problem hiding this comment.
Could define this in the Axe audit base class, instead of in each axe audit. sg?
|
|
||
| /** | ||
| * @param {LH.Artifacts} artifacts | ||
| * @param {LH.Artifacts.Select<typeof module.exports>} artifacts |
There was a problem hiding this comment.
this works! And made the global find & replace feasible.
|
@patrickhulce would you be the reviewer for this? It's mostly 140 mechanical changes :) |
|
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 🤷♂ |
|
summoning @GoogleChrome/lighthouse-hackers , OK with
? |
|
Let's discuss in Monday eng meeting. |
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 wasfalse.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.