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(fr): more precise AnyFRInterface types #12622

Merged
merged 3 commits into from
Jun 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions lighthouse-core/fraggle-rock/config/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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<Symbol, LH.Config.ArtifactDefn>} artifactDefnsBySymbol
* @return {LH.Config.ArtifactDefn['dependencies']}
* @param {LH.Config.AnyFRGathererDefn} gatherer
* @param {Map<Symbol, LH.Config.AnyArtifactDefn>} artifactDefnsBySymbol
* @return {LH.Config.AnyArtifactDefn['dependencies']}
*/
function resolveArtifactDependencies(artifact, gatherer, artifactDefnsBySymbol) {
if (!('dependencies' in gatherer.instance.meta)) return undefined;
Expand All @@ -86,15 +86,15 @@ 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;

const status = {msg: 'Resolve artifact definitions', id: 'lh:config:resolveArtifactsToDefns'};
log.time(status, 'verbose');

/** @type {Map<Symbol, LH.Config.ArtifactDefn>} */
/** @type {Map<Symbol, LH.Config.AnyArtifactDefn>} */
const artifactDefnsBySymbol = new Map();

const coreGathererList = Runner.getGathererList();
Expand All @@ -108,7 +108,9 @@ function resolveArtifactsToDefns(artifacts, configDir) {
throw new Error(`${gatherer.instance.name} gatherer does not support Fraggle Rock`);
}

/** @type {LH.Config.ArtifactDefn<LH.Gatherer.DependencyKey>} */
/** @type {LH.Config.AnyArtifactDefn} */
// @ts-expect-error - Typescript can't validate the gatherer and dependencies match
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the only place I couldn't figure out how to solve.

🤓 🔫 @brendankenny if you're interested :)

the only thing I could think was to re-validate that resolveArtifactDependencies matches outside the function, but that seemed excessive

// even though it knows that they're each valid on their own.
const artifact = {
id: artifactJson.id,
gatherer,
Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/fraggle-rock/config/filters.js
Original file line number Diff line number Diff line change
Expand Up @@ -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<LH.Config.ArtifactDefn>} availableArtifacts
* @param {Array<LH.Config.AnyArtifactDefn>} availableArtifacts
* @return {LH.Config.FRConfig['audits']}
*/
function filterAuditsByAvailableArtifacts(audits, availableArtifacts) {
Expand Down
8 changes: 4 additions & 4 deletions lighthouse-core/fraggle-rock/config/validation.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/fraggle-rock/gather/runner-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
/**
* @typedef CollectPhaseArtifactOptions
* @property {import('./driver.js')} driver
* @property {Array<LH.Config.ArtifactDefn>} artifactDefinitions
* @property {Array<LH.Config.AnyArtifactDefn>} artifactDefinitions
* @property {ArtifactState} artifactState
* @property {LH.Gatherer.FRGatherPhase} phase
* @property {LH.Gatherer.GatherMode} gatherMode
Expand Down Expand Up @@ -99,7 +99,7 @@ async function collectPhaseArtifacts(options) {
}

/**
* @param {LH.Config.ArtifactDefn} artifact
* @param {LH.Config.AnyArtifactDefn} artifact
* @param {Record<string, LH.Gatherer.PhaseResult>} artifactsById
* @return {Promise<Dependencies>}
*/
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/test/fraggle-rock/gather/mock-driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ function createMockSession() {
}

/**
* @param {LH.Gatherer.FRGathererInstance<LH.Gatherer.DependencyKey>['meta']} meta
* @param {LH.Gatherer.AnyFRGathererInstance['meta']} meta
*/
function createMockGathererInstance(meta) {
return {
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ describe('NavigationRunner', () => {
/** @type {Map<string, LH.ArbitraryEqualityMap>} */
let computedCache;

/** @return {LH.Config.FRGathererDefn} */
/** @return {LH.Config.AnyFRGathererDefn} */
function createGathererDefn() {
return {
instance: {
Expand Down
21 changes: 17 additions & 4 deletions lighthouse-core/test/fraggle-rock/gather/runner-helpers-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, any>} */
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 = {
Expand Down Expand Up @@ -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({
Expand Down
18 changes: 16 additions & 2 deletions types/config.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, Category> | null;
Expand Down Expand Up @@ -160,7 +160,7 @@ declare global {
}

export interface NavigationDefn extends Omit<Required<NavigationJson>, 'artifacts'> {
artifacts: ArtifactDefn[];
artifacts: AnyArtifactDefn[];
}

export interface ArtifactDefn<TDependencies extends Gatherer.DependencyKey = Gatherer.DependencyKey> {
Expand All @@ -171,12 +171,26 @@ declare global {
Record<Exclude<TDependencies, Gatherer.DefaultDependenciesKey>, {id: string}>;
}

type ArtifactDefnExpander<TDependencies extends Gatherer.DependencyKey> =
// Lack of brackets intentional here to convert to the union of all individual dependencies.
TDependencies extends Gatherer.DefaultDependenciesKey ?
ArtifactDefn<Gatherer.DefaultDependenciesKey> :
ArtifactDefn<TDependencies>
export type AnyArtifactDefn = ArtifactDefnExpander<Gatherer.DefaultDependenciesKey>|ArtifactDefnExpander<Gatherer.DependencyKey>

export interface FRGathererDefn<TDependencies extends Gatherer.DependencyKey = Gatherer.DependencyKey> {
implementation?: ClassOf<Gatherer.FRGathererInstance<TDependencies>>;
instance: Gatherer.FRGathererInstance<TDependencies>;
path?: string;
}

type FRGathererDefnExpander<TDependencies extends Gatherer.DependencyKey> =
// Lack of brackets intentional here to convert to the union of all individual dependencies.
TDependencies extends Gatherer.DefaultDependenciesKey ?
FRGathererDefn<Gatherer.DefaultDependenciesKey> :
FRGathererDefn<TDependencies>
export type AnyFRGathererDefn = FRGathererDefnExpander<Gatherer.DefaultDependenciesKey>|FRGathererDefnExpander<Gatherer.DependencyKey>

patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
export interface GathererDefn {
implementation?: ClassOf<Gatherer.GathererInstance>;
instance: Gatherer.GathererInstance;
Expand Down
17 changes: 12 additions & 5 deletions types/gatherer.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ declare global {
}

interface GathererMetaWithDependencies<
TDependencies extends DependencyKey = DefaultDependenciesKey
TDependencies extends Exclude<DependencyKey, DefaultDependenciesKey>
> extends GathererMetaNoDependencies {
/**
* The set of required dependencies that this gatherer needs before it can compute its results.
Expand All @@ -96,9 +96,9 @@ declare global {
}

export type GathererMeta<TDependencies extends DependencyKey = DefaultDependenciesKey> =
TDependencies extends DefaultDependenciesKey ?
[TDependencies] extends [DefaultDependenciesKey] ?
GathererMetaNoDependencies :
GathererMetaWithDependencies<TDependencies>;
GathererMetaWithDependencies<Exclude<TDependencies, DefaultDependenciesKey>>;

export interface GathererInstance {
name: keyof LH.GathererArtifacts;
Expand All @@ -114,11 +114,18 @@ declare global {
meta: GathererMeta<TDependencies>;
startInstrumentation(context: FRTransitionalContext<DefaultDependenciesKey>): Promise<void>|void;
startSensitiveInstrumentation(context: FRTransitionalContext<DefaultDependenciesKey>): Promise<void>|void;
stopSensitiveInstrumentation(context: FRTransitionalContext<TDependencies>): Promise<void>|void;
stopInstrumentation(context: FRTransitionalContext<TDependencies>): Promise<void>|void;
stopSensitiveInstrumentation(context: FRTransitionalContext<DefaultDependenciesKey>): Promise<void>|void;
stopInstrumentation(context: FRTransitionalContext<DefaultDependenciesKey>): Promise<void>|void;
getArtifact(context: FRTransitionalContext<TDependencies>): PhaseResult;
}

type FRGathererInstanceExpander<TDependencies extends Gatherer.DependencyKey> =
// Lack of brackets intentional here to convert to the union of all individual dependencies.
TDependencies extends Gatherer.DefaultDependenciesKey ?
FRGathererInstance<Gatherer.DefaultDependenciesKey> :
FRGathererInstance<Exclude<TDependencies, DefaultDependenciesKey>>
export type AnyFRGathererInstance = FRGathererInstanceExpander<Gatherer.DependencyKey>

patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
namespace Simulation {
export type GraphNode = import('../lighthouse-core/lib/dependency-graph/base-node').Node;
export type GraphNetworkNode = _NetworkNode;
Expand Down