Improve support for backfilling types via `.d.ts`'s #732

mcmire · 2022-03-18T22:55:07Z

Currently, we exclude test files from TypeScript's purview, so that only
files in src/ get emitted to dist/. This makes sense from a release
perspective, but it interferes with development.

One example of this interference involves ambient modules. We have
a file dependencies.d.ts which we can use to backfill missing types
for various dependencies. The problem is that while TypeScript (or, more
accurately, tsserver) can see the types specified here when reading
source files, it doesn't seem to pick them up for test files. This
causes in-editor inconsistencies with type analysis and, in some cases,
warnings such as:

import can only be used with esInteropModuleFlag: true
X is missing type definitions

The reason this is happening is that type definition files are not
intended to just be placed in the source directory. You can either use
triple-slash directives to bring them in, or you can modify a setting in
tsconfig.json (paths) that will instruct TypeScript where it should
find types for modules that are imported. This commit chooses the
latter option. Note that this requires we split up dependencies.d.ts
into separate type definition files, one per module.

In addition to modifying tsconfig.json, this commit also breaks up
this config file into a global/development version and a version that is
specific to the build/release process. This allows editors to use
the same exact TypeScript settings for test files as for non-test files.
tsconfig.build.json is used by yarn build and will limit the files
emitted to dist/ to only non-test files in src/, as in the original
configuration.

Manual testing steps

Check out the main branch.
Open src/assets/AssetsContractController.test.ts.
Hover over MAINNET_PROVIDER on line 6. Note how the type is any. This is because the types for ethjs-provider-http is being stubbed in src/dependencies.d.ts.

Update the declare module 'ethjs-provider-http'; line in src/dependencies.d.ts to read:

declare module 'ethjs-provider-http' {
  import type { JsonRpcRequest } from 'json-rpc-engine';
  export type EthQueryMethodCallback<R> = (error: any, response: R) => void;

  export default class HttpProvider {
    host: string;
    timeout: number;

    constructor(host: string, timeout?: number);

    sendAsync<P, R>(
      request: JsonRpcRequest<P>,
      callback: EthQueryMethodCallback<R>,
    ): void;
  }
}

Hover over MAINNET_PROVIDER in src/assets/AssetsContractController.test.ts again. Note how the type is still any.
Remove your changes to src/dependencies.d.ts.
Now check out this branch.
Restore your changes to src/dependencies.d.ts.
Hover over MAINNET_PROVIDER in src/assets/AssetsContractController.test.ts a third time. Note how the type is properly HttpProvider.
Finally, run yarn build. All of the files we would expect to show up in dist/ should be there (i.e. no extra .d.ts files, no tests, etc.)

tsconfig.json

Gudahtt · 2022-03-24T16:25:56Z

Huh. I hadn't considered this before. We should make this update in our template repo as well.

tsconfig.json

Gudahtt

LGTM! I can't believe I hadn't noticed this before. Great PR description too, those repro steps were greatly appreciated

Currently, we exclude test files from TypeScript's purview, so that only files in `src/` get emitted to `dist/`. This makes sense from a release perspective, but it interferes with development. One example of this interference involves [ambient modules][1]. We have a file `dependencies.d.ts` which we can use to backfill missing types for various dependencies. The problem is that while TypeScript (or, more accurately, `tsserver`) can see the types specified here when reading source files, it doesn't seem to pick them up for test files. This causes in-editor inconsistencies with type analysis and, in some cases, warnings such as: * `import` can only be used with `esInteropModuleFlag: true` * `X` is missing type definitions The reason this is happening is that type definition files are not intended to just be placed in the source directory. You can either use triple-slash directives to bring them in, or you can modify a setting in `tsconfig.json` (`paths`) that will [instruct TypeScript where it should find types for modules that are imported][1]. This commit chooses the latter option. Note that this requires we split up `dependencies.d.ts` into separate type definition files, one per module. In addition to modifying `tsconfig.json`, this commit also breaks up this config file into a global/development version and a version that is specific to the build/release process. This allows editors to use the same exact TypeScript settings for test files as for non-test files. `tsconfig.build.json` is used by `yarn build` and will limit the files emitted to `dist/` to only non-test files in `src/`, as in the original configuration. [1]: https://www.typescriptlang.org/docs/handbook/modules.html#ambient-modules [2]: https://www.typescriptlang.org/docs/handbook/module-resolution.html#path-mapping

mcmire · 2022-03-25T19:57:56Z

Okay, I've created a similar PR in the template repo here: MetaMask/metamask-module-template#95. As I was doing this, I learned a few more things about type definition files, and updated this PR to match those findings. I've left some more commentary on that particular PR than here, so that might be informative if you're not sure why dependencies.d.ts got split up. One more review please 🙏🏻

Gudahtt

LGTM!

