core(scripts): move to ScriptElements artifact

[patrickhulce]

Summary
Moves Scripts artifact to ScriptElements and includes information about all script elements on the page.

Related Issues/PRs
#6747 #7882

[brendankenny]

I think #7882 is going to be satisfied by structured data parsing.

But for this PR independent of that issue, I can see the argument in going general like the other *Elements, but are we making this harder to use for not much benefit?

e.g. doesn't unminified-javascript.js need to check the type now? Otherwise people doing inline shaders or whatever are going to get that showing up in that audit's results. That's going to be difficult to remember to always in audits, especially since on most websites js scripts are all you'll usually find.

Basically the more general we get the more widely useful these get (like if you want to write a unminified-glsl.js audit), but we end up passing more corner cases and trickiness onto users of artifacts instead of the gatherer taking the hit.

[brendankenny]

Maybe it's inevitable, like if you want to give warnings on invalid *Elements, you have to have those in the artifact.

We could make help structurally in the artifact shape, like dividing the top level ScriptElements into sub objects of js scripts and all-the-others scripts (or something like that)

[patrickhulce]

doesn't unminified-javascript.js need to check the type now?

Not AFAICT. We never checked the type before, just if it had content and didn't have a src, which is the same as what's happening now. Also, the type is almost always null most scripts in my experience do not specify a type nor do they need to.

This change shouldn't meaningfully impact the breadth of what we collect. It primarily impacts the properties and amount of data we provide about each script. If there are scripts with a src that weren't fetched for some reason, then those are new, but they won't have content so there's not much I can imagine one might try to do with them.

[patrickhulce]

We could make help structurally in the artifact shape, like dividing the top level ScriptElements into sub objects of js scripts and all-the-others scripts (or something like that)

I definitely like the idea of helping consumers, but IMO the benefit of consistentcy of *Element* artifacts isn't worth splitting the structure. I'd prefer to address this concern through exposed helper methods or additional flags.

[brendankenny]

We never checked the type before, just if it had content and didn't have a src, which is the same as what's happening now.

Oh, that's a good point. Inline glsl output definitely ends up in unminified-javascript output right now

Also, the type is almost always null most scripts in my experience do not specify a type nor do they need to.

but we need to know what the type was :)

I definitely like the idea of helping consumers, but IMO the benefit of consistency of Element artifacts isn't worth splitting the structure. I'd prefer to address this concern through exposed helper methods or additional flags.

Doing something like making the structure of the data into documentation of the possible pitfalls means you can't forget it (like a type bug left unnoticed for 1.25 years :). I don't know if anyone will go looking for a helper method if what we have here is "obviously" just an array of all the javascript elements in the page

[patrickhulce]

like a type bug left unnoticed for 1.25 years

if you're referring to the inline scripts here, you'll be glad to learn this has only existed for 28 days, so it's not that old :)

I agree that it will be easy to overlook and worth some of our attention to document/highlight/etc, but seeing as there's only a single usage of this artifact currently and who knows how it will be consumed, I'm just saying I'm not sure it's worth crafting bespoke artifact shapes for each *Elements artifact because we're concerned people will forget to filter for what they really were looking for.

[patrickhulce]

@hoten any thoughts here since you were just touching Scripts?

[connorjclark]

Oh, that's a good point. Inline glsl output definitely ends up in unminified-javascript output right now

this was surprising to me. In order to fix that, we would need the type... good thing this PR adds it to the artifact! let's follow up with an explicit check for JS scripts in that audit.

I do see most use cases of ScriptElements wanting to only touch javascripts. plugins that use that artifact would easily overlook that (who thinks to test inline glsl?!). can we provide a function that just returns JS scripts elements in ScriptElements? where would that go? maybe via a computed artifact JavscriptElements?

[patrickhulce]

can we provide a function that just returns JS scripts elements in ScriptElements? where would that go? maybe via a computed artifact JavscriptElements?

IMO, we should just normalize the type in the artifact in the followup that fixes unminified-javascript so that consuming the type is much easier and we/others can check script.normalizedType === 'javascript' or whatever :)

I think doing anything beyond that puts the cart before the horse a bit. Are people writing plugins that consume script element contents mostly going to target javascript? I'm not convinced the answer is yes :) The most obvious first plugins to me are actually targeting non-standard script types like json-ld or framework-specific template content. I think this highlights more that we need robust guides for consuming artifacts to help folks think through what they intend to flag, so they don't write the same bugs we do 😆

I'm going to go ahead and merge this as a step forward, and we can always discuss the ideal way to guide plugin author usage of our artifacts in the future. @hoten did you say that to mean you wanted to take the followup or shall I?

[connorjclark]

sounds good

@hoten did you say that to mean you wanted to take the followup or shall I?

go for it!