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

Support .d.tss and make them visible in tests #95

Closed
wants to merge 5 commits into from

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Mar 25, 2022

If a project lacks types for one or more dependencies, it is customary
to fill in those types via ambient modules in the form of .d.ts
(type definition) files. Unfortunately this template cannot be used to
properly provide ambient modules.

  • First, because type definition files don't export anything, and ESLint
    is configured to read *.ts files as modules by default, ESLint will
    complain that a .d.ts file could be parsed as a script instead.
    That's an easy problem to solve.

  • Second, TypeScript's treatment of type definition files is not
    always intuitive, and we haven't fully understood how to use them. We
    usually work with type definition files in the context of packages, so
    the mechanics of how they work are kept out of sight. In that case,
    TypeScript will use the types field in package.json to know
    how to find the type definition files for a package.

    But what about backfilling types for existing packages (i.e.
    declaration of ambient modules)? We've been assuming that as long as a
    .d.ts file is matched by the include setting in tsconfig.json,
    that TypeScript will be able to see and use all of the types in that
    file. For instance, if you're importing a module foo in one .ts
    file, and you've backfilled the types for foo in another .d.ts
    file in the same directory, and both files are covered by include,
    then TypeScript should be able to bind types to the objects you're
    importing from foo, right? Not always. In fact, if you have a test
    file that is not covered by include that also imports the module
    foo, then TypeScript seems to ignore your type definition file and
    does not associate any types with foo.

  • Lastly, this problem illustrates that writing TypeScript and building
    TypeScript are two separate processes managed by two different tools
    (tsserver vs. tsc, respectively) which follow different rules and
    have different behavior. We need to acknowledge this in our
    configuration.

So, here's what this commit does to address these issues:

  • We instruct ESLint to treat *.d.ts files as scripts rather than
    modules.
  • We establish a types/ directory where our ambient modules will be
    stored, and then update tsconfig.json to automatically consult this
    directory when resolving types for imported modules. (Note that we do
    not use the typeRoots option for this as the tsconfig.json
    documentation might indicate
    ; this is not intended to be used for
    this purpose
    . Rather, we use paths.)
  • We update tsconfig.json to widen its purview for development so that
    test files have access to all of the types that non-test files do.
  • We add a special tsconfig.build.json file that is only used by yarn build which continues to ensure that only the files we want to
    publish (i.e. files in src/) are emitted to dist/.

Manual testing steps

  • Check out this branch.
  • Check out the commit titled "Set up a scenario that makes use of ambient modules". This commit adds a file and test that makes use of the is-callable module. This module is not typed natively, but types are filled in via src/dependencies.d.ts. This file should be getting included in tsconfig.json, but is somehow not visible to tests.
  • Open src/dependencies.d.ts and observe the new type. (Ignore the ESLint error; that will be addressed later.)
  • Open src/callMeMaybe.ts and hover over isCallable on the first line. Note the type you saw in src/dependencies.d.ts.
  • Open src/callMeMaybe.test.ts and hover over isCallable on line 2. Note the lack of type.
  • git checkout - to go back to the latest commit.
  • Now check out the commit titled "Add proof that this is working". This commit moves dependencies.d.ts to types/ and correctly registers the files therein as type files.
  • Open src/callMeMaybe.ts and hover over isCallable on the first line. You should see the same type as before.
  • Now open src/callMeMaybe.test.ts and hover over isCallable. You should now see the new type!

If a project lacks types for one or more dependencies, it is customary
to fill in those types via [ambient modules][1] in the form of `.d.ts`
(type definition) files. Unfortunately this template cannot be used to
properly provide ambient modules.

* First, because type definition files don't export anything, and ESLint
  is configured to read `*.ts` files as modules by default, ESLint will
  complain that a `.d.ts` file could be parsed as a script instead.
  That's an easy problem to solve.

