GeezerVison

Review requested:

• @nodejs/actions
• @nodejs/gyp
• @nodejs/loaders
• @nodejs/security-wg
• @nodejs/tsc

🎉 thank you for the great work!

I think correct source locations in the error stacks are required in the short term as well.

One possible solution could be that avoid altering source locations in stripping so that source maps generation and mapping would not be required. Note that source maps come with a cost and it would not be trivial.

Sourcemaps are already supported by swc and we might just add a flag to enabled them.
We could ask swc an option to replace syntax that have been removed with whitespaces so that we don't alter original error stacks

Support extensionless imports

Not supporting extension-less imports is by-design: https://nodejs.org/api/esm.html#mandatory-file-extensions

@marco-ippolito, speaking as a module maintainer here, it is very common for TS module codebases to have its own file imports written with .js extensions in the import statements, despite the files being .ts.

If I'm to replace --import=tsx/esm with --experimental-strip-types, or skip a compile step before testing, this would need to be able to resolve to a .ts file even when .js was in the statement (but not in the filesystem), otherwise I'll have to change the project imports and then deal with more pain from the compiler (or tsc typecheck) before emitting the actual module code to publish.

@marco-ippolito, speaking as a module maintainer here, it is very common for TS module codebases to have its own file imports written with .js extensions in the import statements, despite the files being .ts.

If I'm to replace --import=tsx/esm with --experimental-strip-types, or skip a compile step before testing, this would need to be able to resolve to a .ts file even when .js was in the statement (but not in the filesystem), otherwise I'll have to change the project imports and then deal with more pain from the compiler before emitting the actual module code to publish.

This is something we definitely want look into, to be able to maintain compatibility with what you run and what you compile.
It's in the checklist for future iterations, it might be resolved with extensions guessing (?), it requires further discussion.

This is something we definitely want look into

Glad it's on your mind then!

As a sidenote, would --strip-types be a no-op if https://github.com/tc39/proposal-type-annotations ever came to be?

This is something we definitely want look into

Glad it's on your mind then!

As a sidenote, would --strip-types be a no-op if https://github.com/tc39/proposal-type-annotations ever came to be?

It's still in Stage 1 so it will take at least a few years but I hope v8 will implement the api to make it happen 🔥

Resolution of import specifiers isn’t specific to module type, so we can’t have extension searching just for TypeScript files; and for many reasons we don’t support it in ESM. However it’s not necessary: users should write their import specifiers with .ts extensions, like import './file.ts', and there’s https://www.typescriptlang.org/tsconfig/#allowImportingTsExtensions allowImportingTsExtensions to support this via tsconfig.json. Such behavior also aligns us with Deno.

If the user wants to use the same source files both run directly via this new flag and run as transpiled JavaScript output by a build tool, it would be the responsibility of the build tool to convert file.ts into file.js; but this is something that build tools are more than capable of handling.

There are two aspects of TypeScript support here:

1. Stripping the syntax (i.e. essentially polyfilling the type annotation as syntax proposal)
2. Modify the resolution rules so that .ts files are accepted, with 1 applied (and as @panva mentioned, allow e.g. import 'foo' to resolve to /path/to/foo/something.ts)

AFAICT this does both (although the second one does it in a limited way), so that brings up the question on whether we want to make this this unflagged eventually. If we want to unflag it eventually then 2 is going to be a breaking change for existing TypeScript loaders that also try to modify the resolution rules in their own way, then we should only strip the syntax for files with explicit formats but don't try to override the resolution rules. If we want to keep it flagged as --strip-syntax eventually, then doing 2 is fine, though we might want to warn that it would be in conflict with other TypeScript loaders that actually do more like supporting enums.

There are two aspects of TypeScript support here:

1. Stripping the syntax (i.e. essentially polyfilling the type annotation as syntax proposal)
2. Modify the resolution rules so that .ts files are accepted, with 1 applied (and as @panva mentioned, allow e.g. import 'foo' to resolve to /path/to/foo/something.ts)

AFAICT this does both (although the second one does it in a limited way), so that brings up the question on whether we want to make this this unflagged eventually. If we want to unflag it eventually then 2 is going to be a breaking change for existing TypeScript loaders that also try to modify the resolution rules in their own way, then we should only strip the syntax for files with explicit formats but don't try to override the resolution rules. If we want to keep it flagged as --strip-syntax eventually, then doing 2 is fine, though we might want to warn that it would be in conflict with other TypeScript loaders that actually do more like supporting enums.

import foo from "./foo" is not supported, file extension is needed.
With this pr there is no resolution override, the test 👇
https://github.com/nodejs/node/pull/53725/files#diff-2fcd423c995b0d2501236a6f427092c5ab1df06ba9241285a471eebc2f12970dR40
I added an item in the issue to track the work to discuss about this

In test suite I think we should add some test to know how node will work with that:

const X = {} as const;

and

import type { Config } from 'myConfig.d.ts';

export const config = {} satisfies Config;

I see few issues with the module approach:

1. Usage of .ts extensions in imports and the allowImportingTsExtensions flag in TypeScript land is virtually non existent. Imports are either extensionless or use .js one (see point 2).

2. To complicate things even further ESMs resolution in TypeScript is mostly achieved in codebases by adding .js extension to imports even if the .js file at that path does not exist.

https://www.typescriptlang.org/docs/handbook/modules/theory.html#module-resolution-for-libraries

This is especially important in library code (as many projects out of there simply opt-out of ESM entirely and either create bundles FE-wise, or they compile down to CJS for Node ones) but the number of real world products using this pattern increases consistently every day with CJS being phased out by an ever growing number of projects.

I wonder whether the flag could strip types and run @swc/wasm-typescript on files regardless of their extension. If it is a normal js file it will simply not need to strip anything (albeit it will still have to parse it).

This is neat, I'd like to see extensionless imports work if possible, but I also think this can land as-is and those issues fixed before making it stable.

@benjamingr this is currently not possible because of this mandatory-file-extensions, it really depends on the esm loader.

Usage of .ts extensions in imports and the allowImportingTsExtensions flag in TypeScript land is virtually non existent. Imports are either extensionless or use .js one (see point 2).

To complicate things even further ESMs resolution in TypeScript is mostly achieved in codebases by adding .js extension to imports even if the .js file at that path does not exist.

@enricopolanski That is something that might be resolved with guessing, as you can immagine it requires node to perform a few expensive operation on the file system and a heuristic attempt to guess the extension.

Example:

• import foo from './foo.js'
• stat ./foo.js => does not exists
• stat ./foo.ts => exists
  And
• import foo from './foo.ts'
• stat ./foo.ts => does not exists
• stat ./foo.js => exists

We will iterate on this and it's tracked in the issue nodejs/loaders#208