nodejs
[v22.x backport] module: unflag --experimental-strip-types
PR-URL: https://github.com/nodejs/node/pull/56350 Fixes: https://github.com/nodejs/typescript/issues/17 Reviewed-By: Matteo Collina [email protected] Reviewed-By: Mohammed Keyvanzadeh [email protected] Reviewed-By: Paolo Insogna [email protected] Reviewed-By: Pietro Marchini [email protected]
Review requested:
- [ ] @nodejs/config
- [ ] @nodejs/performance
- [ ] @nodejs/test_runner
- [ ] @nodejs/typescript
I still haven't had the time to create an isolated repro, but the type stripping feature broke things for us, executing compiled code via tsc no longer works on node 23 unless I explicitly disable the type stripping. We use experimentalDecorators and compile to ESM in there if that matters. The problem is triggered when dynamically importing the compiled JS files (so no TS involved there). Already mentioned in https://github.com/nodejs/node/issues/54901#issuecomment-2660860322, the repro is technically there, but not really minimal. I will try to find some time to get an isolated reproduction soon.
I still haven't had the time to create an isolated repro, but the type stripping feature broke things for us, executing compiled code via
tscno longer works on node 23 unless I explicitly disable the type stripping. We useexperimentalDecoratorsand compile to ESM in there if that matters. The problem is triggered when dynamically importing the compiled JS files (so no TS involved there). Already mentioned in #54901 (comment), the repro is technically there, but not really minimal. I will try to find some time to get an isolated reproduction soon.
please open an issue with a minimal repro, it's very hard to tell whats going on I see you have decorators enabled 🫤
I still haven't had the time to create an isolated repro, but the type stripping feature broke things for us, executing compiled code via
tscno longer works on node 23 unless I explicitly disable the type stripping. We useexperimentalDecoratorsand compile to ESM in there if that matters. The problem is triggered when dynamically importing the compiled JS files (so no TS involved there). Already mentioned in https://github.com/nodejs/node/issues/54901#issuecomment-2660860322, the repro is technically there, but not really minimal. I will try to find some time to get an isolated reproduction soon.
Enabling the new erasableSyntaxOnly (introduced in 5.8) in tsconfig may help you pinpoint the cause.
Enabling the new erasableSyntaxOnly (introduced in 5.8) in tsconfig may help you pinpoint the cause.
My issue is running a compiled code (via tsc). It's already JS, I don't see how this would help me. I do use stuff that erasableSyntaxOnly disallows, I do not plan to use the native TS support in node, what I care about is being able to still use node on the compiled output from tsc, that is what broke.
I already found the part that breaks, it is an optional symbol property, it's not about decorators.
const OptionalProps = Symbol('OptionalProps');
abstract class BaseEntity<Optional = never> {
[OptionalProps]?: 'createdAt' | 'updatedAt' | Optional;
}
This gets compiled (by tsc) into:
const OptionalProps = Symbol('OptionalProps');
class BaseEntity {
[OptionalProps]; // still here and breaks when running via `node` 23, works if type stripping is disabled
}
This is happening when dynamically importing this compiled JS file. The code is clearly valid JS, I can run it in node as well as browsers.
I will try to wrap this up in the evening and create the issue. Sorry for the noise here, but this will affect pretty much all of our users, so enabling this flag is quite a BC for us.
I found your error:
// node_modules/@mikro-orm/core/metadata/MetadataDiscovery.js
// PATH: /Users/marcoippolito/Documents/projects/test/guide/src/modules/common/base.entity.ts NAME: Base
const targets = await this.getEntityClassOrSchema(path, name);
You are doing a dynamic import of ts with decorators.
base.entity.ts has decorators inside, there is definitely a bug in the logic of your application in
All files you are matching:
filepath [
'./src/modules/common/base.entity.ts',
'./src/modules/article/article-listing.entity.ts',
'./src/modules/article/article.entity.ts',
'./src/modules/article/comment.entity.ts',
'./src/modules/article/tag.entity.ts',
'./src/modules/user/user.entity.ts'
]
Without experimental-strip-types you are matching:
filepath [
'./dist/modules/article/article-listing.entity.js',
'./dist/modules/article/article.entity.js',
'./dist/modules/article/comment.entity.js',
'./dist/modules/article/tag.entity.js',
'./dist/modules/common/base.entity.js',
'./dist/modules/user/user.entity.js'
]
I think your issue is detectTsNode is set to true, so it will use the .ts glob to find files.
static detectTsNode() {
/* istanbul ignore next */
return process.argv[0].endsWith('ts-node') // running via ts-node directly
// @ts-ignore
|| !!process[Symbol.for('ts-node.register.instance')] // check if internal ts-node symbol exists
|| !!process.env.TS_JEST // check if ts-jest is used (works only with v27.0.4+)
|| !!process.env.VITEST // check if vitest is used
|| !!process.versions.bun // check if bun is used
|| process.argv.slice(1).some(arg => arg.includes('ts-node')) // registering ts-node runner
|| process.execArgv.some(arg => arg === 'ts-node/esm') // check for ts-node/esm module loader
|| (require.extensions && !!require.extensions['.ts']); // check if the extension is registered
}
|| (require.extensions && !!require.extensions['.ts']); // check if the extension is registered
There we go, this is your bug, .ts is registered by type-stripping which behaves differently from ts-node.
You really should not rely on a deprecated property since v0.10.6 😞
This breaks some of the workflows with v23, based on mocha + ts-node: https://github.com/open-telemetry/opentelemetry-js/issues/5415
Existing ts projects can be written in module syntax for static analysis and transpiled to CJS. These workflows assume that these TS files are treated as CJS. However, in this case, mocha will try to import a .ts file first, and try require if import fails https://github.com/mochajs/mocha/blob/b0c269616e775689f4f28eedc0a9c5e99048139b/lib/nodejs/esm-utils.js#L37-L53, and a default enabled ts loader takes precedence over user configuered CJS loader.
I'm not sure I understand, what are the actionable items to do in Node.js?
@marco-ippolito: I don't think we caused any breakage when unflagging in v23.
Re this assumption in the OP: should this be considered a breakage in existing workflows in v23?
mocha will try to
importa.tsfile first, and tryrequireifimportfails https://github.com/mochajs/mocha/blob/b0c269616e775689f4f28eedc0a9c5e99048139b/lib/nodejs/esm-utils.js#L37-L53, and a default enabled ts loader takes precedence over user configuered CJS loader
Yeah, that area of Mocha has had some discussions recently (https://github.com/mochajs/mocha/issues/5235, https://github.com/mochajs/mocha/pull/5294). It predates all of the current maintenance team -myself included- as well as Node.js type stripping / .ts support.
What would you like us to change in Mocha? 🙂 (do you want to file an issue on Mocha?)
Hi @marco-ippolito, just a FYI, I'll issue a v22 release soon this week, and this PR might not go due to red-CI.
Will there be a companion PR for updating this guide on the Node.js website for when this backport is released? I'd be happy to contribute one if desired!
Will there be a companion PR for updating this guide on the Node.js website for when this backport is released? I'd be happy to contribute one if desired!
@htunnicliff I think not yet because some LTS lines will still require the flag. (But thank you for the contribution offer!!)
Can you please backport also https://github.com/nodejs/node/pull/58175 in this PR? Be sure to change v23.6.0 with REPLACEME when doing so.
There we go, this is your bug, .ts is registered by type-stripping which behaves differently from ts-node. You really should not rely on a deprecated property since v0.10.6 😞
even require.extensions is deprecated long time ago, it's still heavily used by the ecosystem and nearly all register based solution depends on it(ts-node swc-node and webpack rspack and others which added to over 50M weekly downloads) minimal demo here https://github.com/hardfist/webpack-ts-break)
So it seems it's a big breaking change if this is backported to Node22 and may cause lots of issues.
Thanks for reporting, I think its possible to help migrate those packages to a different check for 'ts-node'. They will have to anyways for Node v24
There we go, this is your bug, .ts is registered by type-stripping which behaves differently from ts-node. You really should not rely on a deprecated property since v0.10.6 😞
even require.extensions is deprecated long time ago, it's still heavily used by the ecosystem and nearly all register based solution depends on it(ts-node swc-node and webpack rspack and others which added to over 50M weekly downloads) minimal demo here https://github.com/hardfist/webpack-ts-break)
So it seems it's a big breaking change if this is backported to Node22 and may cause lots of issues.
The issue comes from rechoir, this was the maintainer answer:
Node TSC and core developers don't care about the ecosystem and constantly try to break it. This is just another reason in a long line of reasons that we ignore you.
🤷🏼 not sure what to do about it but they seem not care. I'd suggest stop using it
@marco-ippolito I wonder why Node.js choose to register require.extensions['.ts'] directly other than detect if user already registered require.extetensions['.ts'] and skip register Node.js builtin ts transformation
I created an issue to track the libraries that break and try to fix them https://github.com/nodejs/typescript/issues/37
I'm confident that this can now be backported safely, see https://github.com/nodejs/typescript/issues/37#issuecomment-3003421042