[v22.x backport] module: unflag --experimental-strip-types

[marco-ippolito]

PR-URL: #56350
Fixes: nodejs/typescript#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]

[nodejs-github-bot]

Review requested:

• @nodejs/config
• @nodejs/performance
• @nodejs/test_runner
• @nodejs/typescript

[mcollina]

lgtm

[marco-ippolito]

Issue with test runner #56546

[B4nan]

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 #54901 (comment), the repro is technically there, but not really minimal. I will try to find some time to get an isolated reproduction soon.

[marco-ippolito]

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 #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 🫤

[JakobJingleheimer]

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 #54901 (comment), 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.

[B4nan]

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.

[marco-ippolito]

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 😞

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/65844/

[legendecas]

This breaks some of the workflows with v23, based on mocha + ts-node: open-telemetry/opentelemetry-js#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.

[legendecas]

I'd prefer to hold on release on v22 to address #57298 (comment).

[marco-ippolito]

I'm not sure I understand, what are the actionable items to do in Node.js?

[legendecas]

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

[JoshuaKGoldberg]

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

Yeah, that area of Mocha has had some discussions recently (mochajs/mocha#5235, mochajs/mocha#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?)

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67825/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67830/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67831/

[marco-ippolito]

Landed in d649e9a...4af5915