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

module: add --experimental-strip-types #53725

Merged
merged 8 commits into from
Jul 24, 2024

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Jul 4, 2024

Moderation Note: This PR has been posted on several social network platforms and thus attracts a lot of mostly off-topic comments.

If you've used this feature and ran into issues or have specific feedback to provide - please open a new issue https://github.com/nodejs/node/issues/new/choose


It is possible to execute TypeScript files by setting the experimental flag --experimental-strip-types.
Node.js will transpile TypeScript source code into JavaScript source code.
During the transpilation process, no type checking is performed, and types are discarded.

Roadmap

Refs: nodejs/loaders#217

Motivation

I believe enabling users to execute TypeScript files is crucial to move the ecosystem forward, it has been requested on all the surveys, and it simply cannot be ignored. We must acknowledge users want to run node foo.ts without installing external dependencies or loaders.

There is a TC39 proposal for type annotations

Why type stripping

Type stripping as the name suggest, means removing all the types, transform the input in a JavaScript module.

const foo: string = "foo";

Becomes:

const foo = "foo";

Other runtimes also perform transformation of some TypeScript only features into JavaScript, for example enums, which do not exists in JavaScript.
At least initially in this PR no trasformation is performed, meaning that using Enum, namespaces etc... will not be possible.

Why I chose @swc/wasm-typescript

Because of simplicity.
I have considered other tools but they require either rust or go to be added to the toolchain.
@swc/wasm-typescript its a small package with a wasm and a js file to bind it.
Swc is currently used by Deno for the same purpose, it's battle tested.
In the future I see this being implemented in native layer.
Massive shoutout to @kdy1 for releasing a swc version for us.


⚠️ Refer to the PR changes in typescript.md for implementation details and limitations.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 4, 2024
@marco-ippolito marco-ippolito added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 4, 2024
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 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 (per memory usage and performance). (idea from @acutmore)

lib/internal/modules/run_main.js Outdated Show resolved Hide resolved
@marco-ippolito
Copy link
Member Author

marco-ippolito commented Jul 4, 2024

🎉 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

@legendecas
Copy link
Member

Support extensionless imports

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

@marco-ippolito

This comment was marked as resolved.

@panva
Copy link
Member

panva commented Jul 4, 2024

@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
Copy link
Member Author

marco-ippolito commented Jul 4, 2024

@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.

@panva
Copy link
Member

panva commented Jul 4, 2024

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?

@marco-ippolito
Copy link
Member Author

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 🔥

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Jul 4, 2024

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.

@joyeecheung
Copy link
Member

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.

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Jul 4, 2024

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

@RedYetiDev RedYetiDev added the experimental Issues and PRs related to experimental features. label Jul 4, 2024
doc/api/cli.md Outdated Show resolved Hide resolved
@AugustinMauroy
Copy link
Member

AugustinMauroy commented Jul 4, 2024

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;

doc/api/module.md Outdated Show resolved Hide resolved
doc/api/module.md Outdated Show resolved Hide resolved
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@enricopolanski
Copy link

enricopolanski commented Jul 5, 2024

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).

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Jul 5, 2024

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

@nodejs-github-bot nodejs-github-bot merged commit 35f92d9 into nodejs:main Jul 24, 2024
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 35f92d9

@karlhorky
Copy link
Contributor

karlhorky commented Jul 25, 2024

Try out the Node.js v23.0.0-nightly202407253de7a4c374 nightly version with --experimental-strip-types

CodeSandbox: https://codesandbox.io/p/devbox/node--experimental-strip-types-with-node-js-nightly-dxdtmp?file=%2Findex.ts

CodeSandbox demo screenshot showing "Hello TypeScript!" being logged in the console, after types were stripped from index.ts by the --experimental-strip-types flag

@xPaw
Copy link

xPaw commented Jul 25, 2024

I am curious, is there really no better way of shipping the wasm besides inlining it as base64 in deps/amaro/dist/index.js which is 2.47mb?

And what guarantees are there this binary blob won't get backdoored at some point in the future?

@marco-ippolito
Copy link
Member Author

I am curious, is there really no better way of shipping the wasm besides inlining it as base64 in deps/amaro/dist/index.js which is 2.47mb?

