report: use source maps to show original file name #10930

connorjclark · 2020-06-09T01:26:18Z

Demo: https://misc-hoten.surge.sh/source-location-map/report.html (go to legacy javascript)

Before (roughly):

After:

Links are blue because renderLink is different than renderTextURL. How do we want to handle this?

types/audit-details.d.ts

…ginal-map

lighthouse-core/audits/byte-efficiency/legacy-javascript.js

lighthouse-core/report/html/renderer/details-renderer.js

types/audit-details.d.ts

paulirish · 2020-08-04T16:35:58Z

types/audit-details.d.ts

@@ -224,6 +224,11 @@ declare global {
        urlProvider: 'network' | 'comment';
        line: number;
        column: number;
+        original?: {
+          file: string;


can we use url instead of file to have consistency with SourceLocationValue?

This would be a misnomer. This value comes from a source maps sources array, and are not urls, but filenames.

I added a comment to note this.

paulirish · 2020-08-04T16:36:30Z

types/audit-details.d.ts

+          file: string;
+          line: number;
+          column: number;
+        };


also... do you know what's up with this comment?

lighthouse/types/audit-details.d.ts

Line 221 in 40baa22

/** urls from the network are always valid urls. otherwise, urls come from either a comment or header, and may not be well-formed. */

" urls from the network are always valid urls" vs "urls [...] may not be well-formed" 🤔

You can put absolutely anything in a soruceMappingURL comment or the associated header. Treating this user input as a well-formed URL is error prone.

In some cases the protocol hides the true resource (network) url from us, if there is a comment url present. see

lighthouse/lighthouse-core/audits/seo/font-size.js

Line 166 in 7a462a2

const urlProvider = stylesheet.hasSourceURL ? 'comment' : 'network';

so we must have a distinction between these two sources of "url", which is what urlProvider is for.

lighthouse-core/audits/byte-efficiency/legacy-javascript.js

lighthouse-core/report/html/renderer/details-renderer.js

paulirish · 2020-08-17T20:56:04Z

lighthouse-core/report/html/renderer/details-renderer.js


-    let element;
+    // First two cases: url is from network.


it seems like there's gotta be a better way to structure these conditionals.

There are certainly different ways to structure this. I went with handling one "provider" at a time. What other way would be simpler?

you have four cases. i figure just an if else handling all those.
plus they all get addDevToolsData so that you can just return once.

diff --git a/lighthouse-core/report/html/renderer/details-renderer.js b/lighthouse-core/report/html/renderer/details-renderer.js index cef7a8897..e334c4f70 100644 --- a/lighthouse-core/report/html/renderer/details-renderer.js +++ b/lighthouse-core/report/html/renderer/details-renderer.js @@ -551,40 +551,32 @@ class DetailsRenderer { // Lines are shown as one-indexed. const generatedLocation = `${item.url}:${item.line + 1}:${item.column}`; + let element; let originalLocation; if (item.original) { const file = item.original.file || '<unmapped>'; originalLocation = `${file}:${item.original.line + 1}:${item.original.column}`; } - // First two cases: url is from network. - if (item.urlProvider === 'network') { - let element; - if (originalLocation) { - element = this._renderLink({ - url: item.url, - text: originalLocation, - }); - element.title = `maps to generated location ${generatedLocation}`; - } else { - element = this.renderTextURL(item.url); - this._dom.find('a', element).textContent += `:${item.line + 1}:${item.column}`; - } - - return addDevToolsData(element); - } - - // Two cases remain: - // 1) urlProvider === 'comment' && item.original - // -> Show the source filename in text, source mapped URL in tooltip. - // 2) urlProvider === 'comment' && !item.original - // -> Show the source mapped URL. - - let element; - if (originalLocation) { + // urlProvider reminder: + // - 'network' when the url is the actual, observed resource url. + // - 'comment' when the url comes from a sourceMapURL comment or X-SourceMap header */ + if (item.urlProvider === 'network' && originalLocation) { + element = this._renderLink({ + url: item.url, + text: originalLocation, + }); + element.title = `Source-mapped from ${generatedLocation}`; + } else if (item.urlProvider === 'network' && !originalLocation) { + element = this.renderTextURL(item.url); + this._dom.find('a', element).textContent += `:${item.line + 1}:${item.column}`; + } else if (item.urlProvider === 'comment' && originalLocation) { + // Show the source filename in text, source mapped URL in tooltip. element = this._renderText(`${originalLocation} (from source map)`); element.title = `${generatedLocation} (from sourceURL)`; } else { + // The remaining case is urlProvider === 'comment' and doesnt have an originalLocation + // Show the source mapped URL. element = this._renderText(`${generatedLocation} (from sourceURL)`); } diff --git a/types/audit-details.d.ts b/types/audit-details.d.ts index 4c5ef556b..11ec7b7e9 100644 --- a/types/audit-details.d.ts +++ b/types/audit-details.d.ts @@ -218,7 +218,7 @@ declare global { */ export interface SourceLocationValue { type: 'source-location'; - /** urls from the network are always valid urls. otherwise, urls come from either a comment or header, and may not be well-formed. */ + /** urls from the network are always valid urls. otherwise, urls come from either a comment or header (see urlProvider), and may not be well-formed. */ url: string; /** 'network' when the url is the actual, observed resource url. 'comment' when the url comes from a sourceMapURL comment or X-SourceMap header */ urlProvider: 'network' | 'comment';

this is much better thx

connorjclark · 2020-08-21T16:06:08Z

(assignee is constant and is the main reviewer as selected by devtools bot. afaiu the current system we're "using" to signal next AI is just swapping labels)

…ginal-map

patrickhulce · 2020-08-21T16:17:11Z

afaiu the current system we're "using" to signal next AI is just swapping labels

Ya, it means each person needs to keep two links handy for themselves, but it's less effort and much easier to find PRs that are stuck due to mislabeling :)

List of PRs where you are a reviewer:
https://github.com/GoogleChrome/lighthouse/pulls/assigned/<YOUR GITHUB USER>

List of PRs where you are an author:
https://github.com/GoogleChrome/lighthouse/pulls/<YOUR GITHUB USER>

List of PRs that are waiting on your review:
https://github.com/GoogleChrome/lighthouse/pulls?q=is%3Aopen+is%3Apr+assignee%3A<YOUR GITHUB USER>+label%3Awaiting4reviewer

List of PRs that are waiting on your commits:
https://github.com/GoogleChrome/lighthouse/pulls?q=is%3Aopen+is%3Apr+author%3A<YOUR GITHUB USER>+label%3Awaiting4committer

connorjclark · 2020-08-21T16:57:27Z

afaiu the current system we're "using" to signal next AI is just swapping labels

Ya, it means each person needs to keep two links handy for themselves, but it's less effort and much easier to find PRs that are stuck due to mislabeling :)

List of PRs where you are a reviewer:
https://github.com/GoogleChrome/lighthouse/pulls/assigned/<YOUR GITHUB USER>

List of PRs where you are an author:
https://github.com/GoogleChrome/lighthouse/pulls/<YOUR GITHUB USER>

List of PRs that are waiting on your review:
https://github.com/GoogleChrome/lighthouse/pulls?q=is%3Aopen+is%3Apr+assignee%3A<YOUR GITHUB USER>+label%3Awaiting4reviewer

List of PRs that are waiting on your commits:
https://github.com/GoogleChrome/lighthouse/pulls?q=is%3Aopen+is%3Apr+author%3A<YOUR GITHUB USER>+label%3Awaiting4committer

We could add to our docs somewhere what queries to put in the custom dashboard filters.

paulirish · 2020-08-21T22:32:30Z

lighthouse-core/report/html/renderer/details-renderer.js

+          text: originalLocation,
+        });
+        element.title =
+          `Location ${generatedLocation} maps to original location ${originalLocation}`;


noted about this being too long..

since the text on display is now the originalLocation.. then the title could be

Suggested change

`Location ${generatedLocation} maps to original location ${originalLocation}`;

`Source-mapped from ${generatedLocation}`;

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

…ginal-map

paulirish · 2021-01-14T23:13:17Z

just capturing the current UX when showing a sourcelocation of a mapped item. note the tooltip:

👍

paulirish

great stuff.

small nits only.

paulirish · 2021-01-14T22:53:07Z

lighthouse-core/report/html/renderer/details-renderer.js

    let element;
-    if (item.urlProvider === 'network') {
+    if (item.urlProvider === 'network' && originalLocation) {


do you mind if we add some comments in here? i always forget what urlProvider is about and wouldn't mind having a lil cheatsheet

Suggested change

if (item.urlProvider === 'network' && originalLocation) {

// Network-served resource w/ sourcemap

if (item.urlProvider === 'network' && originalLocation) {

Isn't this why we write jsdocs? hovering over this field in vs code will show this info

If originalLocation is aliased to hasSourceMap, plus we consider what I said above, it seems the code would be saying the same as your suggested comments. wdyt?

yeah adding a hasSourceMap var seems nice. That plus the upgraded .d.ts comments sorts this out completely.

paulirish · 2021-01-14T22:53:45Z

lighthouse-core/report/html/renderer/details-renderer.js

+        text: originalLocation,
+      });
+      element.title = `maps to generated location ${generatedLocation}`;
+    } else if (item.urlProvider === 'network' && !originalLocation) {


Suggested change

} else if (item.urlProvider === 'network' && !originalLocation) {

// Basic case: network-served resource, no sourcemap

} else if (item.urlProvider === 'network' && !originalLocation) {

paulirish · 2021-01-14T22:56:43Z

lighthouse-core/report/html/renderer/details-renderer.js

      element = this.renderTextURL(item.url);
-      this._dom.find('.lh-link', element).textContent += `:${line}:${column}`;
+      this._dom.find('.lh-link', element).textContent += `:${item.line + 1}:${item.column}`;
+    } else if (item.urlProvider === 'comment' && originalLocation) {


Suggested change

} else if (item.urlProvider === 'comment' && originalLocation) {

// Resource URL defined by a `sourceURL` and sourcemap also attached

} else if (item.urlProvider === 'comment' && originalLocation) {

paulirish · 2021-01-14T22:56:59Z

lighthouse-core/report/html/renderer/details-renderer.js

+    } else if (item.urlProvider === 'comment' && originalLocation) {
+      element = this._renderText(`${originalLocation} (from source map)`);
+      element.title = `${generatedLocation} (from sourceURL)`;
+    } else if (item.urlProvider === 'comment' && !originalLocation) {


Suggested change

} else if (item.urlProvider === 'comment' && !originalLocation) {

// Resource URL defined by `sourceURL`, no sourcemap

} else if (item.urlProvider === 'comment' && !originalLocation) {

types/audit-details.d.ts

…ginal-map

brendankenny · 2021-01-15T00:03:55Z

types/audit-details.d.ts

+          /** The relevant file from the map's `sources` array. If missing, could not associate mapping with a specific file ("unmapped" in other tools). */
+          file?: string;


is this a useless state? Are the line/col numbers still useful if we can't say which file the line/col numbers are in?

brendankenny · 2021-01-15T00:09:10Z

lighthouse-core/test/report/html/renderer/details-renderer-test.js

@@ -491,6 +491,37 @@ describe('DetailsRenderer', () => {
      assert.equal(sourceLocationEl.getAttribute('data-source-column'), `${sourceLocation.column}`);
    });

+    it('renders source-location values using source map data', () => {


may need a few more tests to hit the different cases, e.g. when there's no source map and the title is appended to the title renderTextURL() produces, we'd want to be sure to get an alert if changes to renderTextURL broke the rendering of source-location.

…ginal-map

report: use source maps to show original file name for source-location

25493dd

connorjclark requested a review from a team as a code owner June 9, 2020 01:26

connorjclark requested review from paulirish and removed request for a team June 9, 2020 01:26

googlebot added the cla: yes label Jun 9, 2020

devtools-bot assigned paulirish Jun 9, 2020

devtools-bot added the waiting4reviewer label Jun 9, 2020

brendankenny reviewed Jun 9, 2020

View reviewed changes

types/audit-details.d.ts Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into source-location-ori…

15009ae

…ginal-map

vercel bot deployed to Preview August 3, 2020 23:00 View deployment

patrickhulce reviewed Aug 4, 2020

View reviewed changes

paulirish reviewed Aug 17, 2020

View reviewed changes

paulirish added waiting4committer and removed waiting4reviewer labels Aug 21, 2020

paulirish assigned connorjclark and unassigned paulirish Aug 21, 2020

connorjclark assigned paulirish and unassigned connorjclark Aug 21, 2020

Merge remote-tracking branch 'origin/master' into source-location-ori…

cfd62ed

…ginal-map

pr

d61abda

vercel bot deployed to Preview August 21, 2020 16:39 View deployment

paulirish reviewed Aug 21, 2020

View reviewed changes

edit

f22bd04

vercel bot deployed to Preview August 21, 2020 22:36 View deployment

Update types/audit-details.d.ts

5cd23f8

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

vercel bot deployed to Preview August 21, 2020 23:11 View deployment

oops

832ceca

vercel bot deployed to Preview August 21, 2020 23:56 View deployment

paulirish mentioned this pull request Aug 27, 2020

core(js-usage): normalize url key #11302

Merged

connorjclark added 2 commits December 28, 2020 16:31

Merge remote-tracking branch 'origin/master' into source-location-ori…

7af5053

…ginal-map

add test

9ceca12

vercel bot deployed to Preview December 28, 2020 22:38 View deployment

connorjclark added waiting4reviewer and removed waiting4committer labels Dec 28, 2020

connorjclark mentioned this pull request Dec 29, 2020

core(console-messages): use source-location #11899

Merged

paulirish approved these changes Jan 14, 2021

View reviewed changes

connorjclark added 3 commits January 14, 2021 17:26

Merge remote-tracking branch 'origin/master' into source-location-ori…

3feb713

…ginal-map

tweak

16b7771

comments

c63b65e

vercel bot deployed to Preview January 14, 2021 23:33 View deployment

brendankenny reviewed Jan 15, 2021

View reviewed changes

connorjclark added 2 commits January 15, 2021 12:40

pr

0fbf2fc

Merge remote-tracking branch 'origin/master' into source-location-ori…

e853c83

…ginal-map

vercel bot deployed to Preview January 15, 2021 18:41 View deployment

Merge remote-tracking branch 'origin/master' into source-location-ori…

49d19be

…ginal-map

vercel bot deployed to Preview January 20, 2021 21:05 View deployment

connorjclark changed the title ~~report: use source maps to show original file name for source-location~~ report: use source maps to show original file name Jan 20, 2021

connorjclark merged commit e298b3c into master Jan 20, 2021

connorjclark deleted the source-location-original-map branch January 20, 2021 21:25

paulirish pushed a commit that referenced this pull request Mar 23, 2021

report: use source maps to show original file name (#10930)

5b20070

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: use source maps to show original file name #10930

report: use source maps to show original file name #10930

connorjclark commented Jun 9, 2020 •

edited

Loading

paulirish Aug 4, 2020

connorjclark Aug 21, 2020

paulirish Aug 4, 2020

connorjclark Aug 21, 2020

paulirish Aug 17, 2020

connorjclark Aug 21, 2020 •

edited

Loading

paulirish Aug 21, 2020

connorjclark Dec 28, 2020

connorjclark commented Aug 21, 2020 •

edited

Loading

patrickhulce commented Aug 21, 2020

connorjclark commented Aug 21, 2020

paulirish Aug 21, 2020

paulirish commented Jan 14, 2021

paulirish left a comment

paulirish Jan 14, 2021

connorjclark Jan 14, 2021

connorjclark Jan 14, 2021 •

edited

Loading

paulirish Jan 15, 2021

paulirish Jan 14, 2021

paulirish Jan 14, 2021

paulirish Jan 14, 2021

brendankenny Jan 15, 2021

brendankenny Jan 15, 2021

	`Location ${generatedLocation} maps to original location ${originalLocation}`;
	`Source-mapped from ${generatedLocation}`;

	if (item.urlProvider === 'network' && originalLocation) {
	// Network-served resource w/ sourcemap
	if (item.urlProvider === 'network' && originalLocation) {

	} else if (item.urlProvider === 'network' && !originalLocation) {
	// Basic case: network-served resource, no sourcemap
	} else if (item.urlProvider === 'network' && !originalLocation) {

	} else if (item.urlProvider === 'comment' && originalLocation) {
	// Resource URL defined by a `sourceURL` and sourcemap also attached
	} else if (item.urlProvider === 'comment' && originalLocation) {

	} else if (item.urlProvider === 'comment' && !originalLocation) {
	// Resource URL defined by `sourceURL`, no sourcemap
	} else if (item.urlProvider === 'comment' && !originalLocation) {

		/** The relevant file from the map's `sources` array. If missing, could not associate mapping with a specific file ("unmapped" in other tools). */
		file?: string;

report: use source maps to show original file name #10930

report: use source maps to show original file name #10930

Conversation

connorjclark commented Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Aug 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Aug 21, 2020 • edited Loading

patrickhulce commented Aug 21, 2020

connorjclark commented Aug 21, 2020

Choose a reason for hiding this comment

paulirish commented Jan 14, 2021

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Jun 9, 2020 •

edited

Loading

connorjclark Aug 21, 2020 •

edited

Loading

connorjclark commented Aug 21, 2020 •

edited

Loading

connorjclark Jan 14, 2021 •

edited

Loading