Support for nodenext/node16 module resolution in TypeScript

[benasher44]

It looks like someone has started opening the PRs to support this in prosemirror-view and others. As folks with TypeScript repos migrate to ESM, switching to nodenext/node16 module resolution is the first step to migrating from CommonJS to ESM.

PRs (might not be exhaustive):

• feat: Add exports.types to package.json prosemirror-view#131
• feat: Add exports.types to package.json prosemirror-state#20
• feat: Add exports.types to package.json prosemirror-model#70

Without the necessary package.json pieces in place, you get errors like this when importing prosemirror packages in TypeScript repos using nodenext/node16 module resolution (and module still set to CommonJS):

Module 'prosemirror-state' cannot be imported using this construct. The specifier only resolves to an ES module, which cannot be imported synchronously. Use dynamic import instead.

[marijnh]

Can you show a minimal example project that'd lead to that error?

[benasher44]

After some more digging, I think the error is even more niche. In hindsight, I bumped into this building for ESM, not CommonJS. Here's a reproduction:

https://github.com/benasher44/prosemirror-nodenext

It seems to have something to do with adding the paths config to tsconfig.json. You can reproduce in that project by running npm install and then npm run build

[benasher44]

One more notes as I'm narrowing in on this: in my main project, I can reproduce this with the paths entry in tsconfig.json, but in the smaller repro project, I can only reproduce it by adding paths entry

[benasher44]

Hey I think this might be a TS issue, which I have filed here. I'll keep you posted!

[benasher44]

I'm still confused about this scenario, but I found this comment:

microsoft/TypeScript#46334 (comment)

Specifically this part:

In node16/nodenext, it is expected for the top-level types to be ignored in the presence of exports, because Node ignores the top-level main in the presence of exports. (The point of a targeted moduleResolution is to maintain a parallel logic like this as perfectly as we can, so there is parity between what works at compile time and what works at runtime.)

I think this is maybe where things are falling apart here. In this case, all packages only have a top-level types entry in the package.json, but it appears one needs to be added in exports as well.

[benasher44]

In other words, applying ProseMirror/prosemirror-view#131 to all prosemirror-* packages should fix it 🤞.

There's no response from TypeScript folks yet on the issue I filed, but I'm not optimistic that the fix won't be either of these things:

1. Fix so that this breakage applies regardless of noImplicitAny for consistency
2. Claim that prosemirror's package.json needs fixing 😬

[benasher44]

@marijnh TS maintainers confirmed that the root issue is the lack of a types entry. A types entry needs to be added under exports to work properly.

Clearly the package.json has exports field and its missing types which is the cause of this issue…

[benasher44]

Would it be possible to publish updates to prosemirror packages to fix?

[marijnh]

I still have not seen a minimal example producing this error, and if I try to set one up TypeScript doesn't complain and just uses the types from the top-level package field.

[benasher44]

What about the project I provided above?

[benasher44]

