Concerns with TypeScript 4.5's Node 12+ ESM Support

DanielRosenwasser commented 2 years ago

For TypeScript 4.5, we've added a new module mode called node12 to better-support running ECMAScript modules in Node.js. Conceptually, the feature is simple - for whatever Node.js does, either match it or overlay something TypeScript-specific that mirrors thes same functionality. This is what TypeScript did for our initial Node.js support (i.e. --moduleResolution node and --module commonjs); however, the feature is much more expansive, and over the past few weeks, a few of us have grown a bit concerned about the complexity.

I recently put together a list of user scenarios and possibly useful scripts for a few people on the team to run through, and we found a few sources of concerns.

Bugs
UX
Ecosystem
User Guidance

Bugs

Most complex software ships with a few bugs. Obviously, we want to avoid them, but the more complex a feature is, the harder it is to cover all the use-cases. As we get closer to our RC date, do we feel confident that what we're shipping has as few blocking bugs as possible?

I would like to say we're close, but the truth is I have no idea. It feels like we'll have to keep trying the features for a bit until we don't run into anything - but we have less than 3 weeks before the RC ships.

Here's a few surprising bugs that need to get fixed before I would feel comfortable shipping node12 in stable.

Code changes breaking module resolution
- https://github.com/microsoft/TypeScript/issues/46373
- https://github.com/microsoft/TypeScript/issues/46396
Auto-imports not working: https://github.com/microsoft/TypeScript/issues/46332 (technically present in CommonJS)
resolveJsonModule can't be used with node12: https://github.com/microsoft/TypeScript/issues/46362
No errors on extensionless imports from .ts and .tsx files in node12 (unfiled, reported by @andrewbranch)
Strange errors under pnpm (unfiled, reported by @DanielRosenwasser)
package.json changes in packages not tracked (unfiled, reported by @DanielRosenwasser)

UX Concerns

In addition to bugs we found, there are just several UX concerns. Package authoring is already a source of confusion in the TypeScript ecosystem. It's too easy to accidentally shoot yourself in the foot as a package author, and it's too hard to correctly consume misconfigured packages. The node12 mode makes this a whole lot worse. Two filed examples of user confusion:

It's too hard to tell whether you're in an ESM or a CJS file: https://github.com/microsoft/TypeScript/issues/46408
The export field is confusing to configure and diagnose: https://github.com/microsoft/TypeScript/issues/46334
Poor errors on extensionless imports: https://github.com/microsoft/TypeScript/issues/46152

While there might be a lot of "working as intended" behavior here, the question is not about whether it works, but how it works - how do we tell users when something went wrong. I think the current implementation leaves a lot of room for some polish.

But there are some questions about this behavior, and we've had several questions about whether we can simplify it. One motivating question I have is:

When a user creates a new TypeScript project with this mode, when would they not want "type": "module"? Why? Should that be required by default?

We've discussed this a bit, and it seems a bit strange that because we want to cover the "mixed mode" case so much, every Node 12+ user will have to avoid this foot-gun.

I would like to see a world where we say "under this mode, .ts files must be covered by a "type": "module"". .cts can do their own CommonJS thing, but they need to be in a .cts file.

Another motivating question is:

Why would I use node12 today instead of nodenext?

Node 14.8 added top-level await, but Node 12 doesn't have it. I think this omission is enough of a wart that starting at Node 12 is the wrong move.

Ecosystem

The ecosystem is CONFUSING here. Here's a taste of what we've found:

ts-node, Webpack, and Vite don't like imports with .js extensions, but TypeScript expects them. Not all of these can be configured with a plugin.
ts-node, Webpack, and Vite, and Deno are fine with .ts extensions, but TypeScript doesn't allow them!
Many packages that ship types have started supporting export fields, but don't have a types sub-field within export (e.g. RxJS, Vue 3).
Many packages have started supporting export fields, but their @types package might not reflect that.

The last two can be easily fixed over time, though it would be nice to have the team pitch in and help prepare a bit here, especially because it's something that affects our tooling for JavaScript users as well (see https://github.com/microsoft/TypeScript/issues/46339)