…o add-npm-publish * 'add-npm-publish' of github.com:MetaMask/controllers: Improve support for backfilling types via .d.ts's (#732) Copy .gitattributes from template repo (#760) 27.1.1 (#759) Move new keystone package to dependencies (#757)

Currently, we exclude test files from TypeScript's purview, so that only files in `src/` get emitted to `dist/`. This makes sense from a release perspective, but it interferes with development. One example of this interference involves [ambient modules][1]. We have a file `dependencies.d.ts` which we can use to backfill missing types for various dependencies. The problem is that while TypeScript (or, more accurately, `tsserver`) can see the types specified here when reading source files, it doesn't seem to pick them up for test files. This causes in-editor inconsistencies with type analysis and, in some cases, warnings such as: * `import` can only be used with `esInteropModuleFlag: true` * `X` is missing type definitions The reason this is happening is that type definition files are not intended to just be placed in the source directory. You can either use triple-slash directives to bring them in, or you can modify a setting in `tsconfig.json` (`paths`) that will [instruct TypeScript where it should find types for modules that are imported][1]. This commit chooses the latter option. Note that this requires we split up `dependencies.d.ts` into separate type definition files, one per module. In addition to modifying `tsconfig.json`, this commit also breaks up this config file into a global/development version and a version that is specific to the build/release process. This allows editors to use the same exact TypeScript settings for test files as for non-test files. `tsconfig.build.json` is used by `yarn build` and will limit the files emitted to `dist/` to only non-test files in `src/`, as in the original configuration. [1]: https://www.typescriptlang.org/docs/handbook/modules.html#ambient-modules [2]: https://www.typescriptlang.org/docs/handbook/module-resolution.html#path-mapping

mcmire requested a review from a team as a code owner March 18, 2022 22:55

mcmire changed the title ~~Create tests-only TypeScript configuration~~ Extract build-only TypeScript configuration Mar 18, 2022

mcmire force-pushed the fix-tsconfig-for-tests branch 3 times, most recently from 75afffa to e3a972e Compare March 18, 2022 23:13

mcmire mentioned this pull request Mar 21, 2022

Change root dir and don't exclude test files in tsconfig.json MetaMask/key-tree#47

Closed

mcmire commented Mar 21, 2022

View reviewed changes

tsconfig.json Show resolved Hide resolved

Mrtenz mentioned this pull request Mar 22, 2022

Extract build-only TypeScript configuration MetaMask/key-tree#48

Merged

Gudahtt reviewed Mar 24, 2022

View reviewed changes

tsconfig.json Outdated Show resolved Hide resolved

Gudahtt previously approved these changes Mar 24, 2022

View reviewed changes

mcmire dismissed Gudahtt’s stale review via 25ad0b9 March 24, 2022 21:07

mcmire force-pushed the fix-tsconfig-for-tests branch from f4f0802 to 25ad0b9 Compare March 24, 2022 21:07

Gudahtt previously approved these changes Mar 24, 2022

View reviewed changes

mcmire dismissed Gudahtt’s stale review via ecd3eea March 25, 2022 19:56

mcmire force-pushed the fix-tsconfig-for-tests branch from d130fde to ecd3eea Compare March 25, 2022 19:56

mcmire changed the title ~~Extract build-only TypeScript configuration~~ Improve support for backfilling types via .d.ts's Mar 25, 2022

Merge branch 'main' into fix-tsconfig-for-tests

8d4aa66

Gudahtt approved these changes Mar 29, 2022

View reviewed changes

Merge branch 'main' into fix-tsconfig-for-tests

50910c3

mcmire merged commit 6f3808b into main Mar 30, 2022

mcmire deleted the fix-tsconfig-for-tests branch March 30, 2022 20:06

mcmire mentioned this pull request Mar 30, 2022

Backfill types for some dependencies #733

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Improve support for backfilling types via `.d.ts`'s #732

Improve support for backfilling types via `.d.ts`'s #732

Uh oh!

mcmire commented Mar 18, 2022 •

edited

Loading

Uh oh!

Uh oh!

Gudahtt commented Mar 24, 2022

Uh oh!

Uh oh!

Gudahtt left a comment

Uh oh!

mcmire commented Mar 25, 2022 •

edited

Loading

Uh oh!

Gudahtt left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Improve support for backfilling types via .d.ts's #732

Improve support for backfilling types via .d.ts's #732

Uh oh!

Conversation

mcmire commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Manual testing steps

Uh oh!

Uh oh!

Gudahtt commented Mar 24, 2022

Uh oh!

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

mcmire commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Improve support for backfilling types via `.d.ts`'s #732

Improve support for backfilling types via `.d.ts`'s #732

mcmire commented Mar 18, 2022 •

edited

Loading

mcmire commented Mar 25, 2022 •

edited

Loading