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

Fix: explicitly add ESM types #105

Merged

Conversation

termontwouter
Copy link
Contributor

Stumbled upon some unexpected behavior when having AsyncIterator as a (sub)depenency of a TypeScript project with configured to use the Node16, NodeNext or Bundler module resolution mechanisms. This probably did not turn up earlier because most packages in the ecosystem still compile to CommonJS modules, for which TypeScript defaults to the old Node module resolution mechanism.

The issue

TS7016: Could not find a declaration file for module 'asynciterator'. 
'[...]/node_modules/asynciterator/dist/asynciterator.mjs' implicitly has an 'any' type.
There are types at '[...]/node_modules/asynciterator/dist/asynciterator.d.ts', but this result could not be resolved when respecting package.json "exports". The 'asynciterator' library may need to update its package.json or typings.

When checked with AreTheTypesWrong, we get the following report.

❯ npx -p @arethetypeswrong/cli attw --from-npm asynciterator

asynciterator v3.8.1

Build tools:
- typescript@^3.9.5

❌ Import resolved to JavaScript files, but no type declarations were found. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/UntypedResolution.md

┌───────────────────┬──────────────────────────────┬─────────────────┐
│                   │ "asynciterator/package.json" │ "asynciterator" │
├───────────────────┼──────────────────────────────┼─────────────────┤
│ node10            │ 🟢 (JSON)                    │ 🟢              │
├───────────────────┼──────────────────────────────┼─────────────────┤
│ node16 (from CJS) │ 🟢 (JSON)                    │ 🟢 (CJS)        │
├───────────────────┼──────────────────────────────┼─────────────────┤
│ node16 (from ESM) │ 🟢 (JSON)                    │ ❌ No types     │
├───────────────────┼──────────────────────────────┼─────────────────┤
│ bundler           │ 🟢 (JSON)                    │ ❌ No types     │
└───────────────────┴──────────────────────────────┴─────────────────┘

The problem is explained in the linked page: "In newer TypeScript resolution modes, however, TypeScript understands that an import in Node will resolve to index.mjs, and so it looks for a declaration file named index.d.mts, which doesn’t exist." (For more info, also see the TypeScript Handbook.)

The solution

My first attempt at solving this (993c734) was to simply mention the same type declarations twice in the exports object of the manifest. While this solved my concrete compilation errors, it still left the following "warning" when running AreTheTypesWrong.

❯ npx -p @arethetypeswrong/cli attw --pack .                

asynciterator v3.8.1

Build tools:
- typescript@^3.9.5

🎭 Import resolved to a CommonJS type declaration file, but an ESM JavaScript file. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseCJS.md

┌───────────────────┬──────────────────────────────┬────────────────────────┐
│                   │ "asynciterator/package.json" │ "asynciterator"        │
├───────────────────┼──────────────────────────────┼────────────────────────┤
│ node10            │ 🟢 (JSON)                    │ 🟢                     │
├───────────────────┼──────────────────────────────┼────────────────────────┤
│ node16 (from CJS) │ 🟢 (JSON)                    │ 🟢 (CJS)               │
├───────────────────┼──────────────────────────────┼────────────────────────┤
│ node16 (from ESM) │ 🟢 (JSON)                    │ 🎭 Masquerading as CJS │
├───────────────────┼──────────────────────────────┼────────────────────────┤
│ bundler           │ 🟢 (JSON)                    │ 🟢                     │
└───────────────────┴──────────────────────────────┴────────────────────────┘

Again, this new issue is explained in the linked page: "Import resolved to an ESM type declaration file, but a CommonJS JavaScript file."

That page also lists more consequences than the error I encountered: "TypeScript will not allow consumers in CommonJS files to import/require the module without using await import("pkg"), even though that won’t actually be needed at runtime. In Node, an ES module cannot be accessed with require, so TypeScript will report this as an error at compile time. Note that in CommonJS TypeScript files (.cts, .ts with no package.json "type": "module"), top-level import declarations are emitted as require variable declarations (since CommonJS files cannot use import/export in Node), so this problem can occur even when the consumer is using import syntax."

So, as a second attempt (a5a7bc6) I actually compiled the type declarations files for both module types separately. This gives the desired result. Tests also still run fine, and on @rubensworks' request I tested actual imports in CommonJS and ECMAScript modules from Node.js v12.22.12 onward.

❯ npx -p @arethetypeswrong/cli attw --pack .                                                 

asynciterator v3.8.1

Build tools:
- typescript@^3.9.5

 No problems found 🌟

┌───────────────────┬──────────────────────────────┬─────────────────┐
│                   │ "asynciterator/package.json" │ "asynciterator" │
├───────────────────┼──────────────────────────────┼─────────────────┤
│ node10            │ 🟢 (JSON)                    │ 🟢              │
├───────────────────┼──────────────────────────────┼─────────────────┤
│ node16 (from CJS) │ 🟢 (JSON)                    │ 🟢 (CJS)        │
├───────────────────┼──────────────────────────────┼─────────────────┤
│ node16 (from ESM) │ 🟢 (JSON)                    │ 🟢 (ESM)        │
├───────────────────┼──────────────────────────────┼─────────────────┤
│ bundler           │ 🟢 (JSON)                    │ 🟢              │
└───────────────────┴──────────────────────────────┴─────────────────┘

@RubenVerborgh
Copy link
Owner

Can't believe we're 2024 and we still need to jump through all of these hoops just to get a CJS and ESM version. Thanks for taking this on!

I just notice a CI error, could you have a look?

@termontwouter
Copy link
Contributor Author

That's better :)

@RubenVerborgh
Copy link
Owner

This looks so painful, @termontwouter. Thanks for looking after this!

@RubenVerborgh RubenVerborgh merged commit e3cc846 into RubenVerborgh:main Mar 11, 2024
17 checks passed
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

3 participants