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

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

Merged
merged 11 commits into from
Jan 14, 2021
22 changes: 3 additions & 19 deletions lighthouse-core/audits/deprecations.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

const Audit = require('./audit.js');
const i18n = require('../lib/i18n/i18n.js');
const ConsoleMessages = require('../gather/gatherers/console-messages.js');

const UIStrings = {
/** Title of a Lighthouse audit that provides detail on the use of deprecated APIs. This descriptive title is shown to users when the page does not use deprecated APIs. */
Expand Down Expand Up @@ -57,33 +58,16 @@ class Deprecations extends Audit {
const entries = artifacts.ConsoleMessages;

const deprecations = entries.filter(log => log.source === 'deprecation').map(log => {
// HTML deprecations will have no url and no way to attribute to a specific line.
/** @type {LH.Audit.Details.SourceLocationValue=} */
let source;
if (log.url) {
// JS deprecations will have a stack trace.
// CSS deprecations only expose a line number.
const topCallFrame = log.stackTrace && log.stackTrace.callFrames[0];
const line = log.lineNumber || 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the call frames are now used in ConsoleMessages to set line/col, so we can just defer to that here now.

const column = topCallFrame ? topCallFrame.columnNumber : 0;
source = {
type: 'source-location',
url: log.url,
urlProvider: 'network',
line,
column,
};
}
return {
value: log.text,
source,
source: ConsoleMessages.createSourceLocation(log),
};
});

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'value', itemType: 'text', text: str_(UIStrings.columnDeprecate)},
{key: 'source', itemType: 'source-location', text: str_(i18n.UIStrings.columnURL)},
{key: 'source', itemType: 'source-location', text: str_(i18n.UIStrings.columnSource)},
];
const details = Audit.makeTableDetails(headings, deprecations);

Expand Down
3 changes: 1 addition & 2 deletions lighthouse-core/audits/dobetterweb/geolocation-on-start.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,7 @@ class GeolocationOnStart extends ViolationAudit {

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: str_(i18n.UIStrings.columnURL)},
{key: 'label', itemType: 'text', text: str_(i18n.UIStrings.columnLocation)},
{key: 'source', itemType: 'source-location', text: str_(i18n.UIStrings.columnSource)},
];
// TODO(bckenny): there should actually be a ts error here. results[0].stackTrace
// should violate the results type. Shouldn't be removed from details items regardless.
Expand Down
3 changes: 1 addition & 2 deletions lighthouse-core/audits/dobetterweb/no-document-write.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,7 @@ class NoDocWriteAudit extends ViolationAudit {

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: str_(i18n.UIStrings.columnURL)},
{key: 'label', itemType: 'text', text: str_(i18n.UIStrings.columnLocation)},
{key: 'source', itemType: 'source-location', text: str_(i18n.UIStrings.columnSource)},
];
// TODO(bckenny): see TODO in geolocation-on-start
const details = ViolationAudit.makeTableDetails(headings, results);
Expand Down
3 changes: 1 addition & 2 deletions lighthouse-core/audits/dobetterweb/notification-on-start.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@ class NotificationOnStart extends ViolationAudit {

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: str_(i18n.UIStrings.columnURL)},
{key: 'label', itemType: 'text', text: str_(i18n.UIStrings.columnLocation)},
{key: 'source', itemType: 'source-location', text: str_(i18n.UIStrings.columnSource)},
];
// TODO(bckenny): see TODO in geolocation-on-start
const details = ViolationAudit.makeTableDetails(headings, results);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@ class PassiveEventsAudit extends ViolationAudit {

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: str_(i18n.UIStrings.columnURL)},
{key: 'label', itemType: 'text', text: str_(i18n.UIStrings.columnLocation)},
{key: 'source', itemType: 'source-location', text: str_(i18n.UIStrings.columnSource)},
];
// TODO(bckenny): see TODO in geolocation-on-start
const details = ViolationAudit.makeTableDetails(headings, results);
Expand Down
7 changes: 4 additions & 3 deletions lighthouse-core/audits/errors-in-console.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
const log = require('lighthouse-logger');
const Audit = require('./audit.js');
const i18n = require('../lib/i18n/i18n.js');
const ConsoleMessages = require('../gather/gatherers/console-messages.js');

