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.

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

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

Merged
merged 15 commits into from
Jan 20, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
21 changes: 21 additions & 0 deletions lighthouse-core/audits/byte-efficiency/legacy-javascript.js
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,23 @@ class LegacyJavascript extends ByteEfficiencyAudit {
return transferRatio;
}

/**
* @param {LH.Artifacts.Bundle} bundle
* @param {number} generatedLine
* @param {number} generatedColumn
* @return {LH.Audit.Details.SourceLocationValue['original']}
*/
static _findOriginalLocation(bundle, generatedLine, generatedColumn) {
const entry = bundle && bundle.map.findEntry(generatedLine, generatedColumn);
if (!entry) return;

return {
file: entry.sourceURL,
line: entry.sourceLineNumber || 0,
column: entry.sourceColumnNumber || 0,
};
}

/**
* @param {LH.Artifacts} artifacts
* @param {Array<LH.Artifacts.NetworkRequest>} networkRecords
Expand Down Expand Up @@ -434,8 +451,11 @@ class LegacyJavascript extends ByteEfficiencyAudit {
// Not needed, but keeps typescript happy.
totalBytes: 0,
};

const bundle = bundles.find(bundle => bundle.script.src === url);
for (const match of matches) {
const {name, line, column} = match;

/** @type {SubItem} */
const subItem = {
signal: name,
Expand All @@ -444,6 +464,7 @@ class LegacyJavascript extends ByteEfficiencyAudit {
url,
line,
column,
original: bundle && this._findOriginalLocation(bundle, line, column),
urlProvider: 'network',
},
};
Expand Down
59 changes: 46 additions & 13 deletions lighthouse-core/report/html/renderer/details-renderer.js
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 @@ -539,24 +539,57 @@ 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;
const file = item.original && item.original.file || '<unmapped>';
const originalLocation = item.original && `${file}:${line}:${column}`;
const generatedLocation = `${item.url}:${line}:${column}`;

// 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

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 (originalLocation) {
element = this._renderLink({
url: item.url,
text: originalLocation,
});
element.title =
`Location ${generatedLocation} maps to original location ${originalLocation}`;
Copy link
Member

Choose a reason for hiding this comment

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

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}`;

} 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
// -> 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) {
element = this._renderText(`${originalLocation} (from source map)`);
element.title = `${generatedLocation} (from sourceURL)`;
} else {
element = this._renderText(`${item.url}:${line}:${column} (from sourceURL)`);
element = this._renderText(`${generatedLocation} (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
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ describe('LegacyJavaScript audit', () => {
"location": Object {
"column": 0,
"line": 0,
"original": undefined,
"type": "source-location",
"url": "https://www.googletagmanager.com/a.js",
"urlProvider": "network",
Expand Down
7 changes: 7 additions & 0 deletions types/audit-details.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,13 @@ declare global {
urlProvider: 'network' | 'comment';
line: number;
column: number;
/** The original file location from the source map. */
original?: {
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
/** The associate file from the map's `sources` array. If missing, could not associate mapping with a specific file ("unmapped" in other tools). */
file?: string;
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?

/** 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

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