fix(bundler): erase ORM-shadowing class fields via project-dir tsconfig#5979
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthrough
Changestsconfig placement and class-field shadowing fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the egg-bundler to write the compiler tsconfig.json into the build-managed project entry directory instead of the output directory. This ensures that @utoo/pack correctly resolves the configuration, enabling decorator metadata and setting useDefineForClassFields to false to prevent class-field shadowing issues with ORMs like Leoric without modifying the user's own configuration. The feedback recommends adding a safeguard to prevent accidentally overwriting a user's tsconfig.json, resolving rootPath to an absolute path to handle relative paths safely, and using process.execPath instead of 'node' in tests to guarantee the correct Node.js binary is executed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| await fs.mkdir(projectPath, { recursive: true }); | ||
| await fs.writeFile(path.join(projectPath, 'tsconfig.json'), JSON.stringify(COMPILER_TSCONFIG, null, 2)); |
There was a problem hiding this comment.
Writing tsconfig.json directly to projectPath poses a high risk of silently overwriting a user's actual tsconfig.json if projectPath is not a build-managed directory (e.g., if PackRunner is used directly or configured differently). To prevent catastrophic data loss, we should add a safeguard that checks if an existing tsconfig.json was generated by egg-bundler (using a custom marker property) or if the directory is build-managed (under .egg-bundle) before overwriting it.
const tsconfigPath = path.join(projectPath, 'tsconfig.json');
let isUserTsconfig = false;
try {
const content = await fs.readFile(tsconfigPath, 'utf8');
const parsed = JSON.parse(content);
if (parsed && parsed._generatedBy !== 'egg-bundler' && !projectPath.includes('.egg-bundle')) {
isUserTsconfig = true;
}
} catch (err) {
// Ignore file not found or invalid JSON
}
if (isUserTsconfig) {
throw new Error(`Refusing to overwrite user's tsconfig.json at ${projectPath}. Please ensure projectPath is a build-managed directory.`);
}
await fs.mkdir(projectPath, { recursive: true });
await fs.writeFile(tsconfigPath, JSON.stringify({ ...COMPILER_TSCONFIG, _generatedBy: 'egg-bundler' }, null, 2));There was a problem hiding this comment.
Done in 55e6a9a. PackRunner now refuses to overwrite an existing, differing tsconfig.json when projectPath is not a build-managed (.egg-bundle) directory, and throws a clear error instead of silently clobbering a user's config. Idempotent re-writes of our own identical output stay allowed (content-equality check), so no non-standard marker key is added to tsconfig.json.
| // stays at the app baseDir (or a caller-supplied monorepo root) so the | ||
| // app sources and node_modules above the entry dir still resolve. | ||
| projectPath: entries.entryDir, | ||
| rootPath: mergedPack?.rootPath ?? absBaseDir, |
There was a problem hiding this comment.
If mergedPack?.rootPath is configured as a relative path, passing it directly to PackRunner can cause resolution issues depending on the current working directory. It is safer to resolve it relative to absBaseDir to guarantee it is an absolute path.
| rootPath: mergedPack?.rootPath ?? absBaseDir, | |
| rootPath: mergedPack?.rootPath ? path.resolve(absBaseDir, mergedPack.rootPath) : absBaseDir, |
There was a problem hiding this comment.
Done in 55e6a9a — rootPath is now resolved with path.resolve(absBaseDir, mergedPack.rootPath) so a relative value no longer depends on cwd.
|
|
||
| // Runtime proof: executing the bundle, the setter ran and the column is | ||
| // present in toSQLValues() (the INSERT payload). | ||
| const { stdout } = await execFileAsync('node', [path.join(outputDir, 'worker.js')], { cwd: outputDir }); |
There was a problem hiding this comment.
Using 'node' directly in execFileAsync relies on the system PATH and might execute a different Node.js binary than the one running the tests. Using process.execPath is more robust and guarantees cross-platform consistency.
| const { stdout } = await execFileAsync('node', [path.join(outputDir, 'worker.js')], { cwd: outputDir }); | |
| const { stdout } = await execFileAsync(process.execPath, [path.join(outputDir, 'worker.js')], { cwd: outputDir }); |
There was a problem hiding this comment.
Done in 55e6a9a — switched to process.execPath.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5979 +/- ##
=======================================
Coverage 85.57% 85.57%
=======================================
Files 669 669
Lines 19849 19849
Branches 3923 3923
=======================================
Hits 16985 16985
Misses 2475 2475
Partials 389 389 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Deploying egg with
|
| Latest commit: |
0396df0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://df68a1f2.egg-cci.pages.dev |
| Branch Preview URL: | https://fix-bundler-class-field-shad.egg-cci.pages.dev |
Deploying egg-v3 with
|
| Latest commit: |
0396df0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d64a73b2.egg-v3.pages.dev |
| Branch Preview URL: | https://fix-bundler-class-field-shad.egg-v3.pages.dev |
Declared-but-uninitialized TypeScript class fields (e.g. `createdAt: Date;` on a leoric `Bone` model) compiled with `useDefineForClassFields: true` become native own `undefined` properties at runtime. They shadow the get/set accessors ORMs install on the prototype for decorated attributes, so writes never reach the setter and the column is silently dropped on INSERT (observed as missing `gmt_create`). The bundle must compile these with `useDefineForClassFields: false` (matching how Egg apps build with target <= ES2021) so the bare declarations are erased and the prototype accessors stay effective. Placing that flag is subtle: @utoo/pack (Turbopack) resolves the governing tsconfig from the PROJECT dir (the `projectPath` passed to `build()`), NOT the output dir and NOT via per-file find-up to the nearest tsconfig. Verified against @utoo/pack 1.4.13/1.4.14 with an isolated repro: a tsconfig in the output dir is ignored, and one nearer the source than projectPath is ignored — only `projectPath/tsconfig.json` wins. (See utooland/utoo#2450, confirmed and closed COMPLETED: the option works; only placement matters.) Bundler passed `projectPath = app baseDir`, whose own tsconfig extends @eggjs/tsconfig (target ES2024 → useDefineForClassFields defaults to true), so the field survived. Fix: - PackRunner writes the compiler tsconfig (experimentalDecorators, emitDecoratorMetadata, target, useDefineForClassFields:false) into the PROJECT dir instead of the output dir. It refuses to overwrite an existing, differing tsconfig.json when projectPath is not a build-managed (.egg-bundle) directory, so a user's own tsconfig is never silently clobbered. - Bundler passes `projectPath = the generated entry dir` (build-managed `.egg-bundle/entries`) and `rootPath = app baseDir` (a caller-supplied rootPath is resolved against baseDir), so the compiler tsconfig is the one @utoo/pack resolves; the app's own tsconfig is neither read nor overwritten, and app sources + node_modules above the entry dir still resolve. Verified with a real @utoo/pack build (decorator-free leoric-like model, so no @swc/helpers needed): the bare field is erased, the prototype setter side effect survives (`toSQLValues()` keeps the column), decorator metadata (design:type) is still emitted, external node_modules deps still resolve/inline, and the app's own tsconfig is left untouched. A real-build regression test pins this so a future @utoo/pack upgrade or a tsconfig-placement regression is caught. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
killagu
force-pushed
the
fix/bundler-class-field-shadowing
branch
from
55e6a9a to
0396df0
Compare
Motivation
Declared-but-uninitialized TypeScript class fields (e.g.
createdAt: Date;on a leoricBonemodel) compiled withuseDefineForClassFields: truebecome native ownundefinedproperties at runtime. They shadow the get/set accessors ORMs install on the prototype for decorated attributes, so writes never reach the setter and the column is silently dropped on INSERT (observed as a missinggmt_create).The bundle must compile these with
useDefineForClassFields: false(matching how Egg apps build withtarget <= ES2021) so the bare declarations are erased and the prototype accessors stay effective.Why the obvious placement does not work
@utoo/pack(Turbopack) resolves the governing tsconfig from the project dir (theprojectPathpassed tobuild()) — not the output dir, and not via per-file find-up to the nearest tsconfig. Verified against@utoo/pack1.4.13/1.4.14 with an isolated repro:projectPath/tsconfig.jsonThis is not an upstream bug — see utooland/utoo#2450 (confirmed and closed COMPLETED): the option works, only placement matters. Bundler passed
projectPath = app baseDir, whose own tsconfig extends@eggjs/tsconfig(target: ES2024→useDefineForClassFieldsdefaults totrue), so the field survived.Scope
experimentalDecorators,emitDecoratorMetadata,target,useDefineForClassFields:false) into the project dir instead of the output dir.projectPath = the generated entry dir(build-managed.egg-bundle/entries) androotPath = app baseDir, so the compiler tsconfig is the one@utoo/packresolves; the app's own tsconfig is neither read nor overwritten, and app sources +node_modulesabove the entry dir still resolve.Diff is limited to
tools/egg-bundler.Test evidence
A real
@utoo/packbuild regression test (decorator-free leoric-like model, so no@swc/helpersneeded) asserts:toSQLValues()keeps the column (runtime proof by executing the bundle undernode);It is intentionally a real build so a future
@utoo/packupgrade or a tsconfig-placement regression is caught (a mock cannot). Unit/integration tests are updated for the new tsconfig location and a guard pinsprojectPath = entry dir.pnpm --filter=@eggjs/egg-bundler run testis green (local-only macOS/private/varsymlink failures inEntryGenerator/ManifestLoaderare pre-existing and unrelated to this change).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
createdAtgetter/setter behavior.tsconfig.jsonis generated in the project/entry directory, includes the expected compiler options (includinguseDefineForClassFields: false), and avoids clobbering an existing configuration unexpectedly.Tests / Documentation
tsconfig.jsonlocation and pre-write ordering.