However, the first two are real issues with no obvious solutions that fall within our scope.

There's also other considerations like "what about import maps?" Does TypeScript ever see itself leveraging those in some way, and will package managers ever support generating them?

Guidance

With --moduleResolution node, it became clear over time that everyone should use this mode. It made sense for Node.js apps/scripts, and it made sense for front-end apps that were going to go through a bundler. Even apps that didn't actually load from node_modules could take advantage of @types in a fairly straightforward way.

Now we have an ecosystem mismatch between Node.js and bundlers. No bundler is compatible with this new TypeScript mode (and keep in mind, back-end code also occasionally uses a bundler).

Here's some questions I wouldn't know how to answer confidently:

Is our guidance to always use this mode for plain Node.js apps, and let tools "catch up"?
Should new projects that use this mode pick node12 or nodenext?
There's a big foot-gun with forgetting "type": "module" - should we always recommend .mts?

Next Steps

I see us having 3 options on the table:

A mad dash to fix everything - I think this is too hard in the next 2 weeks to pull off.
Keep the feature in, but make it inaccessible until we're ready - I think temporarily removing the feature would be impractical. It would probably take more work to remove it than to fix the issues I've already mentioned. So we might as well keep it in.
Ship with some "experimental" labeling - I think this makes the most sense, but with some caveats to what I mean by "ship". It would make sense to just ship this as "experimental", but I think we should make this feature only work in nightly releases so that people can continue to easily use it, but not depend on a stable version until it's ready.

justinfagnani commented 2 years ago

@andrewbranch I maintain a package that publishes types and uses package exports. Is there a summary of what we would need to do?

andrewbranch commented 2 years ago

You just need to ensure that typescript@next finds typings for imports of your exports when set to --module nodenext. This will happen automatically if there is a .d.ts file colocated with each .js file referenced by the import map. (IIRC, if these JS files use .mjs or .cts extensions, the declaration files will need to use .d.mts or .d.cts respectively in order to be picked up automatically.) If that’s the case, no changes will be necessary, but you should test and make sure it’s working as expected. If you ship your .d.ts files in a separate folder like RxJS does, you’ll need to add types keys to each leaf entry in exports.

frank-dspeed commented 2 years ago

@andrewbranch how is it with exports + types property directly in the package.json not on the exports. they will get completly ignored? i think it would be great to at last fallback to that.

by the way i would love if it would get considered that types for imports would get hornored. so i could easy overwrite wrong types.

andrewbranch commented 2 years ago

Any exports in a package.json at all will block nodenext resolution from using types and typesVersions fields. That’s by design and will not change. I believe types and typesVersions can still be used for resolutions in nodenext as long as there was no package.json with exports in the way.

demurgos commented 2 years ago

We’re aware that RxJS needs an update. If you or someone else makes that PR before we get to it, please ping me on it. Thanks!

I sent a PR for RxJS in October. At the time they were waiting for the feature to be available in stable TS. I assume this is still the case.

Shakeskeyboarde commented 2 years ago

It seems a little like too much is trying to be accomplished at once. resolving esm modules seems to work fine right now (even if the imports are extensionless, and the node algorithm is now incomplete), so I’m not sure why nodenext is being added to moduleResolution (At least as a first step).

What we really need, is compiled output that is esm compatible. So add the extra suffix necessary (.js, .json, /index.js, etc.), and don’t worry about resolution and import syntax changes in the MVP. Leave off the new moduleResultions option for now, and keep defaulting it to node when module is set to nodenext. Also, make the nodenext option dumb, and ignore the package type setting. Add an output extension option so people can choose .js, .cjs, or .mjs, explicitly. You can make the default for that option smarter, later (though I’m not sure you should)

No changes needed to existing code (win). None of the side issues that seem to introduced by trying to change the module resolution mechanism (win).

