core: extract independent LHR types#12914
Conversation
brendankenny
requested review from
patrickhulce
and removed request for
a team
| const [filename, key] = icuMessageId.split(' | '); | ||
| if (!filename.endsWith('util.js')) throw new Error(`Unexpected message: ${icuMessageId}`); | ||
|
|
||
| const key = /** @type {keyof LH.I18NRendererStrings} */ (varName); |
There was a problem hiding this comment.
this code doesn't actually verify that's what these strings are (it's just looking for any i18n strings starting with 'report/'), and there's really no reason for lighthouse-core/ to keep track of that anyways. It is verified in a report test, so we just move the type cast over there (reuse an existing one, actually) so that the report Util.i18n.strings.* usages are still type checked
| "timing-trace": "node lighthouse-core/scripts/generate-timing-trace.js", | ||
| "changelog": "conventional-changelog --config ./build/changelog-generator/index.js --infile changelog.md --same-file", | ||
| "type-check": "tsc -p . && tsc -p lighthouse-viewer/ && tsc -p lighthouse-treemap/", | ||
| "type-check": "tsc -b . && tsc -p lighthouse-viewer/ && tsc -p lighthouse-treemap/", |
| "diagnostics": true | ||
| "diagnostics": true, | ||
| "composite": true, | ||
| "outDir": ".tmp/tsbuildinfo/", |
There was a problem hiding this comment.
build mode outputs tsconfig.tsbuildinfo files so that project references can be faster and re-use already built projects (if files aren't newer than the tsbuildinfo file). We're still not emitting, but it uses this path for the tsbuildinfo files so they're not in the way. It's ok if .tmp is deleted at any time, it'll just recreate them on the next build
| "outDir": ".tmp/tsbuildinfo/", | ||
| }, | ||
| "include": [ | ||
| "root.js", |
There was a problem hiding this comment.
all the files added here were already in the compile through transitive import/require references, composite projects are just required to have all included files to actually be declared in include or files
| median: number; | ||
| } | ||
|
|
||
| interface ScoreDisplayModes { |
There was a problem hiding this comment.
these were moved to a new audit-result.d.ts file
| } | ||
| } | ||
|
|
||
| /** Simulation settings that control the amount of network & cpu throttling in the run. */ |
There was a problem hiding this comment.
moved to new settings.d.ts file
| /** The unit of `numericValue`, used when the consumer wishes to convert numericValue to a display string. */ | ||
| numericUnit?: string; | ||
| /** 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?: FormattedIcu<AuditDetails>; |
There was a problem hiding this comment.
This is definitely a wart, removing the IcuMessage from the AuditDetails types. The LHR shouldn't know anything about IcuMessage, because there aren't any by the time it's an LHR.
I haven't found a way to cleanly define AuditDetails without the IcuMessages as part of the LHR and then add them for internal usage (kind of like AuditProduct vs AuditResult) without needing a ton of subsequent changes, though.
Something to definitely fix before thinking about publishing any types, at least :)
4 participants
artifacts.d.tsandgatherer.d.ts, etctypes/lhr/*.d.tsa project reference to the rest of the code. The rest of lighthouse can still reference those files normally, but it's kind of an opaque barrier, similar to referencing a library in a node module. Code inside oftypes/lhr/, however, mostly can't reference code outside (mostly, but close enough for us), which means it should be easy to keep the LHR types standaloneSettingshave moved totypes/lhr/settings.d.tsand then referenced fromconfig.d.ts.There's a few moving pieces here, so I can try to split up if that's easier to review. This PR at least shows the point of the moves.
Next: the report/viewer/treemap can build on just the LHR and not the entirety of
lighthouse-core/, making the report considerably simpler to deal with on its own.