core: restructure types for direct import and publishing

[adamraine]

Proposal that helps us expose our types #1773

• Types are generated the same way as before, but now they can be rsynced into the normal directory structure with yarn build-types. This is done in prepack so should be in the release automatically.
• *.d.ts that just modify the global namespace or module declarations are moved to the types/env directory (i.e. types only we use).
• *.js will need to do import * as LH from '../types/lh.js' in order for their generated type declarations to be valid. (see core/index.js for example)
• Remove usages of global LH from core *.d.ts files
• Requires the .js extension when importing *.d.ts files in core
• Closes LH Puppeteer union types TypeScript error #14654

[adamraine]

For this to work, we shouldn't expose LH globally at least in core. This is a temporary measure if we decide to split into multiple PRs.

[connorjclark]

is there an update on this? More PRs to come?

[adamraine]

The changes in this PR should provide enough typings for the normal exposed api. We should still eliminate this global though.

Doing so would be done in a follow up, and probably wouldn't be a breaking change (although it would touch many files)

[brendankenny]

I had assumed we would solve #1773 with a limited set of types (our public interface), but since we're approaching year 6 for that issue and still no public API types, we should absolutely consider this approach :)

d.ts generation for all our js files already works great (that's how the incremental build and inter-project references work at all), so why not just use them? So this is great.

A few thoughts:

• separating Lighthouse types from global augmentation (and typing npm dependencies) generally seems like a really good idea, though there will likely be some questionable overlap depending on what someone imports from Lighthouse since we're exposing everything

  (edit: actually, maybe there won't be overlap. We'd never refer to a type like typeof Window['__nativeURL'], and even the very legitimate Intl.getCanonicalLocales is referenced in code, not in a type, so it should all be contained transitively. Great to separate types just for us vs types we want to export)

• moving types to (apparent) source code ts files seems confusing to me. We already have d.ts files for those types and don't need tsc to generate them, just to move them to the types directory. We could do that ourselves while keeping our type declarations as ambient d.ts files

• alternatively: we can leave them right in place and use the d.ts files next to their source js files approach and it should all just work. We can have tsc handle that, just need to remove the outDir line in tsconfig-base.json so declarations are written in place (or invoke tsc with --outDir ., though there are complications with --build).

• the import * as LH from '../types/lh.js' hack is a really clever way around the lack of jsdoc * imports, but is also pretty gross. Not sure I love it yet :) I wonder if we can get the ts team to finally implement a fix so at least we'd have a limited lifespan of those import LHs around. There's probably also a bunch of files that only need like one thing on LH and could do the direct typedef import instead of the giant namespace.

• you might need to flesh out more of this implementation to flush out some other issues with exporting everything. e.g. I'm pretty sure several of our devDependencies will need to become dependencies without some reworking. puppeteer is a big one with the current puppeteer.d.ts. I'm also not completely sure how it'll work with a types field in package.json since our main isn't currently in the project root. Maybe d.ts files will have to be side by side?

[adamraine]

alternatively: we can leave them right in place and use the d.ts files next to their source js files approach and it should all just work. We can have tsc handle that, just need to remove the outDir line in tsconfig-base.json so declarations are written in place (or invoke tsc with --outDir ., though there are complications with --build).

This idea sounds fine to me. Personally I like the filesystem organization where d.ts files are in a separate directory but it wouldn't be that hard to deal with d.ts alongside every .js file.

moving types to (apparent) source code ts files seems confusing to me. We already have d.ts files for those types and don't need tsc to generate them, just to move them to the types directory. We could do that ourselves while keeping our type declarations as ambient d.ts files

I think this depends on where we decide to put the generated d.ts files. If they are next to the .js files then we don't need to create a second copy because the path to our handwritten d.ts files will be the same for the generated d.ts and source .js files.

If the generated d.ts files are going in a separate directory, I want to avoid having a custom build step that moves our handwritten d.ts files to the new directory. Letting tsc handle this would be better IMO.

the import * as LH from '../types/lh.js' hack is a really clever way around the lack of jsdoc * imports, but is also pretty gross. Not sure I love it yet :) I wonder if we can get the ts team to finally implement a fix so at least we'd have a limited lifespan of those import LHs around. There's probably also a bunch of files that only need like one thing on LH and could do the direct typedef import instead of the giant namespace.

I agree it's not ideal. However the alternative is to painstakingly go through every file and import the required types manually. Adding an import * as LH from '...' to every js file is something we could automate and do once.

you might need to flesh out more of this implementation to flush out some other issues with exporting everything. e.g. I'm pretty sure several of our devDependencies will need to become dependencies without some reworking. puppeteer is a big one with the current puppeteer.d.ts. I'm also not completely sure how it'll work with a types field in package.json since our main isn't currently in the project root. Maybe d.ts files will have to be side by side?

This is a good point. I intended this to be a follow up but it's not a bad idea to look into it now.

[brendankenny]

This idea sounds fine to me. Personally I like the filesystem organization where d.ts files are in a separate directory but it wouldn't be that hard to deal with d.ts alongside every .js file.

Just in case we're speaking past each other, I meant as a pack/publish step. It's not useful to have them around otherwise.

