-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core: add original location to most usages of source-location #13393
Conversation
test updates pending initial feedback |
@@ -7020,6 +7020,12 @@ | |||
"duration": 100, | |||
"entryType": "measure" | |||
}, | |||
{ | |||
"startTime": 0, | |||
"name": "lh:computed:JSBundles", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are recomputing because artifacts
can contain more than SourceMaps
and ScriptElements
when we pass it to JsBundles.request
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. JSBundles isn't the only computed artifact that suffers from this, so I'll put up a PR fixing every case after this.
should we: 1) explicitly pass the keys needed or 2) introduce a second param to makeComputedArtifact
that will do some preprocessing of the input params given to it 3) do 1. but add a helper pick
fn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- introduce a second param to makeComputedArtifact that will do some preprocessing of the input params given to it
Slight preference for this because I like passing artifacts
in without much fuss. Also seems easier than validating every usage of every computed artifact.
Second param could be a shell object whose keys are the same as artifacts
. request
would only check equality on the keys of the shell object passed in. Not super pretty but it would get TS to validate everything:
module.exports = makeComputedArtifact(JSBundles, {
'ScriptElements': undefined,
'SourceMaps': undefined,
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://stackoverflow.com/questions/60131681/make-sure-array-has-all-types-from-a-union
Interesting, might be a way to do this with an array:
module.exports = makeComputedArtifact(JSBundles, ['ScriptElements', 'SourceMaps']);
Previously it was only being done in
legacy-javascript
.The
original
property is currently only used by us in the report to render source-locations using theoriginal
information (and only showing the generated location in a tooltip). It's nice to have this treatment for all audits, not justlegacy-javascript
, but the motivator for this PR is so that downstream partners don't have to juggle source maps to get the same information.*most: there is one usage in
font-size
that this does not apply to, because that is referencing a CSS file. And we don't collect source maps for CSS.