Skip to content

Commit

Permalink
core(runner): make LH.Audit.Context immutable
Browse files Browse the repository at this point in the history
  • Loading branch information
brendankenny committed Apr 7, 2020
1 parent ae0f076 commit 21e13b4
Show file tree
Hide file tree
Showing 15 changed files with 48 additions and 46 deletions.
4 changes: 3 additions & 1 deletion lighthouse-core/audits/bootup-time.js
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -211,6 +212,7 @@ class BootupTime extends Audit {
displayValue: totalBootupTime > 0 ?
str_(i18n.UIStrings.seconds, {timeInMs: totalBootupTime}) : '',
details,
runWarnings,
};
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<LH.Config.Settings>} */
const settings = {...context.settings, throttlingMethod: 'simulate', throttling: regular3G};
const metricComputationData = {trace, devtoolsLog, settings};
const metricResult = await ComputedFcp.request(metricComputationData, context);
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/performance-budget.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class ResourceBudget extends Audit {
}

/**
* @param {LH.Budget} budget
* @param {Immutable<LH.Budget>} budget
* @param {Record<LH.Budget.ResourceType,ResourceEntry>} summary
* @return {Array<BudgetItem>}
*/
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/timing-budget.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class TimingBudget extends Audit {
}

/**
* @param {LH.Budget} budget
* @param {Immutable<LH.Budget>} budget
* @param {LH.Artifacts.TimingSummary} summary
* @return {Array<BudgetItem>}
*/
Expand Down
2 changes: 2 additions & 0 deletions lighthouse-core/computed/computed-artifact.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ function makeComputedArtifact(computableArtifact) {
* @return {ReturnType<C['compute_']>}
*/
const request = (artifacts, context) => {
/** @type {Map<string, ArbitraryEqualityMap>} */
// @ts-ignore - break immutability solely for this caching-controller function.
const computedCache = context.computedCache;
const computedName = computableArtifact.name;

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/computed/load-simulator.js
Original file line number Diff line number Diff line change
Expand Up @@ -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<LH.Config.Settings>}} data
* @param {LH.Audit.Context} context
* @return {Promise<Simulator>}
*/
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/computed/resource-summary.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class ResourceSummary {
'third-party': {count: 0, size: 0},
};
const budget = Budget.getMatchingBudget(context.settings.budgets, mainResourceURL);
let firstPartyHosts = /** @type {Array<string>} */ ([]);
let firstPartyHosts = /** @type {Immutable<Array<string>>} */ ([]);
if (budget && budget.options && budget.options.firstPartyHostnames) {
firstPartyHosts = budget.options.firstPartyHostnames;
} else {
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/config/budget.js
Original file line number Diff line number Diff line change
Expand Up @@ -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<LH.Budget>|null} budgets
* @param {Immutable<Array<LH.Budget>>|null} budgets
* @param {string} url
* @return {LH.Budget | undefined} budget
* @return {Immutable<LH.Budget> | undefined} budget
*/
static getMatchingBudget(budgets, url) {
if (budgets === null) return;
Expand Down
4 changes: 3 additions & 1 deletion lighthouse-core/lib/asset-saver.js
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,9 @@ async function saveAssets(artifacts, audits, pathWithBasename) {
* @return {Promise<void>}
*/
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);

Expand Down
11 changes: 7 additions & 4 deletions lighthouse-core/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -268,11 +268,12 @@ class Runner {
* Otherwise returns error audit result.
* @param {LH.Config.AuditDefn} auditDefn
* @param {LH.Artifacts} artifacts
* @param {Pick<LH.Audit.Context, 'settings'|'LighthouseRunWarnings'|'computedCache'>} sharedAuditContext
* @param {Pick<LH.Audit.Context, 'settings'|'computedCache'>} sharedAuditContext
* @param {Array<string>} runWarnings
* @return {Promise<LH.Audit.Result>}
* @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')}`,
Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ async function main() {
computedCache: new Map(),
options: {},
settings: /** @type {any} */ ({}),
LighthouseRunWarnings: [],
});

const items =
Expand Down
29 changes: 2 additions & 27 deletions lighthouse-core/test/runner-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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],
};
}
},
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion types/artifacts.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ declare global {
export interface MetricComputationDataInput {
devtoolsLog: DevtoolsLog;
trace: Trace;
settings: Config.Settings;
settings: Immutable<Config.Settings>;
simulator?: LanternSimulator;
}

Expand Down
8 changes: 4 additions & 4 deletions types/audit.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, any>;
settings: Config.Settings;
/** Push to this array to add top-level warnings to the LHR. */
LighthouseRunWarnings: Array<string>;
/**
* 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<string, ArbitraryEqualityMap>;
}
}>;

export interface ScoreOptions {
scorePODR: number;
Expand Down Expand Up @@ -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<string>;
}

/** The Audit.Product type for audits that do not return a `numericValue`. */
Expand Down
19 changes: 19 additions & 0 deletions types/externs.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,25 @@ declare global {
T[P];
};

/** Recursively makes all properties of T read-only. */
export type Immutable<T> =
T extends Function ? T :
T extends ReadonlyArray<infer R> ? ImmutableArray<R> :
T extends Map<infer K, infer V> ? ImmutableMap<K, V> :
T extends Set<infer M> ? ImmutableSet<M> :
T extends object ? ImmutableObject<T> :
T

// Intermediate immutable interfaces. Prefer e.g. Immutable<Set<T>> 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<T> extends ReadonlyArray<Immutable<T>> {}
interface ImmutableSet<T> extends ReadonlySet<Immutable<T>> {}
interface ImmutableMap<K, V> extends ReadonlyMap<Immutable<K>, Immutable<V>> {}
type ImmutableObject<T> = {
readonly [P in keyof T]: Immutable<T[P]>;
}

/**
* Exclude void from T
*/
Expand Down

0 comments on commit 21e13b4

Please sign in to comment.