nodejs / typescript

TypeScript support in Node.js core
MIT License
83 stars 0 forks source link

Unflag `--experimental-strip-types` #17

Open marco-ippolito opened 2 days ago

marco-ippolito commented 2 days ago

Issue to track the work needed to unflag --experimental-strip-types, blockers and other considerations.

@nodejs/typescript

mcollina commented 2 days ago
  1. I think we should document a "grow up" path for code written in that way, in other words, what steps a developer should take to publish that code on npm. @DanielRosenwasser mentioned this during one of the calls.

  2. I would like a final ack from the TypeScript team on the current implementation

  3. Some consideration of ESM vs CJS vs tsconfig.json situation - specifically I presume we would have to document that we expect a certain kind of moduleResolution and other TS configurations.

  4. Notify tsx, ts-node and other popular loaders that we are unflagging this, and ask for comments. We should be kind in doing this, as we will likely lower the usage of those tools. We failed at this when we shipped fetch().

  5. Verify if this would ever work for frontend metaframeworks and identify if we need to make changes to our design now.

ljharb commented 2 days ago

For number 3, it would be amazing to have a tool (published under @nodejs/ on npm, perhaps?) that can lint a tsconfig and tell you when it's incompatible with the node flag.

marco-ippolito commented 1 day ago

Created a checklist feel free to edit/suggest points

pi0 commented 1 day ago

Hi, I'm the maintainer of unjs/jiti (used by Nuxt, Nitro, ESLint, ...). A while ago, we released v2, which is being widely adopted.

In v2, we have enabled a new native mode (opt-in) that depends on runtime type-stripping and only adds a few resolutions add-ons mainly to ease up tooling transition to full-native support and eventually phasing out the need of custom loader. (this feature being backported to supported Node.js releases, certainly speeds this up for us!)

We also run tests against Node.js with --experimental-strip-types flag for native mode. You can see ignored tests here mainly failing because in jiti, we tried to maximize ESM<>CJS interop support, and in a more strict system of Node.js/ESM they fail -- I think it is an acceptable price and right thing to do!

One other note to add, jiti does not read or depend on tsconfig.json, it fully depends on Node.js module resolution, and package.json can only affect resolution/behavior.

ematipico commented 1 day ago

Astro isn't currently ready to test the new flag, unfortunately. The CLI binary entry point is a JavaScript file (not a TS file), and all our internal modules are pulled from the dist/ folder, and they all are .js specifiers; this means that we would need to change entirely how our codebase imports the modules.

As a quick test, I renamed our binary to .ts and added a few types, and it works, however it's just one file 😅

ggoodman commented 1 day ago

Since TypeScript shipped support (via transpilation) for Explicit Resource Management, the --experimental-strip-types also has a runtime helper and syntax gap for anyone using that feature.

I'm not aware of any way to disable support for using via tsconfig.json such that popular editors using the TypeScript language services would be proactive in warning users.

Some ideas:

  1. Docs improvement to discuss using and challenges of supporting it.
  2. Docs improvement suggesting lint rules to guard against using.
  3. Code modification in the type-stripping code to explicitly bail out when: 1) it sees using; and 2) the V8 version has no native support for the syntax.
marco-ippolito commented 1 day ago

Since TypeScript shipped support (via transpilation) for Explicit Resource Management, the --experimental-strip-types also has a runtime helper and syntax gap for anyone using that feature.

I'm not aware of any way to disable support for using via tsconfig.json such that popular editors using the TypeScript language services would be proactive in warning users.

Some ideas:

  1. Docs improvement to discuss using and challenges of supporting it.
  2. Docs improvement suggesting lint rules to guard against using.
  3. Code modification in the type-stripping code to explicitly bail out when: 1) it sees using; and 2) the V8 version has no native support for the syntax.

That falls in the same category of decorators, Node.js will not polyfill until V8 supports them. We might document it as a generic rule. @kdy1 is it the current swc policy right?

ggoodman commented 1 day ago

@marco-ippolito have you considered something like option 3 that I proposed above for these syntactic issues? I think the benefit of that would be to fail fast while also giving the opportunity to provide a more meaningful and self-describing error.

