core(scriptelements): expose id attribute for ScriptElements

[uchoudh-zz]

Summary

This is a small feature to include the id attribute in the output when ScriptElements are requested in an audit.

This change makes it easier to choose a specific script and adds ease of use when writing audits.

Related Issues/PRs
This is the issue that the PR addresses. #9681

[brendankenny]

thanks for taking this on @uchoudh!

There are a few more places that will need updating:

• the artifacts type for ScriptElements. Probably helpful to leave a comment that the value will be null if the element had no id or if source is network.
• an explicit null id value down in the version returned in L81-90 (no id for those because they are js file network requests)
• run yarn update:sample-artifacts ScriptElements to update a set of saved test artifacts to include id where applicable
• run yarn update:sample-json to update any cases where the updated artifacts may affect things (I don't imagine this will end up doing anything because we don't yet use id anywhere :)

thanks, and don't hesitate to leave a comment if you run into unexpected trouble!

[uchoudh-zz]

First three changes are looking good. Having some trouble with yarn update:sample-json.
Getting this error regarding google protobuff
Install protobuf ≥ 3.7.1 to compile the proto file

Steps I took:

1. npm install google-protobuf
2. Download the protobuf compiler from https://github.com/protocolbuffers/protobuf/releases/tag/v3.10.0-rc1

After these steps not sure how to use it with the lighthouse project.

[patrickhulce]

@uchoudh are you using a Mac and homebrew?

I just needed to brew install protobuf (or brew upgrade protobuf if you have an older version) without manually downloading the compiler from github.

[uchoudh-zz]

@patrickhulce Yes I am using a mac and homebrew. I tried the what you suggested but the problem still persists. I think it's still an error with protobuf not bring installed properly but the error message is a little different.
ImportError: No module named google.protobuf.internal
Looked around for similar issues but none of those solutions seem to fix it for me.

[brendankenny]

@uchoudh I think some people have run into this before. Check out this comment: #9084 (comment)

You can also commit what you have and one of us can run the command.

Sorry for the trouble!

[uchoudh-zz]

@uchoudh I think some people have run into this before. Check out this comment: #9084 (comment)

You can also commit what you have and one of us can run the command.

Sorry for the trouble!

Thank you. I've pushed my latest changes.

[brendankenny]

Looks like we haven't run yarn update:sample-artifacts ScriptElements in a while so it was pulling in a lot more changes than I thought it would (e.g. #9672 makes unminified-javascript pass in sample_v2.json).

I reverted most of it and kept just the id entries so that someone else can deal with it someday :) I also sprinkled in a few IDs to make sure they were making their way to the artifacts, even though no one is using them yet.

• run yarn update:sample-json to update any cases where the updated artifacts may affect things (I don't imagine this will end up doing anything because we don't yet use id anywhere :)

with these more minimal changes to the saved artifacts, yarn update:sample-json makes no updates

[uchoudh-zz]

@googlebot I consent.

[brendankenny]

oh googlebot...

[brendankenny]

LGTM, thanks @uchoudh!

[brendankenny]

another annoying failure for new contributors :) Made the smoke test expectation less sensitive because we don't need to verify the exact column number

[googlebot]

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

[brendankenny]

Thanks @uchoudh!