if the issue is “but the TS import paths won’t match the esm spec”, I don’t care, and neither should you. Module transformations and interpretations are here to stay. import->require, classic vs node resolution, type only imports. This ship has sailed. The goal of having explicit paths in esm doesn’t even make sense for typescript, which is going to stay a compiled language. It’s important the output have explicit imports so that runtimes can handle them, not that the developer writes them.

Json imports won’t work in node without the experimental flag, but just call it out in the docs and move on. They’ve already committed to fixing it.

Conaclos commented 2 years ago

What we really need, is compiled output that is esm compatible

TSC already supports this: you need to add .js extension to every relative imports. Unfortunately there is no way to enforce this.

Shakeskeyboarde commented 2 years ago

What we really need, is compiled output that is esm compatible

TSC already supports this: you need to add .js extension to every relative imports. Unfortunately there is no way to enforce this.

This does not work when importing a ts file (or will it reinterpret js to mean ts if a ts file exists? Doubting it, but I forget if I’ve tried it yet).

weswigham commented 2 years ago

This is a large feature because it's the minimum required to support and check node esm as it actually exists today, as it was introduced alongside a suite of complex resolver features to handle esm/cjs interop. "Just give me [whatever one thing that frustrated me in particular]" sound great, but we find people stop trusting their type checker if it fails to check for common problems, and believe it or not, node's esm is a big source of common problems. Plus, once you put together every request like that, you get the whole feature (every part of this feature came from some request!), but with an even more confusing array of toggleable flags, which definitely isn't going to help us.

If what you describe is all you want and you think you can get away without having proper checking, you're free to use module: esnext with moduleResolution: commonjs alongside a babel transformer that patches your import paths (which we'll never do because the process is error-prone unless you're at the app bundle level, and we don't bundle), which should get what you want; and if that's enough for you, great, but it's far from actually accomplishing our job of checking real code as it exists.

Shakeskeyboarde commented 2 years ago

“TS modules” are already a thing, with switchable resolution and transform strategies. Happily, until now, they closely resembled node requires. But the ecosystem just got complex enough that that’s no longer true.

Embrace TS modules. Give those bundlers something to target. Give us toggles for the ones that need a specific transform. Toggles absolutely will help.

This dogmatic, “no import path transforms”, is not helping. Please stop. TS operates in two modes today. Transpiling files (where path transforms are easy), and transpiling source (where either the bundler asks typescript how to resolve, or handles the resultion at the bundler). The bundler problem seems solvable.

Conaclos commented 2 years ago

This does not work when importing a ts file (or will it reinterpret js to mean ts if a ts file exists? Doubting it, but I forget if I’ve tried it yet).

This works: I use this in all my projects since more than 1 year. If I am not wrong the release that introduces this also recommends to go this way. This is the only way to have esm-node-compatible code with TSC.

As an example:

// @filename src/a.ts
...

// @filename src/b.ts
import * as a from "./a.js" // this is automatically resolved to src/a.ts by TSC
...

You have to enable three settings in tsconfig to avoid issues with type-imports, commonjs imports, and wrong dead-code eliminations:

  "compilerOptions": {
    "esModuleInterop": true,
    "importsNotUsedAsValues": "error",
    "preserveValueImports": true,
    ...
  }

Shakeskeyboarde commented 2 years ago

This works, I used this in all my projects since more than 1 year. If I am not wrong the release that introduces this also recommand to go this way. This is the only way to have esm-node-compatible code with TSC.

As an example:
// @filename src/a.ts
...

// @filename src/b.ts
import * as a from "./a.js" // this is automatically resolved to src/a.ts by TSC
...

Ah, I didn’t know that! Thanks!

@weswigham look at that, a path transform. Technically, a path resolution, but tell me a transform wouldn’t be better there.

Conaclos commented 2 years ago

@Shakeskeyboarde By the way I could like the support of .ts imports in order to be compatible with deno...

weswigham commented 2 years ago

