From e3c2e8cecac1286c3587341a09673f99f108e727 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Fri, 4 Jun 2021 14:40:01 -0500 Subject: [PATCH 1/3] core(fr): more precise AnyFRInterface types --- lighthouse-core/fraggle-rock/config/config.js | 16 +++++++------- .../fraggle-rock/config/filters.js | 2 +- .../fraggle-rock/config/validation.js | 8 +++---- .../fraggle-rock/gather/runner-helpers.js | 4 ++-- .../test/fraggle-rock/gather/mock-driver.js | 4 ++-- .../gather/navigation-runner-test.js | 2 +- .../gather/runner-helpers-test.js | 21 +++++++++++++++---- types/config.d.ts | 19 +++++++++++++++-- types/gatherer.d.ts | 18 +++++++++++----- 9 files changed, 66 insertions(+), 28 deletions(-) diff --git a/lighthouse-core/fraggle-rock/config/config.js b/lighthouse-core/fraggle-rock/config/config.js index feabb311d0d8..db07e6a28891 100644 --- a/lighthouse-core/fraggle-rock/config/config.js +++ b/lighthouse-core/fraggle-rock/config/config.js @@ -57,9 +57,9 @@ function resolveWorkingCopy(configJSON, context) { * Looks up the required artifact IDs for each dependency, throwing if no earlier artifact satisfies the dependency. * * @param {LH.Config.ArtifactJson} artifact - * @param {LH.Config.FRGathererDefn} gatherer - * @param {Map} artifactDefnsBySymbol - * @return {LH.Config.ArtifactDefn['dependencies']} + * @param {LH.Config.AnyFRGathererDefn} gatherer + * @param {Map} artifactDefnsBySymbol + * @return {LH.Config.AnyArtifactDefn['dependencies']} */ function resolveArtifactDependencies(artifact, gatherer, artifactDefnsBySymbol) { if (!('dependencies' in gatherer.instance.meta)) return undefined; @@ -86,7 +86,7 @@ function resolveArtifactDependencies(artifact, gatherer, artifactDefnsBySymbol) * * @param {LH.Config.ArtifactJson[]|null|undefined} artifacts * @param {string|undefined} configDir - * @return {LH.Config.ArtifactDefn[] | null} + * @return {LH.Config.AnyArtifactDefn[] | null} */ function resolveArtifactsToDefns(artifacts, configDir) { if (!artifacts) return null; @@ -94,7 +94,7 @@ function resolveArtifactsToDefns(artifacts, configDir) { const status = {msg: 'Resolve artifact definitions', id: 'lh:config:resolveArtifactsToDefns'}; log.time(status, 'verbose'); - /** @type {Map} */ + /** @type {Map} */ const artifactDefnsBySymbol = new Map(); const coreGathererList = Runner.getGathererList(); @@ -108,7 +108,9 @@ function resolveArtifactsToDefns(artifacts, configDir) { throw new Error(`${gatherer.instance.name} gatherer does not support Fraggle Rock`); } - /** @type {LH.Config.ArtifactDefn} */ + /** @type {LH.Config.AnyArtifactDefn} */ + // @ts-expect-error - Typescript can't validate the gatherer and dependencies match + // even though it knows that they're each valid on their own. const artifact = { id: artifactJson.id, gatherer, @@ -127,7 +129,7 @@ function resolveArtifactsToDefns(artifacts, configDir) { /** * * @param {LH.Config.NavigationJson[]|null|undefined} navigations - * @param {LH.Config.ArtifactDefn[]|null|undefined} artifactDefns + * @param {LH.Config.AnyArtifactDefn[]|null|undefined} artifactDefns * @return {LH.Config.NavigationDefn[] | null} */ function resolveNavigationsToDefns(navigations, artifactDefns) { diff --git a/lighthouse-core/fraggle-rock/config/filters.js b/lighthouse-core/fraggle-rock/config/filters.js index 7e88699ddf87..40d9c1b9da44 100644 --- a/lighthouse-core/fraggle-rock/config/filters.js +++ b/lighthouse-core/fraggle-rock/config/filters.js @@ -38,7 +38,7 @@ function filterArtifactsByGatherMode(artifacts, mode) { * Filters an array of audits down to the set that can be computed using only the specified artifacts. * * @param {LH.Config.FRConfig['audits']} audits - * @param {Array} availableArtifacts + * @param {Array} availableArtifacts * @return {LH.Config.FRConfig['audits']} */ function filterAuditsByAvailableArtifacts(audits, availableArtifacts) { diff --git a/lighthouse-core/fraggle-rock/config/validation.js b/lighthouse-core/fraggle-rock/config/validation.js index 91b4e4fdc8fa..5d284eda62ee 100644 --- a/lighthouse-core/fraggle-rock/config/validation.js +++ b/lighthouse-core/fraggle-rock/config/validation.js @@ -6,8 +6,8 @@ 'use strict'; /** - * @param {LH.Config.GathererDefn | LH.Config.FRGathererDefn} gathererDefn - * @return {gathererDefn is LH.Config.FRGathererDefn} + * @param {LH.Config.GathererDefn | LH.Config.AnyFRGathererDefn} gathererDefn + * @return {gathererDefn is LH.Config.AnyFRGathererDefn} */ function isFRGathererDefn(gathererDefn) { return 'meta' in gathererDefn.instance; @@ -17,8 +17,8 @@ function isFRGathererDefn(gathererDefn) { * Determines if the artifact dependency direction is valid. The dependency's minimum supported mode * must be less than or equal to the dependent's. * - * @param {LH.Config.FRGathererDefn} dependent The artifact that depends on the other. - * @param {LH.Config.FRGathererDefn} dependency The artifact that is being depended on by the other. + * @param {LH.Config.AnyFRGathererDefn} dependent The artifact that depends on the other. + * @param {LH.Config.AnyFRGathererDefn} dependency The artifact that is being depended on by the other. * @return {boolean} */ function isValidArtifactDependency(dependent, dependency) { diff --git a/lighthouse-core/fraggle-rock/gather/runner-helpers.js b/lighthouse-core/fraggle-rock/gather/runner-helpers.js index 07fd927c0b6b..2f5986dad89c 100644 --- a/lighthouse-core/fraggle-rock/gather/runner-helpers.js +++ b/lighthouse-core/fraggle-rock/gather/runner-helpers.js @@ -8,7 +8,7 @@ /** * @typedef CollectPhaseArtifactOptions * @property {import('./driver.js')} driver - * @property {Array} artifactDefinitions + * @property {Array} artifactDefinitions * @property {ArtifactState} artifactState * @property {LH.Gatherer.FRGatherPhase} phase * @property {LH.Gatherer.GatherMode} gatherMode @@ -99,7 +99,7 @@ async function collectPhaseArtifacts(options) { } /** - * @param {LH.Config.ArtifactDefn} artifact + * @param {LH.Config.AnyArtifactDefn} artifact * @param {Record} artifactsById * @return {Promise} */ diff --git a/lighthouse-core/test/fraggle-rock/gather/mock-driver.js b/lighthouse-core/test/fraggle-rock/gather/mock-driver.js index faa9aaf5138f..090b6141a943 100644 --- a/lighthouse-core/test/fraggle-rock/gather/mock-driver.js +++ b/lighthouse-core/test/fraggle-rock/gather/mock-driver.js @@ -40,7 +40,7 @@ function createMockSession() { } /** - * @param {LH.Gatherer.FRGathererInstance['meta']} meta + * @param {LH.Gatherer.FRGathererInstance['meta']|LH.Gatherer.FRGathererInstance['meta']} meta */ function createMockGathererInstance(meta) { return { @@ -51,7 +51,7 @@ function createMockGathererInstance(meta) { stopSensitiveInstrumentation: jest.fn(), getArtifact: jest.fn(), - /** @return {LH.Gatherer.FRGathererInstance} */ + /** @return {LH.Gatherer.AnyFRGathererInstance} */ asGatherer() { // @ts-expect-error - We'll rely on the tests passing to know this matches. return this; diff --git a/lighthouse-core/test/fraggle-rock/gather/navigation-runner-test.js b/lighthouse-core/test/fraggle-rock/gather/navigation-runner-test.js index 93002fb1dac7..4a4ef5f54faa 100644 --- a/lighthouse-core/test/fraggle-rock/gather/navigation-runner-test.js +++ b/lighthouse-core/test/fraggle-rock/gather/navigation-runner-test.js @@ -31,7 +31,7 @@ describe('NavigationRunner', () => { /** @type {Map} */ let computedCache; - /** @return {LH.Config.FRGathererDefn} */ + /** @return {LH.Config.AnyFRGathererDefn} */ function createGathererDefn() { return { instance: { diff --git a/lighthouse-core/test/fraggle-rock/gather/runner-helpers-test.js b/lighthouse-core/test/fraggle-rock/gather/runner-helpers-test.js index 3feaf1f38a98..ea14353d8edc 100644 --- a/lighthouse-core/test/fraggle-rock/gather/runner-helpers-test.js +++ b/lighthouse-core/test/fraggle-rock/gather/runner-helpers-test.js @@ -13,15 +13,19 @@ const {createMockDriver, createMockGathererInstance} = require('./mock-driver.js /* eslint-env jest */ describe('collectArtifactDependencies', () => { - /** @type {LH.Config.ArtifactDefn} */ + /** @type {LH.Config.AnyArtifactDefn} */ let artifact; /** @type {Record} */ let artifactStateById; beforeEach(() => { + class GathererWithDependency extends Gatherer { + meta = {...new Gatherer().meta, dependencies: {ImageElements: Symbol('')}}; + } + artifact = { id: 'Artifact', - gatherer: {instance: new Gatherer()}, + gatherer: {instance: new GathererWithDependency()}, dependencies: {ImageElements: {id: 'Dependency'}}, }; artifactStateById = { @@ -163,10 +167,19 @@ describe('collectPhaseArtifacts', () => { it('should pass dependencies to gatherers', async () => { const {artifactDefinitions: artifacts_, gatherers} = createGathererSet(); - const gatherer = artifacts_[1].gatherer; + const gatherer = + /** @type {{instance: LH.Gatherer.FRGathererInstance}} */ + (artifacts_[1].gatherer); + const imageElementsGatherer = + /** @type {{instance: LH.Gatherer.FRGathererInstance<'ImageElements'>}} */ + (artifacts_[1].gatherer); const artifactDefinitions = [ {id: 'Dependency', gatherer}, - {id: 'Snapshot', gatherer, dependencies: {ImageElements: {id: 'Dependency'}}}, + { + id: 'Snapshot', + gatherer: imageElementsGatherer, + dependencies: {ImageElements: {id: 'Dependency'}}, + }, ]; await helpers.collectPhaseArtifacts({ diff --git a/types/config.d.ts b/types/config.d.ts index 6814557f4a5f..cee98feb30a6 100644 --- a/types/config.d.ts +++ b/types/config.d.ts @@ -49,7 +49,7 @@ declare global { */ export interface FRConfig { settings: Settings; - artifacts: ArtifactDefn[] | null; + artifacts: AnyArtifactDefn[] | null; navigations: NavigationDefn[] | null; audits: AuditDefn[] | null; categories: Record | null; @@ -160,7 +160,7 @@ declare global { } export interface NavigationDefn extends Omit, 'artifacts'> { - artifacts: ArtifactDefn[]; + artifacts: AnyArtifactDefn[]; } export interface ArtifactDefn { @@ -171,12 +171,27 @@ declare global { Record, {id: string}>; } + type ArtifactDefnExpander = + // Lack of brackets intentional here to convert to the union of all individual dependencies. + TDependencies extends Gatherer.DefaultDependenciesKey ? + ArtifactDefn : + ArtifactDefn + export type AnyArtifactDefn = ArtifactDefnExpander|ArtifactDefnExpander + export interface FRGathererDefn { implementation?: ClassOf>; instance: Gatherer.FRGathererInstance; path?: string; } + type FRGathererDefnExpander = + // Lack of brackets intentional here to convert to the union of all individual dependencies. + TDependencies extends Gatherer.DefaultDependenciesKey ? + FRGathererDefn : + FRGathererDefn + export type AnyFRGathererDefn = FRGathererDefnExpander|FRGathererDefnExpander + + export interface GathererDefn { implementation?: ClassOf; instance: Gatherer.GathererInstance; diff --git a/types/gatherer.d.ts b/types/gatherer.d.ts index 382a49960737..ccdd1a0126a9 100644 --- a/types/gatherer.d.ts +++ b/types/gatherer.d.ts @@ -87,7 +87,7 @@ declare global { } interface GathererMetaWithDependencies< - TDependencies extends DependencyKey = DefaultDependenciesKey + TDependencies extends Exclude > extends GathererMetaNoDependencies { /** * The set of required dependencies that this gatherer needs before it can compute its results. @@ -96,9 +96,9 @@ declare global { } export type GathererMeta = - TDependencies extends DefaultDependenciesKey ? + [TDependencies] extends [DefaultDependenciesKey] ? GathererMetaNoDependencies : - GathererMetaWithDependencies; + GathererMetaWithDependencies>; export interface GathererInstance { name: keyof LH.GathererArtifacts; @@ -114,11 +114,19 @@ declare global { meta: GathererMeta; startInstrumentation(context: FRTransitionalContext): Promise|void; startSensitiveInstrumentation(context: FRTransitionalContext): Promise|void; - stopSensitiveInstrumentation(context: FRTransitionalContext): Promise|void; - stopInstrumentation(context: FRTransitionalContext): Promise|void; + stopSensitiveInstrumentation(context: FRTransitionalContext): Promise|void; + stopInstrumentation(context: FRTransitionalContext): Promise|void; getArtifact(context: FRTransitionalContext): PhaseResult; } + type FRGathererInstanceExpander = + // Lack of brackets intentional here to convert to the union of all individual dependencies. + TDependencies extends Gatherer.DefaultDependenciesKey ? + FRGathererInstance : + FRGathererInstance> + export type AnyFRGathererInstance = FRGathererInstanceExpander + + namespace Simulation { export type GraphNode = import('../lighthouse-core/lib/dependency-graph/base-node').Node; export type GraphNetworkNode = _NetworkNode; From 85920d39b44e1555e6576baf6a64aebedddab37e Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Fri, 4 Jun 2021 15:48:18 -0500 Subject: [PATCH 2/3] Update lighthouse-core/test/fraggle-rock/gather/mock-driver.js --- lighthouse-core/test/fraggle-rock/gather/mock-driver.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/test/fraggle-rock/gather/mock-driver.js b/lighthouse-core/test/fraggle-rock/gather/mock-driver.js index 090b6141a943..333f94535f70 100644 --- a/lighthouse-core/test/fraggle-rock/gather/mock-driver.js +++ b/lighthouse-core/test/fraggle-rock/gather/mock-driver.js @@ -40,7 +40,7 @@ function createMockSession() { } /** - * @param {LH.Gatherer.FRGathererInstance['meta']|LH.Gatherer.FRGathererInstance['meta']} meta + * @param {LH.Gatherer.AnyFRGathererInstance['meta']} meta */ function createMockGathererInstance(meta) { return { From e9889c6eb1343192519b5f54224b79d977728b51 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Fri, 4 Jun 2021 15:49:13 -0500 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Adam Raine <6752989+adamraine@users.noreply.github.com> --- types/config.d.ts | 1 - types/gatherer.d.ts | 1 - 2 files changed, 2 deletions(-) diff --git a/types/config.d.ts b/types/config.d.ts index cee98feb30a6..d5038979c947 100644 --- a/types/config.d.ts +++ b/types/config.d.ts @@ -191,7 +191,6 @@ declare global { FRGathererDefn export type AnyFRGathererDefn = FRGathererDefnExpander|FRGathererDefnExpander - export interface GathererDefn { implementation?: ClassOf; instance: Gatherer.GathererInstance; diff --git a/types/gatherer.d.ts b/types/gatherer.d.ts index ccdd1a0126a9..f6e3a316c7b9 100644 --- a/types/gatherer.d.ts +++ b/types/gatherer.d.ts @@ -126,7 +126,6 @@ declare global { FRGathererInstance> export type AnyFRGathererInstance = FRGathererInstanceExpander - namespace Simulation { export type GraphNode = import('../lighthouse-core/lib/dependency-graph/base-node').Node; export type GraphNetworkNode = _NetworkNode;