From b8ff5a51e3694b1e46ed4e590efc6393e3a9e3ba Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 1 Dec 2020 15:56:45 -0600 Subject: [PATCH 1/3] core(config): remove gatherer options support --- lighthouse-core/config/config.js | 20 +++---- lighthouse-core/gather/gather-runner.js | 6 -- lighthouse-core/test/config/config-test.js | 26 +++------ .../test/gather/gather-runner-test.js | 58 ------------------- types/config.d.ts | 1 - types/gatherer.d.ts | 1 - 6 files changed, 15 insertions(+), 97 deletions(-) diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js index 285099e0a75f..e53931cd4255 100644 --- a/lighthouse-core/config/config.js +++ b/lighthouse-core/config/config.js @@ -16,7 +16,7 @@ const path = require('path'); const Runner = require('../runner.js'); const ConfigPlugin = require('./config-plugin.js'); const Budget = require('./budget.js'); -const {requireAudits, mergeOptionsOfItems, resolveModule} = require('./config-helpers.js'); +const {requireAudits, resolveModule} = require('./config-helpers.js'); /** @typedef {typeof import('../gather/gatherers/gatherer.js')} GathererConstructor */ /** @typedef {InstanceType} Gatherer */ @@ -374,10 +374,6 @@ class Config { gathererDefn.implementation = undefined; // @ts-expect-error Breaking the Config.GathererDefn type. gathererDefn.instance = undefined; - if (Object.keys(gathererDefn.options).length === 0) { - // @ts-expect-error Breaking the Config.GathererDefn type. - gathererDefn.options = undefined; - } } } } @@ -741,12 +737,11 @@ class Config { /** * @param {string} path - * @param {{}|undefined} options * @param {Array} coreAuditList * @param {string=} configDir * @return {LH.Config.GathererDefn} */ - static requireGathererFromPath(path, options, coreAuditList, configDir) { + static requireGathererFromPath(path, coreAuditList, configDir) { const coreGatherer = coreAuditList.find(a => a === `${path}.js`); let requirePath = `../gather/gatherers/${path}`; @@ -761,7 +756,6 @@ class Config { instance: new GathererClass(), implementation: GathererClass, path, - options: options || {}, }; } @@ -788,7 +782,6 @@ class Config { instance: gathererDefn.instance, implementation: gathererDefn.implementation, path: gathererDefn.path, - options: gathererDefn.options || {}, }; } else if (gathererDefn.implementation) { const GathererClass = gathererDefn.implementation; @@ -796,18 +789,19 @@ class Config { instance: new GathererClass(), implementation: gathererDefn.implementation, path: gathererDefn.path, - options: gathererDefn.options || {}, }; } else if (gathererDefn.path) { const path = gathererDefn.path; - const options = gathererDefn.options; - return Config.requireGathererFromPath(path, options, coreList, configDir); + return Config.requireGathererFromPath(path, coreList, configDir); } else { throw new Error('Invalid expanded Gatherer: ' + JSON.stringify(gathererDefn)); } }); - const mergedDefns = mergeOptionsOfItems(gathererDefns); + // De-dupe gatherers by artifact name. + const mergedDefns = Array.from( + new Map(gathererDefns.map(defn => [defn.instance.name, defn])).values() + ); mergedDefns.forEach(gatherer => assertValidGatherer(gatherer.instance, gatherer.path)); return Object.assign(pass, {gatherers: mergedDefns}); diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 0ef474e13ea2..78661df26195 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -382,8 +382,6 @@ class GatherRunner { for (const gathererDefn of passContext.passConfig.gatherers) { const gatherer = gathererDefn.instance; - // Abuse the passContext to pass through gatherer options - passContext.options = gathererDefn.options || {}; const status = { msg: `Gathering setup: ${gatherer.name}`, id: `lh:gather:beforePass:${gatherer.name}`, @@ -412,8 +410,6 @@ class GatherRunner { for (const gathererDefn of gatherers) { const gatherer = gathererDefn.instance; - // Abuse the passContext to pass through gatherer options - passContext.options = gathererDefn.options || {}; const status = { msg: `Gathering in-page: ${gatherer.name}`, id: `lh:gather:pass:${gatherer.name}`, @@ -457,8 +453,6 @@ class GatherRunner { }; log.time(status); - // Add gatherer options to the passContext. - passContext.options = gathererDefn.options || {}; const artifactPromise = Promise.resolve() .then(_ => gatherer.afterPass(passContext, loadData)); diff --git a/lighthouse-core/test/config/config-test.js b/lighthouse-core/test/config/config-test.js index ca023392f930..c70c2c58e6ed 100644 --- a/lighthouse-core/test/config/config-test.js +++ b/lighthouse-core/test/config/config-test.js @@ -64,6 +64,10 @@ describe('Config', () => { super(); this.secret = secretVal; } + + get name() { + return `MyGatherer${this.secret}`; + } } const myGatherer1 = new MyGatherer(1729); const myGatherer2 = new MyGatherer(6); @@ -1209,8 +1213,7 @@ describe('Config', () => { it('should merge gatherers', () => { const gatherers = [ 'viewport-dimensions', - {path: 'viewport-dimensions', options: {x: 1}}, - {path: 'viewport-dimensions', options: {y: 1}}, + {path: 'viewport-dimensions'}, ]; const merged = Config.requireGatherers([{gatherers}]); @@ -1218,7 +1221,7 @@ describe('Config', () => { const mergedJson = JSON.parse(JSON.stringify(merged)); assert.deepEqual(mergedJson[0].gatherers, - [{path: 'viewport-dimensions', options: {x: 1, y: 1}, instance: {}}]); + [{path: 'viewport-dimensions', instance: {}}]); }); function loadGatherer(gathererEntry) { @@ -1325,16 +1328,14 @@ describe('Config', () => { }); describe('#getPrintString', () => { - it('doesn\'t include empty gatherer/audit options in output', () => { - const gOpt = 'gathererOption'; + it('doesn\'t include empty audit options in output', () => { const aOpt = 'auditOption'; const configJson = { extends: 'lighthouse:default', passes: [{ passName: 'defaultPass', gatherers: [ - // `options` merged into default `script-elements` gatherer. - {path: 'script-elements', options: {gOpt}}, + {path: 'script-elements'}, ], }], audits: [ @@ -1347,20 +1348,9 @@ describe('Config', () => { const printedConfig = JSON.parse(printed); // Check that options weren't completely eliminated. - const scriptsGatherer = printedConfig.passes[0].gatherers - .find(g => g.path === 'script-elements'); - assert.strictEqual(scriptsGatherer.options.gOpt, gOpt); const metricsAudit = printedConfig.audits.find(a => a.path === 'metrics'); assert.strictEqual(metricsAudit.options.aOpt, aOpt); - for (const pass of printedConfig.passes) { - for (const gatherer of pass.gatherers) { - if (gatherer.options) { - assert.ok(Object.keys(gatherer.options).length > 0); - } - } - } - for (const audit of printedConfig.audits) { if (audit.options) { assert.ok(Object.keys(audit.options).length > 0); diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 0ce80254fcd9..4944823beba8 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -1464,64 +1464,6 @@ describe('GatherRunner', function() { }); }); - it('passes gatherer options', async () => { - /** @type {Record} */ - const calls = {beforePass: [], pass: [], afterPass: []}; - /** @param {string} name */ - const makeEavesdropGatherer = name => { - const C = class extends Gatherer {}; - Object.defineProperty(C, 'name', {value: name}); - return Object.assign(new C, { - /** @param {LH.Gatherer.PassContext} context */ - beforePass(context) { - calls.beforePass.push(context.options); - }, - /** @param {LH.Gatherer.PassContext} context */ - pass(context) { - calls.pass.push(context.options); - }, - /** @param {LH.Gatherer.PassContext} context */ - afterPass(context) { - calls.afterPass.push(context.options); - // @ts-expect-error - return context.options.x || 'none'; - }, - }); - }; - - const gatherers = [ - {instance: makeEavesdropGatherer('EavesdropGatherer1'), options: {x: 1}}, - {instance: makeEavesdropGatherer('EavesdropGatherer2'), options: {x: 2}}, - {instance: makeEavesdropGatherer('EavesdropGatherer3')}, - ]; - - const config = makeConfig({ - passes: [{ - passName: 'defaultPass', - gatherers, - }], - }); - - /** @type {any} Using Test-only gatherers. */ - const artifacts = await GatherRunner.run(config.passes, { - driver: fakeDriver, - requestedUrl: 'https://example.com', - settings: config.settings, - }); - - assert.equal(artifacts.EavesdropGatherer1, 1); - assert.equal(artifacts.EavesdropGatherer2, 2); - assert.equal(artifacts.EavesdropGatherer3, 'none'); - - // assert that all three phases received the gatherer options expected - const expectedOptions = [{x: 1}, {x: 2}, {}]; - for (let i = 0; i < 3; i++) { - assert.deepEqual(calls.beforePass[i], expectedOptions[i]); - assert.deepEqual(calls.pass[i], expectedOptions[i]); - assert.deepEqual(calls.afterPass[i], expectedOptions[i]); - } - }); - it('uses the last not-undefined phase result as artifact', async () => { const recoverableError = new Error('My recoverable error'); const someOtherError = new Error('Bad, bad error.'); diff --git a/types/config.d.ts b/types/config.d.ts index eaf83a61798d..6a519c301d88 100644 --- a/types/config.d.ts +++ b/types/config.d.ts @@ -90,7 +90,6 @@ declare global { implementation?: typeof Gatherer; instance: InstanceType; path?: string; - options: {}; } export interface AuditDefn { diff --git a/types/gatherer.d.ts b/types/gatherer.d.ts index c49f68f1bc18..69b3ae867c1a 100644 --- a/types/gatherer.d.ts +++ b/types/gatherer.d.ts @@ -39,7 +39,6 @@ declare global { disableJavaScript?: boolean; passConfig: Config.Pass settings: Config.Settings; - options?: object; /** Gatherers can push to this array to add top-level warnings to the LHR. */ LighthouseRunWarnings: Array; baseArtifacts: BaseArtifacts; From 860d5779cd9b650a201071f8875e05b4adae3993 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 1 Dec 2020 21:15:32 -0600 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Brendan Kenny --- lighthouse-core/config/config.js | 2 +- lighthouse-core/test/config/config-test.js | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js index e53931cd4255..60da58ffa3f6 100644 --- a/lighthouse-core/config/config.js +++ b/lighthouse-core/config/config.js @@ -798,7 +798,7 @@ class Config { } }); - // De-dupe gatherers by artifact name. + // De-dupe gatherers by artifact name because artifact IDs must be unique at runtime. const mergedDefns = Array.from( new Map(gathererDefns.map(defn => [defn.instance.name, defn])).values() ); diff --git a/lighthouse-core/test/config/config-test.js b/lighthouse-core/test/config/config-test.js index c70c2c58e6ed..eeb83a05bcc9 100644 --- a/lighthouse-core/test/config/config-test.js +++ b/lighthouse-core/test/config/config-test.js @@ -66,6 +66,7 @@ describe('Config', () => { } get name() { + // Use unique artifact name per instance so gatherers aren't deduplicated. return `MyGatherer${this.secret}`; } } @@ -1210,7 +1211,7 @@ describe('Config', () => { }); describe('#requireGatherers', () => { - it('should merge gatherers', () => { + it('should deduplicate gatherers', () => { const gatherers = [ 'viewport-dimensions', {path: 'viewport-dimensions'}, From c2d89e3a3a814a0018aefa105c3a1cf0dbae3568 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 1 Dec 2020 21:16:27 -0600 Subject: [PATCH 3/3] rename merge -> unique --- lighthouse-core/config/config.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js index 60da58ffa3f6..1190ecff58d4 100644 --- a/lighthouse-core/config/config.js +++ b/lighthouse-core/config/config.js @@ -799,12 +799,12 @@ class Config { }); // De-dupe gatherers by artifact name because artifact IDs must be unique at runtime. - const mergedDefns = Array.from( + const uniqueDefns = Array.from( new Map(gathererDefns.map(defn => [defn.instance.name, defn])).values() ); - mergedDefns.forEach(gatherer => assertValidGatherer(gatherer.instance, gatherer.path)); + uniqueDefns.forEach(gatherer => assertValidGatherer(gatherer.instance, gatherer.path)); - return Object.assign(pass, {gatherers: mergedDefns}); + return Object.assign(pass, {gatherers: uniqueDefns}); }); log.timeEnd(status); return fullPasses;