Plus, we already did the hard work of shipping it (we already did the action items in the OP of this issue, so maybe it's appropriate to close, idk), we're just trying to polish it before it's marked as stable (it is in the codebase and fairly feature-complete) because it's capable of checking so many more real patterns. ❤️

Specifically, for those of you watching at home, we're trying to both make sure its language service features are more bug-free, and we're looking to provide some new type-level syntax that allows reaching across formats and pulling on type information even if your file's default runtime resolution doesn't allow it (which in turn makes declaration emit more reliable). With those in place all we really want is to get more of the people who early-adopted node's exports with TS types to actually list a types condition as well, so we can find types for those exports (if their types aren't adjacent to their js). As embellishment, we're also thinking about how we might be able to help point out common bugs in node export maps, as people can easily misunderstand how they resolve in practice.

Shakeskeyboarde commented 2 years ago

@weswigham i do appreciate that. Thank you. I’m just worried because a language I like is making big decisions, and possibly being too clever :).

weswigham commented 2 years ago

deno resolution is a separate, even more complex beast, supporting all modern browser features in some scopes (import maps, full url imports), and these new node features in others (it has a node package compat layer), and some custom modifications besides (.ts direct loading, override comments, redirect comments).

I don't think we'd consider it until after we've overhauled a bundler/browser-specific resolver mode first, since it'd need to use such a mode as a base. classic doesn't really map to anything useful nowadays (except maybe not-node_modules-based).

frank-dspeed commented 2 years ago

@weswigham i am still for joining the projects rollup and typescript there is a acron to typescript ast and vice versa implementation when the both asts are compatible you can gain a full plugin system for typescript via rollup rollup is basicly a loader not a bundler and it is basicly only a plugin system so a loader with a plugin system.

i am working on that you can contact me by mail and we can schedule something i mentioned that at a few places already. and rollup now got a disk based cache as you can see in the microsoft pwa builder starter example already. Sooner or later it will make sense to create a TypeScript Language server that is able to interpret a tsconfig.js like a rollup.config.js that will then feed everything into the TypeScript Programm (TypeChecker)

https://github.com/cmorten/deno-rollup

andrewbranch commented 2 years ago

TSC already supports this: you need to add .js extension to every relative imports. Unfortunately there is no way to enforce this.

@Conaclos moduleResolution node12/nodenext, the feature being discussed in this issue, enforces this.

By the way I could like the support of .ts imports in order to be compatible with deno...

frank-dspeed commented 2 years ago

this issue closes: https://github.com/microsoft/TypeScript/issues/33079 is that correct that it is only missing the subpath pattern implementation? while i need to say nightly works with "exports": { ".": { "types": "" } }

frank-dspeed commented 2 years ago

@DanielRosenwasser this needs maybe a bit more attention

philkunz commented 2 years ago

This feature should be made available in stable builds, not only nightly and this is why:

We have a project that uses ts-node. ts-node requires a stable peer dependency of typescript.

Thus we cannot update to esm as of yet, since the the node12 module resolution is not available in stable. Meanwhile a lot of packages that don't use TypeScript switch to esm, which makes it impossible to import those packages in a sync way. Since we are not on esm, there is no toplevel await. Some of our dependencies like node-fetch, have vulnerabilites in the outdated commonjs versions.

Thus we have to create our own verdaccio instance, relabeling a nightly build to a stable version.

Thus we break package compatibility of our Open Source MIT licensed packages for other contributors, that don't have access to our private verdaccio instance...

Please label it as unstable, just don't make it fail in stable builds.

weswigham commented 2 years ago

It doesn't "fail" - it just issues a warning diagnostic. Everything actually works in stable same as in nightly.

philkunz commented 2 years ago

@weswigham sure? For me it skips the emit when using node12 type module resolution with stable semver.

weswigham commented 2 years ago

Do you have noEmitOnError set? Afaik, it's not an unconditionally emit blocking error, though I may be mistaken, and our default behavior is to emit even in the presence of errors.

philkunz commented 2 years ago

@weswigham Why would I want to emit on error? Isn’t the while thing about typescript to have type safety?

weswigham commented 2 years ago

