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 9 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
29 changes: 23 additions & 6 deletions lighthouse-core/report/html/renderer/details-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ class DetailsRenderer {

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

// Lines are shown as one-indexed.
const line = item.line + 1;
const column = item.column;
const generatedLocation = `${item.url}:${item.line + 1}:${item.column}`;
let originalLocation;
if (item.original) {
const file = item.original.file || '<unmapped>';
originalLocation = `${file}:${item.original.line + 1}:${item.original.column}`;
}

// We render slightly differently based on presence of source map and provenance of URL.
let element;
if (item.urlProvider === 'network') {
if (item.urlProvider === 'network' && originalLocation) {
Copy link
Member

Choose a reason for hiding this comment

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

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) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@connorjclark connorjclark Jan 14, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

element = this._renderText(`${generatedLocation} (from sourceURL)`);
} else {
element = this._renderText(`${item.url}:${line}:${column} (from sourceURL)`);
return null;
}

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

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
31 changes: 31 additions & 0 deletions lighthouse-core/test/report/html/renderer/details-renderer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,37 @@ describe('DetailsRenderer', () => {
assert.equal(sourceLocationEl.getAttribute('data-source-column'), `${sourceLocation.column}`);
});

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

Choose a reason for hiding this comment

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

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.

const sourceLocation = {
type: 'source-location',
url: 'https://www.example.com/script.js',
urlProvider: 'network',
line: 10,
column: 5,
original: {
file: 'main.js',
line: 100,
column: 10,
},
};
const details = {
type: 'table',
headings: [{key: 'content', itemType: 'source-location', text: 'Heading'}],
items: [{content: sourceLocation}],
};

const el = renderer.render(details);
const sourceLocationEl = el.querySelector('.lh-source-location.lh-link');
assert.strictEqual(sourceLocationEl.localName, 'a');
assert.equal(sourceLocationEl.href, 'https://www.example.com/script.js');
assert.equal(sourceLocationEl.textContent, 'main.js:101:10');
assert.equal(sourceLocationEl.title, 'maps to generated location https://www.example.com/script.js:11:5');
// DevTools should still use the generated location.
assert.equal(sourceLocationEl.getAttribute('data-source-url'), sourceLocation.url);
assert.equal(sourceLocationEl.getAttribute('data-source-line'), `${sourceLocation.line}`);
assert.equal(sourceLocationEl.getAttribute('data-source-column'), `${sourceLocation.column}`);
});

it('renders source-location with lh-link class for relative url', () => {
const sourceLocation = {
type: 'source-location',
Expand Down
15 changes: 13 additions & 2 deletions types/audit-details.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,12 +218,23 @@ 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. */
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
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 */
/**
* - `network` when the url is the actual, observed resource url.
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
* - `comment` when the url comes from a sourceMapURL comment or X-SourceMap header
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
*/
urlProvider: 'network' | 'comment';
/** Zero-indexed. */
line: number;
column: number;
/** The original file location from the source map. */
original?: {
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
/** 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;
Copy link
Member

Choose a reason for hiding this comment

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

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?

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