From 21e13b420f08a1c2b49e1841e878378e1a314bef Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Tue, 7 Apr 2020 16:15:14 -0400 Subject: [PATCH] core(runner): make LH.Audit.Context immutable --- lighthouse-core/audits/bootup-time.js | 4 ++- .../metrics/first-contentful-paint-3g.js | 2 +- lighthouse-core/audits/performance-budget.js | 2 +- lighthouse-core/audits/timing-budget.js | 2 +- lighthouse-core/computed/computed-artifact.js | 2 ++ lighthouse-core/computed/load-simulator.js | 2 +- lighthouse-core/computed/resource-summary.js | 2 +- lighthouse-core/config/budget.js | 4 +-- lighthouse-core/lib/asset-saver.js | 4 ++- lighthouse-core/runner.js | 11 ++++--- .../legacy-javascript/examine-latest-run.js | 1 - lighthouse-core/test/runner-test.js | 29 ++----------------- types/artifacts.d.ts | 2 +- types/audit.d.ts | 8 ++--- types/externs.d.ts | 19 ++++++++++++ 15 files changed, 48 insertions(+), 46 deletions(-) diff --git a/lighthouse-core/audits/bootup-time.js b/lighthouse-core/audits/bootup-time.js index 000d9f16b67b..260eec8c2ede 100644 --- a/lighthouse-core/audits/bootup-time.js +++ b/lighthouse-core/audits/bootup-time.js @@ -181,8 +181,9 @@ class BootupTime extends Audit { // TODO: consider moving this to core gathering so you don't need to run the audit for warning + let runWarnings; if (hadExcessiveChromeExtension) { - context.LighthouseRunWarnings.push(str_(UIStrings.chromeExtensionsWarning)); + runWarnings = [str_(UIStrings.chromeExtensionsWarning)]; } const summary = {wastedMs: totalBootupTime}; @@ -211,6 +212,7 @@ class BootupTime extends Audit { displayValue: totalBootupTime > 0 ? str_(i18n.UIStrings.seconds, {timeInMs: totalBootupTime}) : '', details, + runWarnings, }; } } diff --git a/lighthouse-core/audits/metrics/first-contentful-paint-3g.js b/lighthouse-core/audits/metrics/first-contentful-paint-3g.js index fabf81ded0b4..cfb9c1b88098 100644 --- a/lighthouse-core/audits/metrics/first-contentful-paint-3g.js +++ b/lighthouse-core/audits/metrics/first-contentful-paint-3g.js @@ -44,7 +44,7 @@ class FirstContentfulPaint3G extends Audit { static async audit(artifacts, context) { const trace = artifacts.traces[Audit.DEFAULT_PASS]; const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS]; - /** @type {LH.Config.Settings} */ + /** @type {Immutable} */ const settings = {...context.settings, throttlingMethod: 'simulate', throttling: regular3G}; const metricComputationData = {trace, devtoolsLog, settings}; const metricResult = await ComputedFcp.request(metricComputationData, context); diff --git a/lighthouse-core/audits/performance-budget.js b/lighthouse-core/audits/performance-budget.js index 7ab2e6551fff..99a885798ac9 100644 --- a/lighthouse-core/audits/performance-budget.js +++ b/lighthouse-core/audits/performance-budget.js @@ -64,7 +64,7 @@ class ResourceBudget extends Audit { } /** - * @param {LH.Budget} budget + * @param {Immutable} budget * @param {Record} summary * @return {Array} */ diff --git a/lighthouse-core/audits/timing-budget.js b/lighthouse-core/audits/timing-budget.js index c700b048c1c9..cb1367faebd5 100644 --- a/lighthouse-core/audits/timing-budget.js +++ b/lighthouse-core/audits/timing-budget.js @@ -80,7 +80,7 @@ class TimingBudget extends Audit { } /** - * @param {LH.Budget} budget + * @param {Immutable} budget * @param {LH.Artifacts.TimingSummary} summary * @return {Array} */ diff --git a/lighthouse-core/computed/computed-artifact.js b/lighthouse-core/computed/computed-artifact.js index 4d096954826e..0572f1dea4a8 100644 --- a/lighthouse-core/computed/computed-artifact.js +++ b/lighthouse-core/computed/computed-artifact.js @@ -25,6 +25,8 @@ function makeComputedArtifact(computableArtifact) { * @return {ReturnType} */ const request = (artifacts, context) => { + /** @type {Map} */ + // @ts-ignore - break immutability solely for this caching-controller function. const computedCache = context.computedCache; const computedName = computableArtifact.name; diff --git a/lighthouse-core/computed/load-simulator.js b/lighthouse-core/computed/load-simulator.js index 4523dde86878..af34f9d1d400 100644 --- a/lighthouse-core/computed/load-simulator.js +++ b/lighthouse-core/computed/load-simulator.js @@ -12,7 +12,7 @@ const NetworkAnalysis = require('./network-analysis.js'); class LoadSimulator { /** - * @param {{devtoolsLog: LH.DevtoolsLog, settings: LH.Config.Settings}} data + * @param {{devtoolsLog: LH.DevtoolsLog, settings: Immutable}} data * @param {LH.Audit.Context} context * @return {Promise} */ diff --git a/lighthouse-core/computed/resource-summary.js b/lighthouse-core/computed/resource-summary.js index bcbbfd818791..7bfeebe8db73 100644 --- a/lighthouse-core/computed/resource-summary.js +++ b/lighthouse-core/computed/resource-summary.js @@ -51,7 +51,7 @@ class ResourceSummary { 'third-party': {count: 0, size: 0}, }; const budget = Budget.getMatchingBudget(context.settings.budgets, mainResourceURL); - let firstPartyHosts = /** @type {Array} */ ([]); + let firstPartyHosts = /** @type {Immutable>} */ ([]); if (budget && budget.options && budget.options.firstPartyHostnames) { firstPartyHosts = budget.options.firstPartyHostnames; } else { diff --git a/lighthouse-core/config/budget.js b/lighthouse-core/config/budget.js index 0f376c04f696..bc61e9d3d42a 100644 --- a/lighthouse-core/config/budget.js +++ b/lighthouse-core/config/budget.js @@ -136,9 +136,9 @@ class Budget { * Returns the budget that applies to a given URL. * If multiple budgets match based on thier 'path' property, * then the last-listed of those budgets is returned. - * @param {Array|null} budgets + * @param {Immutable>|null} budgets * @param {string} url - * @return {LH.Budget | undefined} budget + * @return {Immutable | undefined} budget */ static getMatchingBudget(budgets, url) { if (budgets === null) return; diff --git a/lighthouse-core/lib/asset-saver.js b/lighthouse-core/lib/asset-saver.js index 99cb8615f8a0..f971a40a202e 100644 --- a/lighthouse-core/lib/asset-saver.js +++ b/lighthouse-core/lib/asset-saver.js @@ -273,7 +273,9 @@ async function saveAssets(artifacts, audits, pathWithBasename) { * @return {Promise} */ async function saveLanternNetworkData(devtoolsLog, outputPath) { - const context = /** @type {LH.Audit.Context} */ ({computedCache: new Map()}); + /** @type {LH.Audit.Context} */ + // @ts-ignore - the full audit context isn't needed for analysis. + const context = {computedCache: new Map()}; const networkAnalysis = await NetworkAnalysisComputed.request(devtoolsLog, context); const lanternData = LoadSimulatorComputed.convertAnalysisToSaveableLanternData(networkAnalysis); diff --git a/lighthouse-core/runner.js b/lighthouse-core/runner.js index d2d5ff90e652..dc57d836ea73 100644 --- a/lighthouse-core/runner.js +++ b/lighthouse-core/runner.js @@ -248,14 +248,14 @@ class Runner { // Members of LH.Audit.Context that are shared across all audits. const sharedAuditContext = { settings, - LighthouseRunWarnings: runWarnings, computedCache: new Map(), }; // Run each audit sequentially const auditResults = []; for (const auditDefn of audits) { - const auditResult = await Runner._runAudit(auditDefn, artifacts, sharedAuditContext); + const auditResult = await Runner._runAudit(auditDefn, artifacts, sharedAuditContext, + runWarnings); auditResults.push(auditResult); } @@ -268,11 +268,12 @@ class Runner { * Otherwise returns error audit result. * @param {LH.Config.AuditDefn} auditDefn * @param {LH.Artifacts} artifacts - * @param {Pick} sharedAuditContext + * @param {Pick} sharedAuditContext + * @param {Array} runWarnings * @return {Promise} * @private */ - static async _runAudit(auditDefn, artifacts, sharedAuditContext) { + static async _runAudit(auditDefn, artifacts, sharedAuditContext, runWarnings) { const audit = auditDefn.implementation; const status = { msg: `Auditing: ${i18n.getFormatted(audit.meta.title, 'en-US')}`, @@ -341,6 +342,8 @@ class Runner { return narrowedArtifacts; }, /** @type {LH.Artifacts} */ ({})); const product = await audit.audit(narrowedArtifacts, auditContext); + runWarnings.push(...product.runWarnings || []); + auditResult = Audit.generateAuditResult(audit, product); } catch (err) { // Log error if it hasn't already been logged above. diff --git a/lighthouse-core/scripts/legacy-javascript/examine-latest-run.js b/lighthouse-core/scripts/legacy-javascript/examine-latest-run.js index b5c4ae66dd32..32248c63298a 100644 --- a/lighthouse-core/scripts/legacy-javascript/examine-latest-run.js +++ b/lighthouse-core/scripts/legacy-javascript/examine-latest-run.js @@ -49,7 +49,6 @@ async function main() { computedCache: new Map(), options: {}, settings: /** @type {any} */ ({}), - LighthouseRunWarnings: [], }); const items = diff --git a/lighthouse-core/test/runner-test.js b/lighthouse-core/test/runner-test.js index ff0c990a09df..c0605d76d6dc 100644 --- a/lighthouse-core/test/runner-test.js +++ b/lighthouse-core/test/runner-test.js @@ -654,11 +654,11 @@ describe('Runner', () => { static get meta() { return basicAuditMeta; } - static audit(artifacts, context) { - context.LighthouseRunWarnings.push(warningString); + static audit() { return { numericValue: 5, score: 1, + runWarnings: [warningString], }; } }, @@ -670,31 +670,6 @@ describe('Runner', () => { }); }); - it('includes any LighthouseRunWarnings from errored audits in LHR', () => { - const warningString = 'Audit warning just before a terrible error!'; - - const config = new Config({ - settings: { - auditMode: __dirname + '/fixtures/artifacts/empty-artifacts/', - }, - audits: [ - class WarningAudit extends Audit { - static get meta() { - return basicAuditMeta; - } - static audit(artifacts, context) { - context.LighthouseRunWarnings.push(warningString); - throw new Error('Terrible.'); - } - }, - ], - }); - - return Runner.run(null, {config, driverMock}).then(results => { - assert.deepStrictEqual(results.lhr.runWarnings, [warningString]); - }); - }); - describe('lhr.runtimeError', () => { const NO_FCP = LHError.errors.NO_FCP; class RuntimeErrorGatherer extends Gatherer { diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index f1fdd4bbe150..6602a1c6cde8 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -509,7 +509,7 @@ declare global { export interface MetricComputationDataInput { devtoolsLog: DevtoolsLog; trace: Trace; - settings: Config.Settings; + settings: Immutable; simulator?: LanternSimulator; } diff --git a/types/audit.d.ts b/types/audit.d.ts index 193f8a3d0aa5..c2631070136e 100644 --- a/types/audit.d.ts +++ b/types/audit.d.ts @@ -8,19 +8,17 @@ import ArbitraryEqualityMap = require('../lighthouse-core/lib/arbitrary-equality declare global { module LH.Audit { - export interface Context { + export type Context = Immutable<{ /** audit options */ options: Record; settings: Config.Settings; - /** Push to this array to add top-level warnings to the LHR. */ - LighthouseRunWarnings: Array; /** * Nested cache for already-computed computed artifacts. Keyed first on * the computed artifact's `name` property, then on input artifact(s). * Values are Promises resolving to the computedArtifact result. */ computedCache: Map; - } + }>; export interface ScoreOptions { scorePODR: number; @@ -85,6 +83,8 @@ declare global { notApplicable?: boolean; /** Extra information about the page provided by some types of audits, in one of several possible forms that can be rendered in the HTML report. */ details?: Audit.Details; + /** If an audit encounters unusual execution circumstances, strings can be put in this optional array to add top-level warnings to the LHR. */ + runWarnings?: Array; } /** The Audit.Product type for audits that do not return a `numericValue`. */ diff --git a/types/externs.d.ts b/types/externs.d.ts index ad87fdb5c9c4..2ba2d0dcc411 100644 --- a/types/externs.d.ts +++ b/types/externs.d.ts @@ -76,6 +76,25 @@ declare global { T[P]; }; + /** Recursively makes all properties of T read-only. */ + export type Immutable = + T extends Function ? T : + T extends ReadonlyArray ? ImmutableArray : + T extends Map ? ImmutableMap : + T extends Set ? ImmutableSet : + T extends object ? ImmutableObject : + T + + // Intermediate immutable interfaces. Prefer e.g. Immutable> over direct use. + // TODO: switch to recursive type references when using tsc ≥ 3.7 + // see https://github.com/microsoft/TypeScript/issues/13923#issuecomment-557509399 + interface ImmutableArray extends ReadonlyArray> {} + interface ImmutableSet extends ReadonlySet> {} + interface ImmutableMap extends ReadonlyMap, Immutable> {} + type ImmutableObject = { + readonly [P in keyof T]: Immutable; + } + /** * Exclude void from T */