core(unused-javascript): check for SourceMaps before trying to use

[connorjclark]

I think #10387 broke this. unused-javascript always errors in the default config.

FWIW, #9946 would have caught this. Unfortunately, none of our tests did.

[patrickhulce]

FWIW, it still would have required a ts-ignore and manual catching in review because it's an optional artifact ;)

[patrickhulce]

as for testing, how would we feel about running the default config and just asserting no audits threw an error?

[connorjclark]

FWIW, it still would have required a ts-ignore and manual catching in review because it's an optional artifact ;)

I thought someone might call me out on that :) I did that PR before I made optional artifacts a thing, so it was just a matter of updating the PR before landing. the optional bit wasn't a technical issue, just wasn't a thing yet.

[connorjclark]

as for testing, how would we feel about running the default config and just asserting no audits threw an error?

we must do something close to this already eh?

[patrickhulce]

we must do something close to this already eh?

do we? 🤔 I don't think so. smoke for specific things, but we craft the configs usually. cli run-test doesn't check all the audits for errors and it runs on chrome://version so some might actually throw on that anyway

[brendankenny]

FWIW, it still would have required a ts-ignore and manual catching in review because it's an optional artifact ;)

I thought someone might call me out on that :) I did that PR before I made optional artifacts a thing, so it was just a matter of updating the PR before landing. the optional bit wasn't a technical issue, just wasn't a thing yet.

Part of the artifact contract is that runner checks that the required ones really are all defined before running an audit (the runtime counterpart to a type-checking approach like #9946). It makes sense to mark new optional artifacts as optional in LH.Artifacts then, especially since they're limited to internal use anyways.

(though since the SourceMaps artifact is happening so we're going to be fetching them sooner or later anyways and there's an audit that uses them now, another solution is to just graduate them to a required artifact)

[brendankenny]

what do you think of the ⬆️ @connorjclark

[connorjclark]

It makes sense to mark new optional artifacts as optional in LH.Artifacts then, especially since they're limited to internal use anyways.

It being limited to internal use is not an inherent property on the "optional artifact" feature, that's just the state of the single usage we have so far (sidenote: apparently CSSUsage in render-blocking-resources could be optional too). Marking an artifact optional on LH.Artifacts directly is nice if all usages of an artifact will always be optional across all audits, but that isn't even the case for the single case we have now (SourceMaps required by javascript-duplication, but is optional for unused-javascript).

My only point was that I think #9946 could have helped, and I think supporting the "optional" bits on the artifacts parameter would have been straightforward to add.

[brendankenny]

It being limited to internal use is not an inherent property on the "optional artifact" feature

I mean it's called __internalOptionalArtifacts, unless you mean (the indefinite article) an "optional artifact" feature :P

that's just the state of the single usage we have so far (sidenote: apparently CSSUsage in render-blocking-resources could be optional too). Marking an artifact optional on LH.Artifacts directly is nice if all usages of an artifact will always be optional across all audits, but that isn't even the case for the single case we have now (SourceMaps required by javascript-duplication, but is optional for unused-javascript).

The issue is that LH.Artifacts is utility in exchange for a lie, but it's a simple lie. Layering features on it either risks being too complex to be worth the return or breaks the type checking part. The current approach was only ever meant to be used for the default gatherers, and anything extending them needed to augment the type. That obviously doesn't work well for core. If we ever support external optional artifacts, LH.Artifacts probably shouldn't exist as the type it does today (or we could keep it simple and optional artifacts can just get their own object, I don't know).

But as I said above, SourceMaps seem baked enough that it could just be a core artifact and sidestep the problem entirely.

In any case, it seems like everyone is some level of ok with living with this for now so we can move on to the next thing :)

[brendankenny]

as for testing, how would we feel about running the default config and just asserting no audits threw an error?

we must do something close to this already eh?

do we? 🤔 I don't think so. smoke for specific things, but we craft the configs usually. cli run-test doesn't check all the audits for errors and it runs on chrome://version so some might actually throw on that anyway

yarn update:sample-json runs the default config so could be checked, but it runs against a set of artifacts not necessarily generated from the default config (in this case, "SourceMaps": [] was added :)

[brendankenny]

LGTM

[connorjclark]

@paulirish why does build tracker keep failing