node --experimental-strip-types -e "(() => { using test = { [Symbol.dispose]() }; })()"
(node:6400) ERROR: --experimental-strip-types is incompatible with the `using` keyword. See: https://nodejs.org/docs/...
marco-ippolito commented 1 day ago

@marco-ippolito have you considered something like option 3 that I proposed above for these syntactic issues? I think the benefit of that would be to fail fast while also giving the opportunity to provide a more meaningful and self-describing error.

That would also mean to time the swc release (or unflag something) with the v8 update so that we don't block users from using (🥁 ) the new feature. Imho probably not worth it as long as we document it, its like running using in plain javascript.

marco-ippolito commented 1 day ago

Astro isn't currently ready to test the new flag, unfortunately. The CLI binary entry point is a JavaScript file (not a TS file), and all our internal modules are pulled from the dist/ folder, and they all are .js specifiers; this means that we would need to change entirely how our codebase imports the modules.

As a quick test, I renamed our binary to .ts and added a few types, and it works, however it's just one file 😅

@ematipico if you need some support, or accept PRs I could help

ggoodman commented 1 day ago

That would also mean to time the swc release (or unflag something) with the v8 update so that we don't block users from using the new feature. Imho probably not worth it

I see what you mean. Perhaps there's an opportunity for swc to inform Node.js that these syntax features were used and allow Node.js to take action conditionally. That would remove the coordination challenge. The trick would be to tweak swc with an api to inform it of the syntax features to track and return when used.

ggoodman commented 1 day ago

Imho probably not worth it as long as we document it, its like running using in plain javascript.

I disagree here. Folks who are likely to be users of this flag are ones who are authoring TypeScript. Part of the benefit of authoring TypeScript is the immediate feedback loop from the language services. For users who are not experts in the nuances of transpilation and type-stripping will not get the early feedback they expect and will be confused.

Anyway, it's a small thing in service of a more delightful user experience. I understand if it creates too much of a lift.

jakebailey commented 1 day ago

Preventing the use of select features because they have a larger downlevel than others seems very much out of the scope of Node.js. If you use these features with strip types (or similar), you aren't even publishing the downleveled code, you're just running it in-memory.

Banning these features seems much more like a personal preference and can be pretty easily done in a linter.

A runtime is just there to run the user's code. A runtime could even choose to use a transpilation approach to implement newer features and the effect would be the same.

ggoodman commented 1 day ago

I agree with both of you on a technical level but feel convinced that the broader user-base that will want to adopt type stripping won't understand what's going on. I'm thinking there's an opportunity to avoid issues being filed across TS and Node.js repos if the docs + implementation adopt a proactive strategy for informing users of these edge cases.

robpalme commented 11 hours ago

I agree with @jakebailey that we probably don't need to take any extra steps to prevent feature usage.

The reason is that Node/Amaro already uses target: "esnext" which disables all downlevelling of JS features. Meaning there will be JS feature parity across *.js and *.ts files in Node.

So it would feel odd to add more error checking that was specific to *.ts files.

Conversely, I would support auditing to ensure any errors are consistent across *.js and *.ts files. We could add a test case to ensure the use of unimplemented JS features result in the same error message.

theoludwig commented 7 hours ago

To clarify, marking it as stable means that instead of --experimental-strip-types, we'll be able to use --strip-types?

Or does that mean the option will not even be necessary, and that users will be able to directly execute node app.ts? Is that even planned, that without options, we can execute TypeScript directly with Node.js? I'm guessing, that it will enable type stripping based on the file extension .ts.

marco-ippolito commented 7 hours ago

To clarify, marking it as stable means that instead of --experimental-strip-types, we'll be able to use --strip-types?

Or does that mean the option will not even be necessary, and that users will be able to directly execute node app.ts? Is that even planned, that without options, we can execute TypeScript directly with Node.js? I'm guessing, that it will enable type stripping based on the file extension .ts.

This plan is for the unflagging: executing node foo.ts without additional flag. While marking as stable means removing the experimental warning.

targos commented 5 hours ago

Marking as stable also means further breaking changes would only happen in major releases.