The process will still print the errors, return a nonzero exit code, and the like - you can just still work with a work-in-progress output file. It's so temporary errors while you're working don't block, eg, live reloading for a preview. You still have errors, they just don't block getting a potentially usable output (that also has the same errors).

philkunz commented 2 years ago

@weswigham sounds hacky. Either way we would need to adjust CI to ignore errors then. It works in nightly, but nightly is not supported by ts-node. It would just be nice if typescript wouldn’t actively block esm adoption, when it clearly works already in most cases since a lot of upstream security maintenance gets blocked by it. Just label it as unstable, so people know its unstable as is the —experimental-loader api for esm in nodejs. People are using it, sindresorhus has updated a huge part of his modules, so we kind of need to move with it.

frank-dspeed commented 2 years ago

@philkunz maybe you should think about delaying the adoption of node12 nodenext resolvers anyway as i am experimenting with it on large mixed mono repo bases i found out that many packages are incompatible with that resolve mode

they do not got a exports: types: fild or other edgecases where they mix main and exports and then typescript does not support at present subPath patterns

go with a file in your project put that into a extra folder create a tsconfig with node resolve for it and then import the dependencies there see consumption of typescript modules.

when you switch for example to node12 only exports: types will get looked up rest gets ignored. there is no fallback.

i try to writte a fast guide for incremental adoption of the new resolve modes via sub projects and project references.

vajahath commented 2 years ago

I'm sorry but can someone please explain why we need nodenext or node12 when we already have the esnext. The documentation seem a little unclear there. So I asked on SO. https://stackoverflow.com/q/71463698/3370568 If someone could explain it there, that would be very helpful for the community coming new to this topic.

philkunz commented 2 years ago

@vajahath Its not about the "module", but mainly about module-resolution with nodenext and node12.

andrewbranch commented 2 years ago

@vajahath it’s a good question; I answered there.

philkunz commented 2 years ago

@DanielRosenwasser Could you state what the current status is here? I'm using the node12 resolution mode. It works for me and already helped me identifying esm implementation issues in projects like parcel, lit that are being worked on now.

Can we count on node12 being enabled by default in 4.7.0?

Jack-Works commented 2 years ago

Another request, can we drop Node 12, but start from Node 14? Node 14 starts to support top-level await.

shadowspawn commented 2 years ago

I see the comment https://github.com/microsoft/TypeScript/issues/46452#issuecomment-1031697471

One of the things we plan to do before shipping this as stable is auditing the top N most popular npm packages that ship their own types and use package.json exports.

Is adding "types" to "exports" stable enough that package authors can adopt this now in release versions? Or should we be ready but wait? (I have been reading long threads but still unsure!)

Example PR against rxjs: https://github.com/ReactiveX/rxjs/pull/6802/files

And this question prompted by PR for Commander: https://github.com/tj/commander.js/pull/1704

weswigham commented 2 years ago

Is adding "types" to "exports" stable enough that package authors can adopt this now in release versions?

100% yes.

weswigham commented 2 years ago

Example PR against rxjs

From what I can see, that uses the same types for both the cjs and esm entry points - as far as we know, that means only one of the two exists (cjs if not type:module, esm otherwise). Technically every entry point with a different module kind should probably have its own types entry. (Meaning you should probably use a compound condition)

frank-dspeed commented 2 years ago

@weswigham what means the term compund condition? can you give me a code example? as far as i am aware typescript can not even handle subPathPatterns and also not nestedConditions at last in the last nightly it could not do that.

so my question what gets combined? do you mean creating some how a conditional wrapper? in a additional file and reference that via that single entry?

there is as far as i am aware not even a condition Selector see:

[ ] https://github.com/microsoft/TypeScript/issues/47931

weswigham commented 2 years ago

what means the term compund condition?

Being very explicit:

"exports": {
  ".": {
    "import": {
      "types": "./types/esm/index.d.mts",
      "default": "./dist/esm/index.mjs"
    },
    "default": {
      "types": "./types/cjs/index.d.ts",
      "default": "./dist/cjs/index.js"
    }
  }
}

It's nesting another conditions object within the one matched.