If the generated d.ts files are going in a separate directory, I want to avoid having a custom build step that moves our handwritten d.ts files to the new directory. Letting tsc handle this would be better IMO.

Mixing ts and js files is pretty bad when the ts files aren't intended as source files, IMO, and makes the codebase less self evident. We're using d.ts files as the ambient declarations they already are, and it's good to have things enforced like no implementation allowed inside.

But I'm like 80% sure the question is moot because having more than one importable path with an explicit types is tricky and we'll probably have to do the in-place d.ts files anyways.

I agree it's not ideal. However the alternative is to painstakingly go through every file and import the required types manually. Adding an import * as LH from '...' to every js file is something we could automate and do once.

The best time to not use global variables is before you start, the second best time is now (or whenever they finally fix type module imports :) The LH global type continues to make things more complicated than they need to be. It shouldn't take convention to prevent dependencies from stomping all over each other; imports keep it clean by default and make any conflicts local.

The LH. is distinctive and it wouldn't be that hard to find and replace. Biggest thing is we shouldn't have to have e.g. separate typedefs for AuditDefn, Category, Config, Group, Json, Pass, PassJson, and Settings typedefs when we could just import Config as a module.

[adamraine]

Just in case we're speaking past each other, I meant as a pack/publish step. It's not useful to have them around otherwise.

Ah yup, that makes sense.

Mixing ts and js files is pretty bad when the ts files aren't intended as source files, IMO, and makes the codebase less self evident. We're using d.ts files as the ambient declarations they already are, and it's good to have things enforced like no implementation allowed inside.

I think I would support keeping our handwritten d.ts more or less as they are now. Then we generate the remaining d.ts files next to the corresponding .js files like you described as part of the pack/publish step.

The best time to not use global variables is before you start, the second best time is now (or whenever they finally fix type module imports :) The LH global type continues to make things more complicated than they need to be. It shouldn't take convention to prevent dependencies from stomping all over each other; imports keep it clean by default and make any conflicts local.

The LH. is distinctive and it wouldn't be that hard to find and replace. Biggest thing is we shouldn't have to have e.g. separate typedefs for AuditDefn, Category, Config, Group, Json, Pass, PassJson, and Settings typedefs when we could just import Config as a module.

Not sure I understand. Are you saying that import * as LH from '...' doesn't eliminate all complications with our current global LH approach? Would it be better to do individual module imports like import Config from '../../types/config'?

[connorjclark]

just better to do this earlier (failing sooner if types are bad), or was there some reason to move?

[adamraine]

yarn type-check is run as part of yarn build-all now, that would have made yarn type-check redundant and it would be confusing to receive type check errors as part of the build step.

Now we do the type check before building which fails sooner and caches types for the build step.

[connorjclark]

Didn't come across any problems when using the types generated by this PR. Nice work! This should be so much better for our Node users.

[connorjclark]

Maybe add a note to RunnerResult artifacts: Artifacts; that a subset of artifacts are considered unstable? We could mark those as @deprecated to get IDEs to call it out, but that seems like it could have undesirable effects with certain tooling.

[adamraine]

Maybe add a note to RunnerResult artifacts: Artifacts; that a subset of artifacts are considered unstable? We could mark those as @deprecated to get IDEs to call it out, but that seems like it could have undesirable effects with certain tooling.

If we don't want people to use them, maybe @internal could work? I'm good with just a comment though.

[brendankenny]

Didn't come across any problems when using the types generated by this PR. Nice work! This should be so much better for our Node users.

We're not quite there even for a basic user flow. Sprinkling in some more LH imports will get things working for most under "moduleResolution": "node", but if everyone is really as eager to use "moduleResolution": "NodeNext" as claim they are :P, there's still a couple of tricky parts to fix before it can be correctly imported

[connorjclark]

Maybe add a note to RunnerResult artifacts: Artifacts; that a subset of artifacts are considered unstable? We could mark those as @deprecated to get IDEs to call it out, but that seems like it could have undesirable effects with certain tooling.

If we don't want people to use them, maybe @internal could work? I'm good with just a comment though.

"@internal" is the right semantics. It'll at least show up as the first line of the jsdoc hover tooltip. A comment on the artifacts field is also good I think, let's do both?

We're not quite there even for a basic user flow. Sprinkling in some more LH imports will get things working for most under "moduleResolution": "node", but if everyone is really as eager to use "moduleResolution": "NodeNext" as claim they are :P, there's still a couple of tricky parts to fix before it can be correctly imported

I only verified IDE support looked good... which it does for user flows AFAICT. but yeah using tsc with nodenext and node16 is giving me various issues about file extensions missing:

Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../core/gather/fetcher.js'?

[adamraine]

(e2e test failure is unrelated)

The errors @connorjclark noticed were valid and have been fixed in ba3104f. I also added a recipe doc + test for a project using typescript and nodenext module resolution 324012a.

At this point, I think this PR is ready to merge.

[brendankenny]

Very exciting! 🎉

The test is a great addition. It's possible it will end up annoying detecting (additional) type errors in this roundabout way (does anyone run the docs tests except in CI?) but let's see if it's actually a problem and if there's more we can do to mitigate it (e.g. lint import paths in d.ts files or whatever)