-
-
Notifications
You must be signed in to change notification settings - Fork 32k
[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
base: v22.x-staging
Are you sure you want to change the base?
[v22.x backport] module: unflag --experimental-strip-types #57298
Conversation
Review requested:
|
37874fb
to
3e34c66
Compare
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
Issue with test runner #56546 |
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 |
please open an issue with a minimal repro, it's very hard to tell whats going on |
Enabling the new |
My issue is running a compiled code (via 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 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. All files you are matching:
Without experimental-strip-types you are matching:
I think your issue is 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, |
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)
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)
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) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](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) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](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) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>mikro-orm/mikro-orm (@​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 [#​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 [#​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 [#​6470](https://redirect.github.com/mikro-orm/mikro-orm/issues/6470) - **core:** skip TS support detection via `require.extensions` ([#​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 [#​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>
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 |
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'd prefer to hold on release on v22 to address #57298 (comment).
I'm not sure I understand, what are the actionable items to do in Node.js? |
Re this assumption in the OP: should this be considered a breakage in existing workflows in v23? |
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 / What would you like us to change in Mocha? 🙂 (do you want to file an issue on Mocha?) |
The issue comes from rechoir, this was the maintainer answer:
🤷🏼 not sure what to do about it but they seem not care. I'd suggest stop using it |
34281cc
to
de0dcee
Compare
@marco-ippolito I wonder why Node.js choose to register |
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.
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.
I created an issue to track the libraries that break and try to fix them nodejs/typescript#37 |
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 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.
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.
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>
de0dcee
to
0d38583
Compare
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>
I'm confident that this can now be backported safely, see nodejs/typescript#37 (comment) |
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>
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
(but we should be prepared on a quick revert if this is still breaking)
@legendecas are you still blocking on this PR? |
It seems the SEA tests that were failing on the last v20 release are also failing on v22 rhel8 💀 |
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