And what guarantees are there this binary blob won't get backdoored at some point in the future?

because we build the wasm in our repo https://github.com/nodejs/amaro

@Daniel15
Copy link

Daniel15 commented Jul 27, 2024

I have have missed a discussion (sorry - there's a lot of comments here) but is the idea to eventually support other typing systems (for example, Flow, Hegel, or whatever comes next after TypeScript) like https://github.com/tc39/proposal-type-annotations proposes, or is this feature focused solely on TypeScript? If it's only going to be for TypeScript, wouldn't it be better to include typescript or ts in the name of the flag?

@marco-ippolito
Copy link
Member Author

I have have missed a discussion (sorry - there's a lot of comments here) but is the idea to eventually support other typing systems (for example, Flow, Hegel, or whatever comes next after TypeScript) like https://github.com/tc39/proposal-type-annotations proposes, or is this feature focused solely on TypeScript? If it's only going to be for TypeScript, wouldn't it be better to include typescript or ts in the name of the flag?

Please lets keep the conversation in the relevant issue nodejs/loaders#217

targos pushed a commit that referenced this pull request Jul 28, 2024
PR-URL: #53725
Refs: nodejs/loaders#217
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
@ovflowd
Copy link
Member

ovflowd commented Jul 28, 2024

Awesome to see this landing ❤️

@ovflowd
Copy link
Member

ovflowd commented Jul 28, 2024

FYI @nodejs/nodejs-website https://nodejs.org/en/learn/getting-started/nodejs-with-typescript this should be updated ASAP.

@ovflowd
Copy link
Member

ovflowd commented Jul 28, 2024

@nodejs/releasers @nodejs/build can we add to the dist.json which TypeScript version the Node binary is using for each release to strip types? It'd make sense to add this meta info within the Website and our Distribution meta. I believe it'd also make sense to mention which version of SQLite we ship.

@AugustinMauroy
Copy link
Member

FYI @nodejs/nodejs-website https://nodejs.org/en/learn/getting-started/nodejs-with-typescript this should be updated ASAP.

I can do it, I've already written part of this page, but is it really necessary because it's not yet in release and not stable?

@marco-ippolito
Copy link
Member Author

@nodejs/releasers @nodejs/build can we add to the dist.json which TypeScript version the Node binary is using for each release to strip types? It'd make sense to add this meta info within the Website and our Distribution meta. I believe it'd also make sense to mention which version of SQLite we ship.

can you create an issue for this so we can followup

@liudonghua123

This comment has been minimized.

@nodejs nodejs locked as too heated and limited conversation to collaborators Jul 28, 2024
@ovflowd
Copy link
Member

ovflowd commented Jul 28, 2024

FYI @nodejs/nodejs-website https://nodejs.org/en/learn/getting-started/nodejs-with-typescript this should be updated ASAP.

I can do it, I've already written part of this page, but is it really necessary because it's not yet in release and not stable?

I think it is fair to mention about this new feature there. But Im also fine deferring to wait until it becomes stable.

@ovflowd
Copy link
Member

ovflowd commented Jul 29, 2024

@nodejs/releasers @nodejs/build can we add to the dist.json which TypeScript version the Node binary is using for each release to strip types? It'd make sense to add this meta info within the Website and our Distribution meta. I believe it'd also make sense to mention which version of SQLite we ship.

can you create an issue for this so we can followup

@marco-ippolito should it be an issue on the Build repo?

@marco-ippolito
Copy link
Member Author

@nodejs/releasers @nodejs/build can we add to the dist.json which TypeScript version the Node binary is using for each release to strip types? It'd make sense to add this meta info within the Website and our Distribution meta. I believe it'd also make sense to mention which version of SQLite we ship.

can you create an issue for this so we can followup

@marco-ippolito should it be an issue on the Build repo?

I think so yes @nodejs/build

@ovflowd
Copy link
Member

ovflowd commented Jul 30, 2024

Sorry, @marco-ippolito! I was busy with other stuff, here we go nodejs/build#3847

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
baking-for-lts PRs that need to wait before landing in a LTS release. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features. lib / src Issues and PRs related to general changes in the lib or src directory. loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. tsc-agenda Issues and PRs to discuss during the meetings of the TSC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.