new_audit: add valid-source-maps audit - #11236
Conversation
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Nice work! Some quick structural feedback that's hopefully helpful.
There was a problem hiding this comment.
left comments + need some unit tests
| let missingMapsForLargeFirstPartyFile = false; | ||
| const results = []; | ||
| for (const ScriptElement of artifacts.ScriptElements) { | ||
| if (!ScriptElement.src) continue; // TODO: inline scripts, how do they work? |
There was a problem hiding this comment.
This was a note I took because I need to go through all uses of our source maps and see what happens if the script is inline with a source map url comment. I think many usages assume a network record exists, and I'm not sure what happens when that network record is the HTML document. I don't think inline bundled code is common ... so the work involved may just be to ensure it doesn't blow up Lighthouse. if supporting it would be too much work we won't bother.
I need to file an issue, then we can link to it here.
There was a problem hiding this comment.
This was a note I took because I need to go through all uses of our source maps and see what happens if the script is inline with a source map url comment.
What does it mean for the script to be inline with a source map url comment?
I don't think inline bundled code is common ... so the work involved may just be to ensure it doesn't blow up Lighthouse.
In this case, would an error be caught within the audit or break the entire runner?
There was a problem hiding this comment.
What does it mean for the script to be inline with a source map url comment?
<script>
a;b;f;d;a;a;s;s;minified code.....
// sourceMapURL=file.js.map
</script>
In this case, would an error be caught within the audit or break the entire runner?
I'm not sure, that's the investigation that this comment is alluding needs to be done. first of all, here L82 const sourceMap = wouldn't work because there is not a url to tie together a source map and an inline script. I think the source map gatherer may not even process inline scripts?
in any case, it's not a high priority concern to support. I'd just want to see what happens w/ an inline script w/ a map, dbl check none of these new audits error.
f0ccb5a to
a348a94
Compare
|
PTAL - I updated the PR description with screenshots |
|
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. |
|
should this be named just |
What about |
We actually just decided in our discussion about #11266 that verbs and phrases are 👎 and nouns with adjectives are 👍 moving forward @SaavanNanavati :)
I actually like Is your counterpoint to the |
yes |
|
The verbosity of your argument has convinced me! 😆 Can I sell you on any adjectives that attempt to combine those two ideas: invalid and/or missing
The challenge with our policy in #11266 is that this audit surfaces scripts OR source maps depending on the situation so "noun of the thing being surfaced" doesn't work so well :/ |
|
@googlebot I consent. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
(cont. from #10313)
Summary
This PR adds a new audit which fails if a user has large, first-party JavaScript with invalid or missing source maps. The audit shows a table with two columns (the JavaScript URLs and the source map URLs). Additionally, the audit performs a trivial detection of source map validity, and displays the corresponding error messages if there are any. A deeper validation of the source map is something to consider for a separate PR.
TBD: Add tests
Screenshot 1 | Audit Failure State
Screenshot 2 | Audit Success State
Screenshot 3 | Sorting Bug
Related Issues/PRs