const UIStrings = {
/** Title of a Lighthouse audit that provides detail on browser errors. This descriptive title is shown to users when no browser errors were logged into the devtools console. */
Expand Down Expand Up @@ -48,7 +49,6 @@ class ErrorLogs extends Audit {
return {};
}


/**
* @template {{description: string | undefined}} T
* @param {Array<T>} items
Expand Down Expand Up @@ -81,14 +81,15 @@ class ErrorLogs extends Audit {
/** @type {AuditOptions} */
const auditOptions = context.options;

/** @type {Array<{source: string, description: string|undefined, url: string|undefined}>} */
/** @type {Array<{source: string, description: string|undefined, sourceLocation: LH.Audit.Details.SourceLocationValue|undefined}>} */
const consoleRows = artifacts.ConsoleMessages
.filter(item => item.level === 'error')
.map(item => {
return {
source: item.source,
description: item.text,
url: item.url,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wondering if url should stick around and be removed in v8 (ditto for all these other audits), or are we good with removing the old properties immediately?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion on whether we should keep the url field around until v8. However, if you decide to leave it for now please add a TODO to remove it later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like leaving it here too 👍

sourceLocation: ConsoleMessages.createSourceLocation(item),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's unfortunate that source can't be used here, like every other audit.

};
});

Expand All @@ -97,7 +98,7 @@ class ErrorLogs extends Audit {

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: str_(i18n.UIStrings.columnURL)},
{key: 'sourceLocation', itemType: 'source-location', text: str_(i18n.UIStrings.columnSource)},
{key: 'description', itemType: 'code', text: str_(i18n.UIStrings.columnDescription)},
];

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/no-unload-listeners.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class NoUnloadListeners extends Audit {

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'source', itemType: 'source-location', text: str_(i18n.UIStrings.columnURL)},
{key: 'source', itemType: 'source-location', text: str_(i18n.UIStrings.columnSource)},
];

// Look up scriptId to script URL via the JsUsage artifact.
Expand Down
24 changes: 18 additions & 6 deletions lighthouse-core/audits/violation-audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,26 +6,38 @@
'use strict';

const Audit = require('./audit.js');
const ConsoleMessages = require('../gather/gatherers/console-messages.js');

class ViolationAudit extends Audit {
/**
* @param {LH.Artifacts} artifacts
* @param {RegExp} pattern
* @return {Array<{label: string, url?: string}>}
* @return {Array<{source: LH.Audit.Details.SourceLocationValue}>}
*/
static getViolationResults(artifacts, pattern) {
/**
* @template T
* @param {T} value
* @return {value is Exclude<T, undefined>}
*/
function filterUndefined(value) {
return value !== undefined;
}

const seen = new Set();
return artifacts.ConsoleMessages
.filter(entry => entry.url && entry.source === 'violation' && pattern.test(entry.text))
.map(entry => ({label: `line: ${entry.lineNumber}`, url: entry.url}))
.filter(entry => {
// Filter out duplicate entries by URL/label since they are not differentiable to the user
.map(entry => ConsoleMessages.createSourceLocation(entry))
.filter(filterUndefined)
.filter(source => {
// Filter out duplicate entries since they are not differentiable to the user
// @see https://github.com/GoogleChrome/lighthouse/issues/5218
const key = `${entry.url}!${entry.label}`;
const key = `${source.url}!${source.line}!${source.column}`;
if (seen.has(key)) return false;
seen.add(key);
return true;
});
})
.map(source => ({source}));
}
}

Expand Down
24 changes: 24 additions & 0 deletions lighthouse-core/gather/gatherers/console-messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,22 @@ function remoteObjectToString(obj) {
}

class ConsoleMessages extends Gatherer {
/**
* @param {LH.Artifacts.ConsoleMessage} entry
* @return {LH.Audit.Details.SourceLocationValue | undefined}
*/
static createSourceLocation(entry) {
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
if (!entry.url) return;

return {
type: 'source-location',
url: entry.url,
urlProvider: 'network',
line: entry.lineNumber || 0,
column: entry.columnNumber || 0,
};
}

constructor() {
super();
/** @type {LH.Artifacts.ConsoleMessage[]} */
Expand All @@ -51,13 +67,15 @@ class ConsoleMessages extends Gatherer {
// Only gather warnings and errors for brevity.
return;
}

/** @type {LH.Crdp.Runtime.RemoteObject[]} */
const args = event.args || [];
const text = args.map(remoteObjectToString).join(' ');
if (!text && !event.stackTrace) {
// No useful information from Chrome. Skip.
return;
}

const {url, lineNumber, columnNumber} =
event.stackTrace && event.stackTrace.callFrames[0] || {};
/** @type {LH.Artifacts.ConsoleMessage} */
Expand Down Expand Up @@ -107,6 +125,11 @@ class ConsoleMessages extends Gatherer {
*/
onLogEntry(event) {
const {source, level, text, stackTrace, timestamp, url, lineNumber} = event.entry;

// JS events have a stack trace, which we use to get the column.
// CSS/HTML events only expose a line number.
const {columnNumber} = event.entry.stackTrace && event.entry.stackTrace.callFrames[0] || {};

this._logEntries.push({
eventType: 'protocolLog',
source,
Expand All @@ -116,6 +139,7 @@ class ConsoleMessages extends Gatherer {
timestamp,
url,
lineNumber,
columnNumber,
});
}

Expand Down
4 changes: 2 additions & 2 deletions types/artifacts.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -773,9 +773,9 @@ declare global {
stackTrace?: Crdp.Runtime.StackTrace;
/** The URL of the log/exception, if known. */
url?: string;
/** Line number in the script (0-based), if known. */
/** Line number in the script (0-indexed), if known. */
lineNumber?: number;
/** Column number in the script (0-based), if known. */
/** Column number in the script (0-indexed), if known. */
columnNumber?: number;
}

Expand Down