Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 15 commits into from
Jan 20, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
25493dd
report: use source maps to show original file name for source-location
connorjclark Jun 9, 2020
15009ae
Merge remote-tracking branch 'origin/master' into source-location-ori…
connorjclark Aug 3, 2020
cfd62ed
Merge remote-tracking branch 'origin/master' into source-location-ori…
connorjclark Aug 21, 2020
d61abda
pr
connorjclark Aug 21, 2020
f22bd04
edit
connorjclark Aug 21, 2020
5cd23f8
Update types/audit-details.d.ts
connorjclark Aug 21, 2020
832ceca
oops
connorjclark Aug 21, 2020
7af5053
Merge remote-tracking branch 'origin/master' into source-location-ori…
connorjclark Dec 28, 2020
9ceca12
add test
connorjclark Dec 28, 2020
3feb713
Merge remote-tracking branch 'origin/master' into source-location-ori…
connorjclark Jan 14, 2021
16b7771
tweak
connorjclark Jan 14, 2021
c63b65e
comments
connorjclark Jan 14, 2021
0fbf2fc
pr
connorjclark Jan 15, 2021
e853c83
Merge remote-tracking branch 'origin/master' into source-location-ori…
connorjclark Jan 15, 2021
49d19be
Merge remote-tracking branch 'origin/master' into source-location-ori…
connorjclark Jan 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions lighthouse-core/audits/byte-efficiency/legacy-javascript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,18 @@ class LegacyJavascript extends ByteEfficiencyAudit {
};
for (const match of matches) {
const {name, line, column} = match;

let original;
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
const bundle = bundles.find(bundle => bundle.script.src === url);
const entry = bundle && bundle.map.findEntry(line, column);
if (entry) {
original = {
file: entry.sourceURL || '<unmapped>',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
line: entry.sourceLineNumber || 0,
column: entry.sourceColumnNumber || 0,
};
}

/** @type {SubItem} */
const subItem = {
signal: name,
Expand All @@ -444,6 +456,7 @@ class LegacyJavascript extends ByteEfficiencyAudit {
url,
line,
column,
original,
urlProvider: 'network',
},
};
Expand Down
50 changes: 38 additions & 12 deletions lighthouse-core/report/html/renderer/details-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ class DetailsRenderer {

/**
* @param {{text: string, url: string}} details
* @return {Element}
* @return {HTMLElement}
*/
_renderLink(details) {
const allowedProtocols = ['https:', 'http:'];
Expand Down Expand Up @@ -536,24 +536,50 @@ class DetailsRenderer {
return null;
}

/** @param {HTMLElement} el */
function addDevToolsData(el) {
el.classList.add('lh-source-location');
el.setAttribute('data-source-url', item.url);
// DevTools expects zero-indexed lines.
el.setAttribute('data-source-line', String(item.line));
el.setAttribute('data-source-column', String(item.column));
return el;
}

// Lines are shown as one-indexed.
const line = item.line + 1;
const column = item.column;
const line = (item.original ? item.original.line : item.line) + 1;
const column = item.original ? item.original.column : item.column;

let element;
// First two cases: url is from network.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@connorjclark connorjclark Aug 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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';

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is much better thx

if (item.urlProvider === 'network') {
element = this.renderTextURL(item.url);
this._dom.find('a', element).textContent += `:${line}:${column}`;
let element;
if (item.original) {
element = this._renderLink({
url: item.url,
text: `${item.original.file}:${line}:${column}`,
});
element.title = `${item.url}:${line}:${column}`;
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
} else {
element = this.renderTextURL(item.url);
this._dom.find('a', element).textContent += `:${line}:${column}`;
}

return addDevToolsData(element);
}

// Two cases remain:
// 1) urlProvider === 'comment' && item.original
// 2) urlProvider === 'comment' && !item.original
connorjclark marked this conversation as resolved.
Show resolved Hide resolved

let element;
if (item.original) {
element = this._renderText(`${item.original.file}:${line}:${column} (from source map)`);
element.title = `${item.url}:${line}:${column} (from sourceURL)`;
} else {
element = this._renderText(`${item.url}:${line}:${column} (from sourceURL)`);
}

element.classList.add('lh-source-location');
element.setAttribute('data-source-url', item.url);
// DevTools expects zero-indexed lines.
element.setAttribute('data-source-line', String(item.line));
element.setAttribute('data-source-column', String(item.column));
return element;
return addDevToolsData(element);
}

/**
Expand Down
5 changes: 5 additions & 0 deletions types/audit-details.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,11 @@ declare global {
urlProvider: 'network' | 'comment';
line: number;
column: number;
original?: {
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
file: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

line: number;
column: number;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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" 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
Expand Down