-
Notifications
You must be signed in to change notification settings - Fork 31.2k
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
[v22.x backport] module: unflag --experimental-strip-types #57298
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:
|
PR-URL: nodejs#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]>
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>
PR-URL: nodejs#57359 Fixes: nodejs#56546 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
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?) |
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. |
<!-- Thank you for submitting a pull request! We appreciate the time and effort you have invested in making these changes. Please ensure that you provide enough information to allow others to review your pull request. Upon submission, your pull request will be automatically assigned with reviewers. If you want to learn more about contributing to this project, please visit: https://github.com/lynx-family/lynx-stack/blob/main/CONTRIBUTING.md. --> ## Summary <!-- Can you explain the reasoning behind implementing this change? What problem or issue does this pull request resolve? --> Node.js v23.6 has unflagged the `--experimental-strip-types` feature (nodejs/node#56350). Instead of relying solely on this flag, it’s recommended to also check the standard `process.feature.typescript` to confirm native TypeScript support. > [!NOTE] > It will be backport to v22 soon. See: nodejs/node#57298 <!-- It would be helpful if you could provide any relevant context, such as GitHub issues or related discussions. --> ## Checklist <!--- Check and mark with an "x" --> - [x] Tests updated (or not required). - [x] Documentation updated (or not required).
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! |
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]
Just making sure no blockers
It requires #57121 to be landed together since it contains an important fix, and I want to be as stable as possible before landing.
Technically #57121 needs to land in v23 first and wait two weeks.
Next v23 is planned for 2025-03-11 and the next v22 is 2025-03-18 which is not enough time between the two.
So probably needs to wait 2025-04-15
@nodejs/tsc for fyi
I don't think we caused any breakage when unflagging in v23.