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

Unflag --experimental-strip-types #17

Open
5 of 6 tasks
marco-ippolito opened this issue Nov 12, 2024 · 27 comments · May be fixed by nodejs/node#56350
Open
5 of 6 tasks

Unflag --experimental-strip-types #17

marco-ippolito opened this issue Nov 12, 2024 · 27 comments · May be fixed by nodejs/node#56350

Comments

@marco-ippolito
Copy link
Member

marco-ippolito commented Nov 12, 2024

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

@nodejs/typescript

@mcollina
Copy link
Member

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

ljharb commented Nov 12, 2024

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

Created a checklist feel free to edit/suggest points

@pi0
Copy link

pi0 commented Nov 13, 2024

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
Copy link

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
Copy link

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

marco-ippolito commented Nov 13, 2024

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
Copy link

ggoodman commented Nov 13, 2024

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

marco-ippolito commented Nov 13, 2024

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

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
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link

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

marco-ippolito commented Nov 14, 2024

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

targos commented Nov 14, 2024

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

@marco-ippolito
Copy link
Member Author

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

Can someone help me pinging the right people? Thanks 🙏🏻

@ematipico
Copy link

There's one thing that isn't clear, how can someone use this flag from a binary? In my case, astro is a CLI, and users wouldn't call it a using node ../path/to/binary.js.

Plus, is it possible to specify a .ts file inside the bin field of the package.json? That's not clear from the docs

@robpalme
Copy link

robpalme commented Dec 6, 2024

My guess here is that the potential Astro CLI use-case is no different to any other CLI use-case for executing TS. If anything, metaframework CLIs might be considered a more sophisticated use-case than the average Node CLI that might want to use this feature. And therefore more of long shot as potential user.

Nevertheless I think the pitch being explored is that when this feature becomes unflagged, during development of the Astro CLI itself, you might be able to simplify your tooling stack and have fewer moving parts by using this feature. If and only if you don't need features outside of basic TS, e.g. no downlevelling, no paths aliasing, no custom preprocessing. It depends on how simple the build process for the Astro CLI is today as to how feasible it is.

My guess is that in production you would not want to use this feature anyway, because ahead-of-time compilation to JS will give your CLI users better performance. Node won't need to compile the CLI before executing it.

So this pitch is all about easing the life of the Astro CLI developer and is not about directly benefitting users of the Astro CLI.

You asked about using the flag from a binary, but that shouldn't matter since this topic is discussing the unflagged case. So you'd get the feature for free.

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Dec 6, 2024

There's one thing that isn't clear, how can someone use this flag from a binary? In my case, astro is a CLI, and users wouldn't call it a using node ../path/to/binary.js.

Plus, is it possible to specify a .ts file inside the bin field of the package.json? That's not clear from the docs

Once the feature is unflagged it should be possibile to specificy a .ts file in the bin field of the package.json, covering your use case.

@karlhorky
Copy link

karlhorky commented Dec 6, 2024

Once the feature is unflagged it should be possibile to specificy a .ts file in the bin field of the package.json, covering your use case.

@marco-ippolito But an Astro installed CLI - both in the case of A) local installation in <project>/node_modules or B) global installation in some node_modules folder - with a .ts entry point would not currently be allowed to run because of banning of type stripping of *.ts in node_modules, right?

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Dec 6, 2024

Once the feature is unflagged it should be possibile to specificy a .ts file in the bin field of the package.json, covering your use case.

@marco-ippolito But an Astro installed CLI - both in the case of A) local installation in <project>/node_modules or B) global installation in some node_modules folder - with a .ts entry point would not currently be allowed to run because of banning of type stripping of *.ts in node_modules, right?

I haven't tested it yet, it's a symlink, so it might work locally (npm link)🤔 For now, the rule of thumb is that if its published on npm it should be transpiled.

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Dec 16, 2024

I removed the block for nodejs/node#54645
Both Node and SWC put effort into the investigation but since it's an exotic architecture and it only happens in a very specific circumstance (in a custom .ts loader), I'd wait until someone interested in the platform (windows arm64) that has a machine, volunteers to fix it.
EDIT: it seems the bug has disappeared nodejs/node#54657

@kdy1
Copy link
Member

kdy1 commented Dec 16, 2024

Oh I forgot to ask my sister for borrowing her arm notebook.
I’ll see if it would be possible tomorrow

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Dec 16, 2024

I found a possible issue with the unflagging: --eval will always emit the warning and transpile the input if --experimental-strip-types is enabled by default.
Created a PR to fix this nodejs/node#56273

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.