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

type issues with d.ts files #12860

Closed
brendankenny opened this issue Aug 4, 2021 · 0 comments · Fixed by #12888
Closed

type issues with d.ts files #12860

brendankenny opened this issue Aug 4, 2021 · 0 comments · Fixed by #12888
Assignees

Comments

@brendankenny
Copy link
Member

brendankenny commented Aug 4, 2021

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.ts files 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 LH type 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants