-
-
Notifications
You must be signed in to change notification settings - Fork 17
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.ts
s and make them visible in tests
#95
Conversation
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
This reverts commit 04e426b.
This reverts commit a57fa0c.
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 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 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 The result of the changes in this PR is that we are essentially replacing ambient types as defined in
it will now try:
I suspect (and now I am really guessing) that even if TypeScript finds a
Following this route — using We don't have to do this — we could figure out a better way to get TypeScript to "see" |
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/*"] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
So, after testing this pattern in 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 "include": ["./types/**/*.d.ts", "./src/**/*.ts"] That means that the original way we were doing things in this repo was sufficient. |
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 willcomplain 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 inpackage.json
to knowhow 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 theinclude
setting intsconfig.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 testfile that is not covered by
include
that also imports the modulefoo
, then TypeScript seems to ignore your type definition file anddoes 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 andhave different behavior. We need to acknowledge this in our
configuration.
So, here's what this commit does to address these issues:
*.d.ts
files as scripts rather thanmodules.
types/
directory where our ambient modules will bestored, and then update
tsconfig.json
to automatically consult thisdirectory when resolving types for imported modules. (Note that we do
not use the
typeRoots
option for this as thetsconfig.json
documentation might indicate; this is not intended to be used for
this purpose. Rather, we use
paths
.)tsconfig.json
to widen its purview for development so thattest files have access to all of the types that non-test files do.
tsconfig.build.json
file that is only used byyarn build
which continues to ensure that only the files we want topublish (i.e. files in
src/
) are emitted todist/
.Manual testing steps
is-callable
module. This module is not typed natively, but types are filled in viasrc/dependencies.d.ts
. This file should be getting included intsconfig.json
, but is somehow not visible to tests.src/dependencies.d.ts
and observe the new type. (Ignore the ESLint error; that will be addressed later.)src/callMeMaybe.ts
and hover overisCallable
on the first line. Note the type you saw insrc/dependencies.d.ts
.src/callMeMaybe.test.ts
and hover overisCallable
on line 2. Note the lack of type.git checkout -
to go back to the latest commit.dependencies.d.ts
totypes/
and correctly registers the files therein as type files.src/callMeMaybe.ts
and hover overisCallable
on the first line. You should see the same type as before.src/callMeMaybe.test.ts
and hover overisCallable
. You should now see the new type!