Skip to content

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

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

Open
wants to merge 7 commits into
base: v22.x-staging
Choose a base branch
from

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Mar 3, 2025

PR-URL: #56350
Fixes: nodejs/typescript#17
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Mohammed Keyvanzadeh mohammadkeyvanzade94@gmail.com
Reviewed-By: Paolo Insogna paolo@cowtech.it
Reviewed-By: Pietro Marchini pietro.marchini94@gmail.com

@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels Mar 3, 2025
@marco-ippolito marco-ippolito added needs-citgm PRs that need a CITGM CI run. strip-types Issues or PRs related to strip-types support labels Mar 3, 2025
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@marco-ippolito
Copy link
Member Author

Issue with test runner #56546

@B4nan
Copy link

B4nan commented Mar 6, 2025

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

marco-ippolito commented Mar 6, 2025

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

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

B4nan commented Mar 6, 2025

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

marco-ippolito commented Mar 6, 2025

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 😞

B4nan added a commit to mikro-orm/mikro-orm that referenced this pull request Mar 7, 2025
This check was problematic because of Node.js type stripping uses it now too, and it does fail to parse decorators.

Related to nodejs/node#57298 (comment)
B4nan added a commit to mikro-orm/mikro-orm that referenced this pull request Mar 7, 2025
This check was problematic because of Node.js type stripping uses it now
too, and it does fail to parse decorators.

It was already removed from v7, but since the type stripping will be
backported to Node.js 22, I am removing it from v6 too, otherwise things
break when executing the compiled code.

