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

NodeNext module resolver thinks we're importing a module, while Node requires a CommonJS file #60663

Closed
jthemphill opened this issue Dec 3, 2024 · 9 comments

Comments

@jthemphill
Copy link

🔎 Search Terms

module resolver nodenext node16 commonjs module dual package error require import

Here are some similar issues:

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about module resolution

⏯ Playground Link

https://github.com/jthemphill/ts-cjs-repro

💻 Code

The "lib" package in my repo is modeled after the popular project https://github.com/yjs/yjs. Like yjs, lib uses Rollup to build CJS and ESM versions of the package, then uses TypeScript to emit a declaration file.

Its package.json has a conditional export:

"exports": {
  ".": {
    "types": "./dist/src/index.d.ts",
    "import": "./dist/index.mjs",
    "require": "./dist/index.cjs"
  }
},

This enables the package to be either imported or required, whether from ESM or CJS files.

The app package represents my project, which I am trying to move from {"module": "commonjs"} to the more correct {"module": "node16"}. Its index.ts file is two lines:

import * as dual from "lib";
console.log(dual.dualCjsMjs);

You can reproduce the problem with

cd packages/app
pnpm install && pnpm build

🙁 Actual behavior

node app/dist/index.js will run the code successfully. But TypeScript claims that the code will crash:

ts-cjs-repro/packages/app % pnpm build

> app@ build /Users/jhemphill/oss/ts-cjs-repro/packages/app
> tsc

index.ts:1:23 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("lib")' call instead.
  To convert this file to an ECMAScript module, change its file extension to '.mts', or add the field `"type": "module"` to '/Users/jhemphill/oss/ts-cjs-repro/packages/app/package.json'.

1 import * as dual from "lib";
                        ~~~~~


Found 1 error in index.ts:1

🙂 Expected behavior

TypeScript should not report an error here. Node is correctly importing index.cjs, and this file really does contain the values described in index.d.ts.

Additional information about the issue

I would like to migrate my team's project from the commonjs resolver to the more correct node16 resolver, but it is hard to sell teammates on this migration if the "more correct" resolver leads to a bunch of false positives until we also complete an ESM migration😕

I understand why the module resolver thinks that index.d.ts describes a module file, as the nearest package.json contains a "type": "module" field. This doesn't help me migrate my project, though.

@Andarist
Copy link
Contributor

Andarist commented Dec 3, 2024

I understand why the module resolver thinks that index.d.ts describes a module file, as the nearest package.json contains a "type": "module" field. This doesn't help me migrate my project, though.

It is how it's designed though and for good reasons. Your package.json#exports shape is not correct - the fact that it's modeled after a popular yjs package doesn't move the needle, that's not correct. Conceptually, if you ship 2 runtime files then you should also ship 2 type declaration files. This way your types can model your runtime semantics correctly. It's not a surprise stuff doesn't work as you expect if you try to "make a shortcut" by shipping a single declaration file.

@RyanCavanaugh
Copy link
Member

Yeah, yjs isn't doing this correctly. See e.g. https://arethetypeswrong.github.io/?p=y-websocket%402.0.4

@jthemphill
Copy link
Author

Thank you for the swift response! I am an application developer rather than a library author, and your response seems tailored to a library author. For what it's worth, I had already posted an issue to yjs's repo, and hopefully I represented your point of view well there. While I work on TS projects, I will help to push culture and build tools towards TypeScript's model of correctness.

But I am simply working on an application which uses libraries. If I change my compilerOptions from {"module": "commonjs"} to {"module": "node16"}, then TypeScript throws a false positive error at me every time I use a library with a 👺 warning. There are many, very popular, libraries with 👺 or 🎭 warnings, and the TypeScript errors do not accurately describe how Node will run. I believe TypeScript should accurately represent how Node will run, at least when TypeScript is using a node16 or nodenext resolver.

@RyanCavanaugh
Copy link
Member

This is a "garbage in, garbage out" situation. TypeScript can only accurately represent how Node will run if the packages give TypeScript accurate information about how they're configured.

@jthemphill
Copy link
Author

Okay, if you don't find this compelling then I'll close this out.

@jthemphill jthemphill closed this as not planned Won't fix, can't repro, duplicate, stale Dec 3, 2024
@dmonad
Copy link

dmonad commented Dec 3, 2024

Hi, I'm the author of Yjs, and several other open source projects that expose
typescript declaration files.

This whole topic is news to me. I can't keep up with the typescript development. It becomes harder and harder to stay up-to-date, so that typescript users can use my libraries. I believe your decision is too restrictive. Please keep in mind that it breaks a huge part of the typescript ecosystem.

CJS and MJS are different languages. I understand why they need to be declared
as such.

Can you please clarify why it is necessary to provide duplicate typescript
declaration files. Don't just tell me that I'm wrong. Please give me a reasonable answer that I can work with.

In my case, the declaration files for my mjs and cjs bundles are identical. Why does typescript need yet another declaration file? Is it just so they have a different extension name? It's not like typescript types supports cjs-only features. There is no d.cts language.

This is a "garbage in, garbage out" situation

You have all the information you need to deduct that the declaration file refers to a commonjs module. No garbage on my side.

I care about bundle size, so I would prefer not to duplicate the declaration files.

@RyanCavanaugh
Copy link
Member

https://www.typescriptlang.org/docs/handbook/modules/guides/choosing-compiler-options.html#notes-on-dual-emit-solutions covers some of this.

The problem is that, in a single compilation, you could have two inbound module resolutions (one CJS and one ESM) land at the same .d.ts file, at which point outbound resolutions from that file differ in their resolution mode too, which means that the same type at the same source location could refer to two completely separate unrelated entities. This isn't even architecturally supported, but if it were, it would lead to some extremely bad developer experiences: while it can be the case that your module presents identical shapes in CJS and ESM, this isn't necessarily true of its dependencies, and if anything ever goes wrong here (which it will) then you could end up with errors that basically say "Type Foo (from line 13 in bar.d.ts) is not assignable to Type Foo (from line 13 in bar.d.ts)".

@Andarist
Copy link
Contributor

Andarist commented Dec 3, 2024

To expand on the Ryan's last note - the structure of your runtime code creates a so-called dual package hazard. It might not be apparent immediately but types are prone to it too. Even though TypeScript is a structural type system at heart it supports some nominal typing.

If your library exposes some class then:

const { Cls } = require('lib')

const foo = new Cls()

if (foo instanceof (await import('lib')).Cls) {
  // this won't ever be true with your setup
}

You need to consider the same situation at the type level. This is essentially what "Type Foo (from line 13 in bar.d.ts) is not assignable to Type Foo (from line 13 in bar.d.ts)" means once you start considering classes with private members (compared nominally). Or unique symbols instantiated in both "copies" of your library.

@dmonad
Copy link

dmonad commented Dec 3, 2024

Thanks for both of your explanations.

So it's mostly about the file-names in error descriptions. Personally, I find this acceptable. You could still publish your recommendations to avoid this scenario. But I understand your point.

I imagine that almost all projects emit identical declaration files for cjs and mjs files. So I hoped there would be a better solution than duplicating declarations. Maybe I should remove the cjs bundle altogether.

If typescript declarations support nominal typings, then it makes sense to have duplicative declaration files.

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

No branches or pull requests

4 participants