-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
module: mark type stripping as stable #60600
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: mark type stripping as stable #60600
Conversation
|
Review requested:
|
4b39f3b to
8170b1b
Compare
8170b1b to
f34b2a8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #60600 +/- ##
==========================================
- Coverage 88.56% 88.56% -0.01%
==========================================
Files 704 704
Lines 208077 208088 +11
Branches 40084 40083 -1
==========================================
- Hits 184289 184283 -6
- Misses 15826 15831 +5
- Partials 7962 7974 +12
🚀 New features to boost your workflow:
|
| Disables the family autoselection algorithm unless connection options explicitly | ||
| enables it. | ||
|
|
||
| ### `--no-strip-types` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the goal is to keep a stable, long-term flag to disable type stripping? Is this something that users want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a long term goal in the sense I wish there was no need to disable it, but given the ecosystem monkeypatches internal etc and some packages break... it's necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep the disabling flag as experimental? I don't think we gain anything from migrating that to stable, even though we don't forsee getting rid of it just yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean rename it --no-strip-types but keep it experimental?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the thing, I wouldn't rename it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want type stripping to be stable, it should feel weird to disable it. Keeping the experimental in the flag does a good job at that – albeit the word order is a bit off, but IMO that's acceptable. I'm not against adding the alias without experimental in the name, I don't think it sends the right signal is all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda agree with your point of view, the fact that can be disabled it's not something we want to drag for too long and the flag is there only for people who probably are using some library that does some monkeypatching or wrong assumption and it breaks. We have to temporarly provide users a way to disabled, we will eventually get rid of it. So --no-experimental-strip-types stays but at the same time it feels weird to call it experimental, since we mark it as stable. Whats unstable is the way to disable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggested approach is to remove experimental from the name, and in a future semver major, make it a no-op
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to drop the experimental prefix but I am -1 to make it a no-op in the future. That would leave no room for people getting broken to run their code on newer versions of Node.js (I don't think it's just ts-node, the other day I was running the tests of tsx and on 22 there was a regression from 20 that would go away if I disable type stripping).
I don't think having disabling option is weird for a stable feature, we have many disabling flags in Node.js for stable features. Bugs and gotchas happen and people need them to work around whatever issues their setup has with the default behavior. That's fine, the default may work for 99% of the people but there can always be 1% that really have to disable it to be able to run their code at all. For example some time ago I added --disable-wasm-trap-handler because in some environments, the WASM trap handler would make it impossible to run WASM at all. But WASM trap-based bound checks should still be considered stable and perfectly fine to use to provide significant speed up for WASM code. Its design just unfortunately doesn't work in certain niche environments and people have to accept the slowdown for keeping their app functional at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in a future semver major, make it a no-op
If you meant "make --strip-types a no-op, then sure that can be discussed; if you mean "make --no-strip-types a no-op, that would be quite weird, it's not how we usually handles such flags (e.g. --experimental-top-level-await is still accepted, --no-experimental-top-level-await throws).
+1 to drop the experimental prefix
But we're not dropping the experimental one, we're adding a new flag that has the same effect, so now we have two.
Anyway, either way is fine, it's not like the additional flag would be a maintenance burden, and I don't feel strongly about the name.
|
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
| ##### Goal | ||
|
|
||
| The goal is to raise awareness of the Node.js TypeScript Type Stripping in the JavaScript ecosystem. | ||
| The goal is to raise awareness of the Node.js TypeScript Type stripping in the JavaScript ecosystem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Why capitalise "Type" but not "stripping"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk 😆 I've read "Type Stripping", "Type stripping", "type-stripping" not sure which one is correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be "type stripping", personally
/ducks
gurgunday
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
Landed in 8e5c80d |
Refs: nodejs/typescript#24
It's been a while and there are no outstading issues.
The only issue so far is #59364 but it's really not about Node, but the ecosystem around ts-node to adapt to it.
Node v22 and v24 have enough download that I can confidently say that it's safe to mark this feature stable.