Related to
nodejs/node#57298 (comment)
github-merge-queue bot pushed a commit to rustymotors/server that referenced this pull request Mar 7, 2025
This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@mikro-orm/core](https://mikro-orm.io)
([source](https://redirect.github.com/mikro-orm/mikro-orm)) | [`^6.4.8`
->
`^6.4.9`](https://renovatebot.com/diffs/npm/@mikro-orm%2fcore/6.4.8/6.4.9)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@mikro-orm%2fcore/6.4.9?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@mikro-orm%2fcore/6.4.9?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@mikro-orm%2fcore/6.4.8/6.4.9?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@mikro-orm%2fcore/6.4.8/6.4.9?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
| [@mikro-orm/postgresql](https://mikro-orm.io)
([source](https://redirect.github.com/mikro-orm/mikro-orm)) | [`^6.4.8`
->
`^6.4.9`](https://renovatebot.com/diffs/npm/@mikro-orm%2fpostgresql/6.4.8/6.4.9)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@mikro-orm%2fpostgresql/6.4.9?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@mikro-orm%2fpostgresql/6.4.9?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@mikro-orm%2fpostgresql/6.4.8/6.4.9?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@mikro-orm%2fpostgresql/6.4.8/6.4.9?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
| [@mikro-orm/reflection](https://mikro-orm.io)
([source](https://redirect.github.com/mikro-orm/mikro-orm)) | [`^6.4.8`
->
`^6.4.9`](https://renovatebot.com/diffs/npm/@mikro-orm%2freflection/6.4.8/6.4.9)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@mikro-orm%2freflection/6.4.9?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@mikro-orm%2freflection/6.4.9?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@mikro-orm%2freflection/6.4.8/6.4.9?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@mikro-orm%2freflection/6.4.8/6.4.9?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>mikro-orm/mikro-orm (@&#8203;mikro-orm/core)</summary>

###
[`v6.4.9`](https://redirect.github.com/mikro-orm/mikro-orm/blob/HEAD/CHANGELOG.md#649-2025-03-07)

[Compare
Source](https://redirect.github.com/mikro-orm/mikro-orm/compare/v6.4.8...v6.4.9)

##### Bug Fixes

- **core:** ensure correct alias is used in complex join conditions
([328c809](https://redirect.github.com/mikro-orm/mikro-orm/commit/328c8097f690056ec188a1e954162e04fc7bd442)),
closes
[#&#8203;6484](https://redirect.github.com/mikro-orm/mikro-orm/issues/6484)
- **core:** fix type of virtual entity `expression` callback
([a13a8a0](https://redirect.github.com/mikro-orm/mikro-orm/commit/a13a8a0c91bc0e51125d5e39e22ec038c0c56399)),
closes
[#&#8203;6481](https://redirect.github.com/mikro-orm/mikro-orm/issues/6481)
- **core:** skip `convertToDatabaseValueSQL` for missing values
([63b028b](https://redirect.github.com/mikro-orm/mikro-orm/commit/63b028b05bfc5810f87046947cc74da097dc01e7)),
closes
[#&#8203;6470](https://redirect.github.com/mikro-orm/mikro-orm/issues/6470)
- **core:** skip TS support detection via `require.extensions`
([#&#8203;6488](https://redirect.github.com/mikro-orm/mikro-orm/issues/6488))
([3efdcd0](https://redirect.github.com/mikro-orm/mikro-orm/commit/3efdcd0a00d038b2eb24a668329f4b1cea46b2a2)),
closes
[/github.com/nodejs/node/pull/57298#issuecomment-2703430792](https://redirect.github.com//github.com/nodejs/node/pull/57298/issues/issuecomment-2703430792)
- **schema:** support indexes on inlined embeddables
([6689c02](https://redirect.github.com/mikro-orm/mikro-orm/commit/6689c02bae207a7648a4fb356cd3aa4212dd0796)),
closes
[#&#8203;6469](https://redirect.github.com/mikro-orm/mikro-orm/issues/6469)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these
updates again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/rustymotors/server).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4xODUuNCIsInVwZGF0ZWRJblZlciI6IjM5LjE4NS40IiwidGFyZ2V0QnJhbmNoIjoiZGV2IiwibGFiZWxzIjpbXX0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@marco-ippolito marco-ippolito marked this pull request as ready for review March 17, 2025 08:54
@marco-ippolito marco-ippolito added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-citgm PRs that need a CITGM CI run. labels Mar 20, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 20, 2025
@nodejs-github-bot
Copy link
Collaborator

@legendecas
Copy link
Member

legendecas commented Mar 20, 2025

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.

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Mar 20, 2025

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

@legendecas
Copy link
Member

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

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

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Jun 9, 2025

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

@hardfist
Copy link
Contributor

hardfist commented Jun 9, 2025

@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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the widespread ecosystem impact, I don't think we can safely backport this - I thought we could.

I think we should revisit this later on.

@marco-ippolito
Copy link
Member Author

I created an issue to track the libraries that break and try to fix them nodejs/typescript#37

Ethan-Arrowood
Ethan-Arrowood previously approved these changes Jun 9, 2025
Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should've read the thread first! I'm generally in favor of this, but I understand its causing issues in the ecosystem then we need to reconsider.

@Ethan-Arrowood Ethan-Arrowood dismissed their stale review June 9, 2025 21:03

I should've read the thread first! I'm generally in favor of this, but I understand its causing issues in the ecosystem then we need to reconsider.

marco-ippolito and others added 2 commits June 25, 2025 07:51
PR-URL: nodejs#56350
Fixes: nodejs/typescript#17
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com>
PR-URL: nodejs#58175
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This commit refactors the CommonJS loader to remove TypeScript-specific
extensions from the require.extensions object for compatibility with
libraries that depended on it to initialize extenal TypeScript loaders.

PR-URL: nodejs#58657
Refs: nodejs/typescript#37
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com>
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
PR-URL: nodejs#58643
Refs: nodejs/typescript#24
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jordan Harband <ljharb@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
@marco-ippolito
Copy link
Member Author

I'm confident that this can now be backported safely, see nodejs/typescript#37 (comment)

marco-ippolito and others added 3 commits June 25, 2025 08:20
PR-URL: nodejs#57687
Fixes: nodejs#56830
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
PR-URL: nodejs#57687
Fixes: nodejs#56830
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
PR-URL: nodejs#57704
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

(but we should be prepared on a quick revert if this is still breaking)

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@marco-ippolito
Copy link
Member Author

@legendecas are you still blocking on this PR?

@nodejs-github-bot
Copy link
Collaborator

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Jun 30, 2025

It seems the SEA tests that were failing on the last v20 release are also failing on v22 rhel8 💀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. strip-types Issues or PRs related to strip-types support v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.