core(tsc): add initial trivial type info to config.js #5481
Conversation
| declare global { | ||
| module LH { | ||
| /** | ||
| * The full, normalized Lighthouse Config. | ||
| */ | ||
| export interface Config { | ||
| export interface Config extends Config.Json { |
There was a problem hiding this comment.
this will give the very nice property that a Config is a valid Config.Json. Will allow easy serialization, --print-config or whatever, and means you don't have to keep track of what you have, a real config or a configJson. Either way, just pass in to the Config constructor (followup will ensure that a Config passed into new Config() will end up unchanged).
| passes: Config.Pass[] | null; | ||
| audits: Config.AuditDefn[] | null; | ||
| categories: Record<string, Config.Category> | null; | ||
| groups: Record<string, Config.Group> | null; |
There was a problem hiding this comment.
we had a mixture of optional and null used when one of these wasn't provided (e.g. don't need audits when in gatherMode). Went with the null as otherwise we'd need to be deleting things when e.g. everything ends up filtered out or whatever
| @@ -107,6 +117,13 @@ declare global { | |||
| auditRefs: AuditRef[]; | |||
| } | |||
| export interface Group extends GroupJson {} | |||
|
|
|||
| export type MergeOptionsOfItems = <T extends {path?: string, options: Record<string, any>}>(items: T[]) => T[]; | |||
There was a problem hiding this comment.
good news is with the latest tsc changes, these will be able to live in jsdocs soon
| } | ||
|
|
||
| module Config { | ||
| /** | ||
| * The pre-normalization Lighthouse Config format. | ||
| */ | ||
| export interface Json { | ||
| extends?: 'lighthouse:default' | 'lighthouse:full' | string | boolean; |
There was a problem hiding this comment.
what's the benefit of doing 'lighthouse:default' | string ?
There was a problem hiding this comment.
what's the benefit of doing
'lighthouse:default' | string?
no benefit, just documentation. I can remove or it might not matter :) (originally it was just the specific strings, but it gets automatically widened to string when requiring json files, so default-config et al were failing the type check)
| if (typeof item === 'object') { | ||
| // Return copy of instance and prototype chain (in case item is instantiated class). | ||
| return Object.assign( | ||
| Object.create( |
There was a problem hiding this comment.
did we need this? if we do, add a test for it?
There was a problem hiding this comment.
did we need this? if we do, add a test for it?
we need it for supporting live gatherer instances or {instance: liveGathererIntance, ...}, which seems reasonable if we support live implementations.
The reason I want it most is so Config is derived from Config.Json, but another approach to do that would be for shorthand expansion to drop {instance: ...}...we'd just need to make sure there was a path or implementation in that case.
I'll add a test but let me know if you'd rather just not support that
There was a problem hiding this comment.
that makes sense seems reasonable! just test case then :)
|
|
||
| // Copy arrays that could contain plugins to allow for programmatic | ||
| // injection of plugins. | ||
| if (Array.isArray(json.passes)) { | ||
| cloned.passes.forEach((pass, i) => { | ||
| if (Array.isArray(cloned.passes) && Array.isArray(json.passes)) { |
There was a problem hiding this comment.
could this ever not be true if one of them is true? just for TS?
There was a problem hiding this comment.
just for TS?
just for TS and pretty annoying :)
Part 1 of 2 to add type checking to
config.jsThis is an attempt to reduce noise around the more major changes by pulling out the trivial type annotations/changes needed without changing any functionality. Some of these types are slightly white lies, but will be correct with part 2.
Main trickiness here is some of the generic functions, like
merge()anddeepClone(), but they maintain their earlier behavior (exceptdeepCloneConfigJson()is split out), it's mostly just convincing the compiler that they do what they say they do.