* Second, TypeScript's treatment of type definition files is not
  always intuitive, and we haven't fully understood how to use them. We
  usually work with type definition files in the context of packages, so
  the mechanics of how they work are kept out of sight. In that case,
  [TypeScript will use the `types` field in `package.json`][2] to know
  how to find the type definition files for a package.

  But what about backfilling types for existing packages (i.e.
  declaration of ambient modules)? We've been assuming that as long as a
  `.d.ts` file is matched by the `include` setting in `tsconfig.json`,
  that TypeScript will be able to see and use all of the types in that
  file. For instance, if you're importing a module `foo` in one `.ts`
  file, and you've backfilled the types for `foo` in another `.d.ts`
  file in the same directory, and both files are covered by `include`,
  then TypeScript should be able to bind types to the objects you're
  importing from `foo`, right? Not always. In fact, if you have a test
  file that is *not* covered by `include` that also imports the module
  `foo`, then TypeScript seems to ignore your type definition file and
  does not associate any types with `foo`.

* Lastly, this problem illustrates that writing TypeScript and building
  TypeScript are two separate processes managed by two different tools
  (`tsserver` vs. `tsc`, respectively) which follow different rules and
  have different behavior. We need to acknowledge this in our
  configuration.

So, here's what this commit does to address these issues:

* We instruct ESLint to treat `*.d.ts` files as scripts rather than
  modules.
* We establish a `types/` directory where our ambient modules will be
  stored, and then update `tsconfig.json` to automatically consult this
  directory when resolving types for imported modules. (Note that we do
  *not* use the `typeRoots` option for this [as the `tsconfig.json`
  documentation might indicate][3]; this is [not intended to be used for
  this purpose][4]. Rather, we use [`paths`][5].)
* We update `tsconfig.json` to widen its purview for development so that
  test files have access to all of the types that non-test files do.
* We add a special `tsconfig.build.json` file that is only used by `yarn
  build` which continues to ensure that only the files we want to
  publish (i.e. files in `src/`) are emitted to `dist/`.

[1]: https://www.typescriptlang.org/docs/handbook/modules.html#ambient-modules
[2]: https://www.typescriptlang.org/docs/handbook/declaration-files/dts-from-js.html#editing-the-packagejson
[3]: https://www.typescriptlang.org/tsconfig#typeRoots
[4]: microsoft/TypeScript#22217
[5]: https://www.typescriptlang.org/docs/handbook/module-resolution.html#path-mapping
@Gudahtt
Copy link
Member

Gudahtt commented Mar 25, 2022

We establish a types/ directory where our ambient modules will be
stored, and then update tsconfig.json to automatically consult this
directory when resolving types for imported modules

This seems like a good idea to me. Nice and easy to read and maintain. But I don't follow how it solves any of the described problems. Is there a connection that I am missing, or was this just a nicer way to organize things?

@mcmire
Copy link
Contributor Author

mcmire commented Mar 25, 2022

This seems like a good idea to me. Nice and easy to read and maintain. But I don't follow how it solves any of the described problems. Is there a connection that I am missing, or was this just a nicer way to organize things?

I feel that this approach is less magical, in that it fits more closely with how TypeScript resolves modules (and type information for modules).

From I was able to gather — and granted, this is not spelled out 100% in the TS docs, so I may be making some assumptions here — when TypeScript resolves a NPM module (called a non-relative module in the docs) it can derive type information for that module various ways. It'll first try a litany of possible files in node_modules based on the name of the module being imported. If it can't do that, it will fall back to ambient types (or ambient declarations). These are never fully defined in the TS docs — although there is an example here — but I interpret them as types that are separated from their implementation, housed in a .d.ts file which is not a module (has no imports/exports) and which lives within a codebase that depends on said implementation. Thus, dependencies.d.ts would be an example of a file that provides ambient types because it does not export anything per se — it merely redeclares modules that are already imported via node_modules, only with added type information.

Even if TypeScript can make use of ambient types, it is unclear to me how it would automatically load the type declaration files that hold them in the first place. The most bulletproof way to do this, as far as I've seen, is to use a triple-slash directive, as in the ambient modules example in the TS docs. However, ESLint won't allow us to use them by default, and claims that imports are better. You can't import .d.ts files, though — that's not how module resolution in TypeScript works.

How module resolution does work, however — or, at least, seems to work — is that there is a list of locations TypeScript will search to find files (including type definition files). By default, it uses node_modules, but you can reconfigure TypeScript to insert some extra paths that it will try first. Again, there's not a clear explanation, but this is the best thing I could find.