weswigham commented 2 years ago

as far as i am aware typescript can not even handle subPathPatterns

We should, we just don't provide auto-imports for them yet.

weswigham commented 2 years ago

there is as far as i am aware not even a condition Selector

Correct, we do not currently allow setting custom conditions like "development" or "production" - just the core "node", "import", "require", "types" (and versioned variants, eg "types@4.8"), and "default" (obviously only one of "require" or "import" is set, depending on usage).

schickling commented 2 years ago

as far as i am aware typescript can not even handle subPathPatterns

We should, we just don't provide auto-imports for them yet.

Is there a separate GH issue for this I can subscribe to? @weswigham

yordis commented 2 years ago

How is syntactic sugar supposed to work? https://nodejs.org/api/packages.html#exports-sugar

{
  "exports": {
    ".": "./main.js"
  }
}

and

{
  "exports": "./main.js"
}

weswigham commented 2 years ago

We look for a main.d.ts alongside main.js.

jasonwilliams commented 2 years ago

We look for a main.d.ts alongside main.js.

How does compilation work with exports? Do I need to manually generate those d.ts files for each subpath or will tsc handle that?

frank-dspeed commented 2 years ago

@jasonwilliams its tsc default behavior to create the files at the same position as the src files if you do not define anything else in your tsconfig you should read the tsconfig docs carefully. everything gets a tandem lookup so if you import or require a file it will strip the file extension and look for the .d.ts file under the same file name that also covers .cjs .d.cts and .mjs .d.mts

frank-dspeed commented 2 years ago

@Jack-Works top level await makes no diffrence in the module resolve mode the 12 is simply indicating 12+ it makes no sense to add every version.

weswigham commented 2 years ago

Actually it does. TLA is a module system feature (because it mucks with how modules are loaded and isn't downlevelable), which is why it's enabled in module: nodenext, but not module: node12.

frank-dspeed commented 2 years ago

@weswigham but TLA does not change anything not even the order in nodejs there are 2 total indipendent trees for the modules there is even a context function to sync the trees. https://nodejs.org/api/module.html#modulesyncbuiltinesmexports.

sure TLA blocks script execution until the other one is inited but thats it when we would abstract all that we could come out with.

maybe we talk out of different views i for my self speaking from the v8 view of things there are 2 diffrent v8::isolate instances running one for the CJS tree and one for the ESM tree while the CJS tree gets shadowed Copyed one time to the ESM tree thats why https://nodejs.org/api/module.html#modulesyncbuiltinesmexports exists. And as TLA will send only a RPC call to the CJS context if needed to require cache that in the instance and return on the internal Queue that it is done then the linking kicks in. If it is not CJS where we do TLA on then nothing changes it stays the same interpreted Stack and will finish in one process-nexttick() loop cycle.

So when we say TLA has impact on resolve then only on Resolve in CJS Context. and there it has less impact because of the linking. Things will fail directly if the CJS code does anything after many process-nextticks there is not much that can be done thats also why https://nodejs.org/api/module.html#modulesyncbuiltinesmexports is documented as there maybe the need to call that from the CJS code after you as the coder Know that you did load fully.

the await call blocks only the instantiation of the current importing script while it is already parsed until the cjs context resolved the module fully instantiated and then link the current state. It does not Block parsing of other tree parts that are not directly related to resolve the current tree that uses await.

weswigham commented 2 years ago

The implementation details don't really matter; node 12 esm format files don't support tla. Higher versions of node do in esm files. No version of node supports tla in cjs files. That matrix means it's only enabled for node-ish modes that distinguish between esm and cjs and which are node versions higher than 12, ie, nodenext.

frank-dspeed commented 2 years ago

@weswigham thats fully correct but the most people do code for bundlers and all bundlers do support that. I know near no one who directly uses Typescript Compiler Ouput for deployments. So this should be reconsidered some how as it would help to algin the ecosystem out of my view. And the Other Factor that kicks in is NodeJS does not support TLA in CJS context while many other Popular Engines do support that !!!!

microsoft / TypeScript