-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Description
Issue
We have a couple of weird things going on with our types right now, and while trying to play with some of the report types, other weird things start cropping up.
As an example: the ArbitraryEqualityMap in audit.d.ts's computedCache: Map<string, ArbitraryEqualityMap> is not resolving to the ArbitraryEqualityMap import just 13 lines above, it's resolving to LH.ArbitraryEqualityMap defined over in artifacts.d.ts.
This isn't the worst, at least it's resolving to the correct thing and we could just remove the import in audit.d.ts, but something similar is happening when I mess with the LHR types just a little bit (I haven't found a minimal repro). In this case references to Audit in config.d.ts—which is supposed to be a reference to the real Audit class—start resolving to the module/namespace LH.Audit, which causes a bunch of type errors because we access audit implementations via the config and the LH.Audit namespace isn't an object and doesn't have properties like an Audit does :)
Again we could change the import name to AuditClass or whatever to work around, but I think this is indicative of the growing pains we're finally hitting from relying on global types and declaration merging across our d.ts files. This is also the cause of some of the growing pains across the report/viewer/treemap @connorjclark has been running into and has had to work around.
Proposal
In an ideal world we could maybe move to importing only the types we need, the same as importing any dependency, but jsdoc import types still make that annoying (microsoft/TypeScript#41825).
Instead I suggest
- we still move our
.d.tsfiles to be real declaration modules with explicit exports of their types and imports of their dependencies without modifying the global scope - expose the types in the global
LHtype namespace in a single place, making sure they still work exactly as they do now
This should make following/resolving/fixing type dependencies much easier as things aren't getting magically merged from multiple files by tsc, and it should make type isolation quite a bit easier, so if/when we do want to start tackling #1773, we could publish e.g. the LHR types without having to publish the entire LH.* world (this would still take work to unwind the cross dependencies, but I believe this is still a necessary step to start doing that). This would also make it easier for e.g. viewer and treemap to not import all of lighthouse-core types (and vice versa), which is what got me looking at this in the first place :)
I have this mostly working, so I think it should all be doable, but wanted to get a temperature check before finishing and opening a PR.