@marijnh I updated the sample project to be even more minimal. It now reproduces just importing prosemirror-view (doesn't go through tiptap). Issue seems to be when noImplicitAny is set to true, and we're building a CJS project with nodenext.

Latest comment in the TS issue covers the "why" about noImplicitAny

Not being able to resolve dependency typings is only an error in noImplicitAny. It doesn’t work fine without noImplicitAny; the typings aren’t found and the import is silently any.

[marijnh]

What about the project I provided above?

Which one? prosemirror-nodenext?

[benasher44]

Yep! That one

[marijnh]

Ah, what you appear to be doing differently from my tests is that you don't have "type": "module" in your package.json. When I add that, the problem somehow goes away. That's a bit odd, that that would make TypeScript's resolution work differently. But since you're using "module": "ESNext" in your TS config, maybe just telling node that this is an ES module codebase is a reasonable thing to do anyway?

[benasher44]

So that's actually an important difference. With nodenext and no "type": "module", this allows us to build in this intermediate mode where we can use .js suffixes in imports (for local imports) and generally update imports to prepare for ESM, but it's still technically building CJS output. The final migration step is adding "type":"module" to signal to TS that we're building ESM now. Hope that makes sense

[benasher44]

We're using prosemirror in a shared library that needs to exist in this intermediate mode for a brief period, while we update/prepare downstream consumers of the library

[benasher44]

Would it help if I opened PRs to the prosemirror packages to add the missing types entries (aside from the handful of cases where PRs already exist)?

[marijnh]

I really don't want to update all my packages to address a weird limitation in another tool. And, since many people are publishing CJS+ES module packages without using this annoying syntax, I'm still not convinced breaking all those is intentional behavior on the part of TypeScript.

[benasher44]

This issue has been raised many times with TS, and they've made it clear that despite some fallback behaviors that can make it work, you need to have a types entry in exports.

I'm at a loss I suppose. If you won't even accept PRs if I agree to open all of them, I suppose we'll have to locally patch these packages

[andrewbranch]

👋 TypeScript maintainer here, just popping in to clarify a few things.

they've made it clear that despite some fallback behaviors that can make it work

There are no fallbacks we can implement. The mitigation we might be able to consider is providing a more descriptive error message saying that the library’s package.json is probably underspecified or misconfigured.

Ah, what you appear to be doing differently from my tests is that you don't have "type": "module" in your package.json. When I add that, the problem somehow goes away. That's a bit odd, that that would make TypeScript's resolution work differently.

When "type": "module" is present and --module node16 is used, .ts files get compiled to use ESM imports. When "type" is "commonjs" or not set, .ts files get compiled to use CJS require statements. This is analogous to Node treating .js files as either CJS or ESM based on the value of that field, but admittedly TypeScript syntax can obscure exactly what’s going on. So, when you remove "type": "module", you’re dealing with a CJS module that will use require at runtime. That means it’s going through the "require" conditional export of your library:

  "exports": {
     ".": {
       "import": "./dist/index.js",  // Hit with "type": "module"
       "require": "./dist/index.cjs" // Hit without "type": "module"
     },
     "./style/prosemirror.css": "./style/prosemirror.css"
   },

When we see that the require resolves to ./dist/index.cjs, we look for a corresponding ./dist/index.d.cts file and find nothing, and that’s it. Types for the import (require at runtime) don’t resolve. This explains why it does work with "type": "module"—the implementation file is ./dist/index.js so the extension substitution we try is ./dist/index.d.ts and that works, without any special field to tell us where to look.

you need to have a types entry in exports.

So now we should be able to understand too that this isn’t the only solution—in fact, it’s not even the best solution. The best solution is to have both an index.d.ts and an index.d.cts. TypeScript will interpret the former as ESM and the latter as CJS, which could potentially be important for how imports within them resolve, whether they can be imported synchronously from other CJS files, etc. @benasher44 is right that a "types" field in exports would let you resolve to a single type declaration file both for the "import" and for the "require" conditional export, but this can raise other issues and we recommend using separate declarations for separate package entries. This is especially critical if, e.g., the ESM file uses export default Foo where the CJS file uses module.exports = Foo—these constructs are interoperable but not identical, and need to be described differently in types.

And, since many people are publishing CJS+ES module packages without using this annoying syntax, I'm still not convinced breaking all those is intentional behavior on the part of TypeScript.

Things are very much working as intended on our end. We didn’t “break” anything here; typings for your CJS "require" entrypoint have just never existed. Older module resolution algorithms in TypeScript correspond to versions of Node that didn’t support package.json exports, so in those we’re just looking at the top-level main and types and it coincidentally works out. At some point, you took advantage of conditional exports, probably even before TypeScript supported them, but now that we do they need to be properly set up with typings for the first time.

You’re right that many libraries needed updates after TypeScript rolled out support for these newer Node features. Most of the very popular ones are already updated thanks to users like @benasher44 who took the time to track down the issue and report problems and open PRs. And as I said, the best option is to add a colocated .d.cts file so you don’t need any “annoying” syntax (which is documented by Node itself, by the way).

[benasher44]

Thanks Andrew for the detailed walkthrough! Also happy to open PRs that add collocated types files.

[benasher44]

@marijnh any more thoughts here? I'd be happy to contribute the PRs that add the necessary support.

[marijnh]

Would having the build tool emit a symlink from the .d.ts to a .d.cts file, and including that in the package, work for this? Or are symlinks in npm packages problematic?

[benasher44]

I'm not sure about symlinks, but if you're open to that direction, I confirmed that copying index.d.ts to index.d.cts does work! I think the effect would be the same (i.e. would create the file transparently in build tooling).

[marijnh]

npm pack ignores symlinks, so I guess that's a non-starter. Does this structure (matching what the TS docs suggest) work for you:

  "exports": {
    ".": {
      "import": "./dist/index.js",
      "require": {
        "types": "./dist/index.d.ts",
        "default": "./dist/index.cjs"
      }
    },
    "./style/prosemirror.css": "./style/prosemirror.css"
  },

[benasher44]

Yep that works! In my trial, I also tried with and without the top level types entry in place, and both worked.

[benasher44]

Small update: if you remove the top-level types entry, then it breaks when not using NodeNext. So, best to leave that one there 😅. So in other words, the final should look like this:

  "types": "dist/index.d.ts",
  "exports": {
    ".": {
      "import": "./dist/index.js",
      "require": {
        "types": "./dist/index.d.ts",
        "default": "./dist/index.cjs"
      }
    },
    "./style/prosemirror.css": "./style/prosemirror.css"
  },

[andrewbranch]

This doesn’t look right, because there’s still only one .d.ts file. A single .d.ts file cannot represent two different modules (much less modules of different formats) at the same time. Assuming that this package.json has "type": "module" (since the import condition points to a .js file), this structure tells TypeScript that the require condition resolves to an ES module, because the types for require has a .d.ts extension, which is equivalent to a .js extension, whose format is controlled by the package.json "type".

[marijnh]

Assuming that this package.json has "type": "module" (since the import condition points to a .js file), this structure tells TypeScript that the require condition resolves to an ES module

Yes. And that seems to work fine in my tests.

[andrewbranch]

It will result in false-positive TypeScript errors, because a Node require cannot access an ES module. In actuality, it’s not, because the default condition points to a .cjs file. But you’re telling TypeScript that it will, so you’ll get an error when importing from a non-ESM-emitting file. This is very difficult to test if you’re not very familiar with all the relevant permutations. I recommend using https://arethetypeswrong.github.io.

[marijnh]

Could you be more specific about this issue? TypeScript will emit the original require string, I believe, which points to the CommonJS module, so there shouldn't be a run-time problem with requiring an ES module.

[andrewbranch]

A require that resolves to an ES module is always an error in Node. But as you said, a require of the package given the configuration above will, at runtime, resolve through exports["."]["require"]["default"] which is ./dist/index.cjs, which is a CommonJS module, so there’s no issue at runtime. However, TypeScript will resolve the same require (or any import which gets emitted as a require) through exports["."]["require"]["types"], which is ./dist/index.d.ts, which represents ./dist/index.js, which is an ES module. So while there is no problem at runtime for Node, the information available to TypeScript through this configuration indicates that there is a problem at runtime—that a require is going to resolve to an ES module, which will crash in Node. So TypeScript will error accordingly.

[marijnh]

So TypeScript will error accordingly.

Which version of TS in which configuration does this? Because I've been sharing ESM-syntax declaration files between CJS and ESM in TS for a long time without trouble.

[andrewbranch]

All versions of TypeScript that support --module nodenext. Isn’t this issue implying that this practice causes trouble? Here are checks on a bunch of prosemirror packages:

• https://arethetypeswrong.github.io/?p=prosemirror-view%401.31.2
• https://arethetypeswrong.github.io/?p=prosemirror-model%401.19.0
• https://arethetypeswrong.github.io/?p=prosemirror-state%401.4.2
• https://arethetypeswrong.github.io/?p=prosemirror-keymap%401.2.1
• https://arethetypeswrong.github.io/?p=prosemirror-transform%401.7.1

None of them can resolve types from CJS files under --module nodenext. Here’s a full repro in one line:

// main.cts
import {} from "prosemirror-view";

$ npm i typescript
$ npm i prosemirror-view
$ npx tsc --module nodenext --noImplicitAny main.cts

main.cts:1:16 - error TS7016: Could not find a declaration file for module 'prosemirror-view'. '/Users/andrew/Developer/microsoft/eg/ts/node_modules/prosemirror-view/dist/index.cjs' implicitly has an 'any' type.
  Try `npm i --save-dev @types/prosemirror-view` if it exists or add a new declaration (.d.ts) file containing `declare module 'prosemirror-view';`

1 import {} from "prosemirror-view";
                 ~~~~~~~~~~~~~~~~~~

[marijnh]

Isn’t this issue implying that this practice causes trouble?

This issue, as well as the arethetypeswrong links you posted, is about TS not being able to resolve a declaration file when using ProseMirror in a CommonJS project with nodenext module resolution.

If I use "module": "nodenext" I don't get any issues, neither with "type": "module" or without it.

If I use "module": "commonjs" with "moduleResolution": "nodenext", I do get a type resolution error with the current exports structure, but that vanishes when I make the adjustment I suggested above. Hence I'm asking if you can show a configuration that doesn't work with that structure.

[andrewbranch]

If I use "module": "nodenext" I don't get any issues, neither with "type": "module" or without it.

I just gave a one-line repro for a problem with "module": "nodenext".

If I use "module": "commonjs" with "moduleResolution": "nodenext", I do get a type resolution error with the current exports structure, but that vanishes when I make the adjustment I suggested above.

This is an unsupported combination. However, I explained earlier why the proposed structure is a problem for "module": "nodenext" in general. I will give you a full repro for that too.

// main.cts
import {} from "prosemirror-view";

npm i typescript
npm i prosemirror-view

Now edit node_modules/prosemirror-view/package.json with the "exports" change suggested.

$ npx tsc --module nodenext main.cts

main.cts:1:16 - 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("prosemirror-view")' call instead.
   
1 import {} from "prosemirror-view";
                 ~~~~~~~~~~~~~~~~~~

[marijnh]

I just gave a one-line repro for a problem with "module": "nodenext".

Sure, but you didn't mention that reproducing involved compiling a CommonJS file with an import declaration. I wasn't even aware import/export was valid in non-module TypeScript input.

So it seems adding a types export for require breaks this combination, whereas it works fine without it (which is a little puzzling, given that it seems to also fall back to the ESM types there). And the "module": "commonjs" / "moduleResolution": "nodenext" combination which fails is unsupported. What kind of configuration does the node16 (from CJS) row that indicates failure on arethetypeswrong refer to then?

[andrewbranch]

Sure, but you didn't mention that reproducing involved compiling a CommonJS file with an import declaration. I wasn't even aware import/export was valid in non-module TypeScript input.

Imports/exports in CommonJS files get compiled to require on the output (just like in --module commonjs), so they are effectively require calls and resolve like require calls. I could have equivalently written a repro with import ProsemirrorView = require("prosemirror-view") in a TS file or const ProsemirrorView = require("prosemirror-view") in a checked JS file.

So it seems adding a types export for require breaks this combination, whereas it works fine without it

It doesn’t work fine without it. Trying to represent a CJS and ESM module with a single .d.ts file is never valid and will always cause some kind of problem. There is literally no way to correctly type a dual ESM/CJS entrypoint without adding a new type declaration file.

which is a little puzzling, given that it seems to also fall back to the ESM types there

An ESM types file cannot correctly represent a CJS module. The ESM-ness of them is a property of the file extension and package.json "type". It cannot become or stand in for a CJS types file just because a require happened to resolve to them. When a require resolves to an ESM file at runtime, Node errors. Therefore, when a require resolves to an ESM file in TypeScript, TypeScript errors.

What kind of configuration does the node16 (from CJS) row that indicates failure on arethetypeswrong refer to then?

The same thing my repro showed, --module node16 with a require (or import that gets compiled to require) from a .cts file (or equivalently, a .ts file and no package.json "type": "module").

[marijnh]

Therefore, when a require resolves to an ESM file in TypeScript, TypeScript errors.

The thing is, it doesn't. With the current package.json structure, your test case compiles fine. I've been providing a single export-syntax .d.ts files for dual ES/CJS packages for a long time, and except in this case (where the declaration file is explicitly mentioned as a CJS export in package.json), TypeScript seems to accept that.

[andrewbranch]

I believe I’ve provided you with a repro for issues with both the existing structure and the proposed structure. I would be happy to provide it again. For what it’s worth—and I apologize in advance because I always struggle to find a way to say this that doesn’t sound arrogant—I can promise that I can be trusted on this topic, as it’s been my primary focus working on the TypeScript team for the last year. But, I would rather package authors understand this for themselves than take my word for it, so please let me know what issues you would like to see demonstrated. However, do skim through my past messages, because I’m pretty sure I’ve covered the problems with the current package structure.

[marijnh]

But, I would rather package authors understand this for themselves than take my word for it

That's what I'm trying to do, but the tool's behavior keeps throwing off any mental model I'm building on this. As I said, just compiling stuff with --module commonjs works with the existing package.json (which has declarations for ESM but not for CJS in its exports), but starts failing when I add a CJS export pointing to an ESM declaration. I realize that's not intended to work, but then why does it work without the export?

[andrewbranch]

just compiling stuff with --module commonjs works with the existing package.json

I assume you mean --module commonjs --moduleResolution nodenext which you mentioned before. The situation there is actually the same as just --module nodenext. It doesn’t “work,” it just doesn’t error unless you have --noImplicitAny enabled. It fails to find types. Even without noImplicitAny, you can see the suggestion diagnostic in VS Code:

When you point it to an ESM type declaration file, then you get the error about resolving to an ES module from a CJS require.

[marijnh]

Okay, that explains that — if it doesn't find an export it defaults to treating the dependency as untyped, if it does find an export it insists on it being valid.

Now, from further experiments, it seems that I can provide a .d.cts file that has export syntax, as long as the file resolution logic categorizes it as a CommonJS file (which the extension seems to take care of). Can you confirm that this is valid, and putting a copy of dist/index.d.ts under dist/index.d.cts is a reasonable thing to do? I haven't found a convenient way to make my build system emit CommonJS-style declaration files without having it do a lot of compilation work twice, so if I can just reuse my ESM compilation output, that'd be nice.

[andrewbranch]

That’s correct. It’s usually safe to reuse ESM-generated declaration files for CJS declaration files, with a few caveats, from most likely to cause problems to least likely:

• Is export default used in the ESM source, and how does the build tool translate that to CJS?
• Do you have any ESM-only dependencies? If so, any top-level import of those translated to CJS will be invalid (require resolving to ESM error). This is probably a runtime problem for your CJS emitting build tool too, not just a problem with reusing the ESM declarations.
• Do you have any dependencies where resolution from an ES module would resolve to a different file from the resolution from a CJS module due to "import" and "require" conditions, and do those separate modules contain distinct API shapes? This is rare, but not unheard of. If so, reusing ESM declarations will be invalid, but probably the CJS output from your build tool will also be broken.

Checking for these things is why you’d have to do compilation work twice if you want to produce a safe dual build. (I’ve pretty much never seen any dual format library do this safely. Every build tool that exists for dual publishing leaves you in this tough situation with missing TypeScript types.) One thing you could do is copy the ESM declaration files and check that they’re at least error-free with tsc after the fact.

[marijnh]

Great, that should provide a workable solution then.

[marijnh]

I have published new versions of the prosemirror-* modules and the other dependencies under my control.

I still feel, given the fact that ESM declarations work fine in most CJS contexts, that forcing all packages to include a separate .d.cjs file is a needlessly burdensome decision on the part of the TS team, but I guess it's what they decided on, so I went ahead and updated the build system to split out the duplicated file under the prescribed filename.

[benasher44]

Amazing, thank you!