The result of the changes in this PR is that we are essentially replacing ambient types as defined in dependencies.d.ts with "direct" types as defined under types/. So, when we import a module is-callable from ./src/callMeMaybe.ts, whereas before our change TypeScript might have tried (note: I am simplifying slightly here due to the sheer amount of possibilities):

  • ./node_modules/is-callable.ts
  • ./node_modules/is-callable.tsx
  • ./node_modules/is-callable.d.ts
  • ./node_modules/is-callable/package.json -> value of types
  • ./node_modules/@types/is-callable.d.ts
  • ./node_modules/is-callable/index.ts
  • ./node_modules/is-callable/index.tsx
  • ./node_modules/is-callable/index.d.ts
  • ./node_modules/is-callable.js
  • ./node_modules/is-callable/package.json -> value of main
  • ./node_modules/is-callable/index.js

it will now try:

  • ./src/types/is-callable.ts
  • ./src/types/is-callable.tsx
  • ./src/types/is-callable.d.ts
  • ./src/types/is-callable/package.json -> value of types
  • ./src/types/@types/is-callable.d.ts
  • ./src/types/is-callable/index.ts
  • ./src/types/is-callable/index.tsx
  • ./src/types/is-callable/index.d.ts
  • ./src/types/is-callable.js
  • ./src/types/is-callable/package.json -> value of main
  • ./src/types/is-callable/index.js
  • ./node_modules/is-callable.ts
  • ./node_modules/is-callable.tsx
  • ./node_modules/is-callable.d.ts
  • ./node_modules/is-callable/package.json -> value of types
  • ./node_modules/@types/is-callable.d.ts
  • ./node_modules/is-callable/index.ts
  • ./node_modules/is-callable/index.tsx
  • ./node_modules/is-callable/index.d.ts
  • ./node_modules/is-callable.js
  • ./node_modules/is-callable/package.json -> value of main
  • ./node_modules/is-callable/index.js

I suspect (and now I am really guessing) that even if TypeScript finds a .d.ts file, it will continue searching until it finds a .ts/.tsx file. So the files it would land on in this case are probably typical:

  • ./src/types/is-callable.d.ts
  • ./node_modules/is-callable/package.json -> value of main field

Following this route — using paths in tsconfig.json to instruct TypeScript where our backfilled types are — necessitates that we split up dependencies.d.ts into separate files, so that TypeScript's module resolution logic can do its thing.

We don't have to do this — we could figure out a better way to get TypeScript to "see" dependencies.d.ts. I am just not sure that triple-slash directives are the right approach for ambient types.

@Gudahtt
Copy link
Member

Gudahtt commented Mar 29, 2022

Hmm. I see, I thought they would get loaded automatically just as regular type definitions. Hadn't realized the triple slash directive was required. Agreed that that seems ambiguous from the docs, and that we should probably avoid it if it's no too much trouble.

"outDir": "dist",
"rootDir": "src",
"paths": {
"*": ["./types/*"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These docs on path mapping seem to imply this would overwrite the compiler's default behavior for looking up type information. So I was a bit concerned that this wouldn't look in node_modules anymore, or follow the standard Node.js module resolution. But it looks like it still does that if it can't find types here. At least locally that's what I'm seeing.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mcmire
Copy link
Contributor Author

mcmire commented Mar 30, 2022

So, after testing this pattern in controllers, I've come to realize that this approach doesn't work. It turns out that if you use paths like in this PR, it will totally override how TypeScript resolves type definitions for packages. This means that if a package already provides type definitions, then those will be ignored, if you also provide type definitions. Therefore it isn't possible to use module augmentation to add more types to a package or extend existing types in a package.

I came across this article (scroll down to "Ambient declarations") which seems to indicate that there are a few different ways to get TypeScript to include a .d.ts file in a build. As I've mentioned before, TypeScript will either pick them up automatically, but you can also do it manually. The most manual option is to use a triple-slash directive. However, since we don't want to do that, we need another way. Well... it turns out you were right before. The files or include option in tsconfig.json can be used, after all, to insert new files into the build. This includes type definition files. I just tested this approach in controllers and it worked fine:

"include": ["./types/**/*.d.ts", "./src/**/*.ts"]

That means that the original way we were doing things in this repo was sufficient.

@mcmire mcmire closed this Mar 30, 2022
@mcmire mcmire deleted the extract-build-only-tsconfig branch April 26, 2022 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants