fix(core): detect class when Function#toString is unreliable#4553
fix(core): detect class when Function#toString is unreliable#4553nobu121 wants to merge 2 commits intomidwayjs:v4-nextfrom
Conversation
Updated the isClass function to correctly identify class constructors even when their source text is obscured by tools like Bytenode. Added tests to validate this behavior, ensuring that classes are accurately recognized regardless of modifications to Function.prototype.toString. Additionally, updated deployment documentation to reflect the importance of this fix when using Bytenode.
There was a problem hiding this comment.
Pull request overview
This PR hardens Midway core’s isClass detection so dependency injection remains reliable when Function.prototype.toString() source text is obscured (e.g., Bytenode single-file bytecode bundles), and documents the Bytenode deployment flow.
Changes:
- Add a fallback in
isClassthat detects ES class constructors via a non-writableprototypedescriptor whentoString()is unreliable. - Extend core util tests to cover class detection when
toString()is obscured. - Add EN/ZH deployment documentation for optionally compiling single-file bundles to Bytenode bytecode.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packages/core/src/util/types.ts |
Adds a prototype descriptor-based fallback to detect classes when toString() is unreliable. |
packages/core/test/decorator/util/index.test.ts |
Adds tests intended to validate the new isClass fallback behavior under obscured toString(). |
site/i18n/en/docusaurus-plugin-content-docs/current/deployment.md |
Documents optional Bytenode bytecode compilation for single-file deployments (EN). |
site/docs/deployment.md |
Documents optional Bytenode bytecode compilation for single-file deployments (ZH). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Function.prototype.toString = function (this: unknown) { | ||
| if (this === ObscuredCtor) { | ||
| return 'function () {}'; | ||
| } | ||
| return origToString.call(this); | ||
| }; | ||
| expect(Types.isClass(ObscuredCtor)).toBeTruthy(); | ||
| } finally { | ||
| Function.prototype.toString = origToString; |
There was a problem hiding this comment.
The test overrides Function.prototype.toString, but Types.isClass calls a cached reference (const ToString = Function.prototype.toString captured when packages/core/src/util/types.ts is first loaded). Since Types is imported at the top of the file, this override won’t affect Types.isClass, so the new fallback branch (prototype descriptor check) is not actually exercised.
To reliably cover the new behavior, patch Function.prototype.toString before importing the module (e.g., via jest.resetModules()/jest.isolateModules() and require after the patch), or otherwise ensure the cached ToString reference used by isClass sees the stubbed behavior.
| Function.prototype.toString = function (this: unknown) { | |
| if (this === ObscuredCtor) { | |
| return 'function () {}'; | |
| } | |
| return origToString.call(this); | |
| }; | |
| expect(Types.isClass(ObscuredCtor)).toBeTruthy(); | |
| } finally { | |
| Function.prototype.toString = origToString; | |
| jest.resetModules(); | |
| Function.prototype.toString = function (this: unknown) { | |
| if (this === ObscuredCtor) { | |
| return 'function () {}'; | |
| } | |
| return origToString.call(this); | |
| }; | |
| let isolatedTypes!: typeof Types; | |
| jest.isolateModules(() => { | |
| isolatedTypes = require('../../../src').Types; | |
| }); | |
| expect(isolatedTypes.isClass(ObscuredCtor)).toBeTruthy(); | |
| } finally { | |
| Function.prototype.toString = origToString; | |
| jest.resetModules(); |
| } | ||
| ``` | ||
|
|
||
| All constraints from the [single-file deployment](#single-file-deployment) section still apply: no default exports in DI-related code, config as object mode, datasource `entities` path scanning limitations, etc. Bytecode is harder to debug; keep an uncompiled build path for troubleshooting. |
There was a problem hiding this comment.
The in-page link target #single-file-deployment doesn’t appear to exist in this document. The relevant section header is ## Single file build deployment, which Docusaurus will slug to #single-file-build-deployment, so this link likely won’t navigate correctly.
| All constraints from the [single-file deployment](#single-file-deployment) section still apply: no default exports in DI-related code, config as object mode, datasource `entities` path scanning limitations, etc. Bytecode is harder to debug; keep an uncompiled build path for troubleshooting. | |
| All constraints from the [single-file deployment](#single-file-build-deployment) section still apply: no default exports in DI-related code, config as object mode, datasource `entities` path scanning limitations, etc. Bytecode is harder to debug; keep an uncompiled build path for troubleshooting. |
|
CI is failing because the isClass fallback added in this PR is too broad. The new fallback treats any function with a non-writable prototype as a class. That works for the Bytenode case, but it also incorrectly classifies native constructors like Function, Object, and Array as classes. Once that happens, MetadataManager.formatTarget() can normalize DTO helper intermediate functions to the wrong target, so metadata is copied onto the wrong place and PickDto / OmitDto end up with no validation rules. That is why validate and validation-zod tests start failing. I verified this locally. Narrowing the fallback so it still covers obscured class constructors but does not misclassify native constructors fixes the regression, and the relevant core, validate, and validation-zod tests pass again. |
The prototype-based fallback incorrectly treated native constructors (Object, Array, Function, etc.) as user-defined classes because they also have non-writable prototype. Add a [native code] check to the fallback condition to skip these. This caused CI failures in validation/validation-zod tests because MetadataManager.formatTarget() would normalize targets incorrectly, leading to lost metadata on PickDto/OmitDto. Also use jest.isolateModules in tests so the toString mock takes effect after module-level caching.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4-next #4553 +/- ##
==========================================
Coverage ? 85.48%
==========================================
Files ? 631
Lines ? 65106
Branches ? 7034
==========================================
Hits ? 55656
Misses ? 9404
Partials ? 46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Need
Single-file bundles compiled with Bytenode can break dependency injection because
isClassrelied onFunction.prototype.toStringmatchingclass.Updating Reason
ECMAScript class constructors expose a non-writable
prototypedata property. Use it as a fallback whentoStringis unreliable (Bytenode replaces source text).Related Testing
pnpm -C packages/core test -- test/decorator/util/index.test.tsFunction.prototype.toStringfor a realclassctor.User Tips
Function.prototype.toStringin app code when using Bytenode.