fix(bundler): rely on @utoo/pack upstream CJS/ESM require interop fix#5981
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 (3)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughBumps the ChangesEGG-69: Upstream
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 introduces a post-processing patch to the emitted Turbopack runtime in egg-bundler to resolve CJS/ESM require interop issues (EGG-69), specifically unwrapping genuine ESM-only-default modules so that CommonJS require calls can access their members correctly. It also adds comprehensive unit and real-build regression tests. The reviewer suggested making the ESM-only-default check more robust by allowing __esModule to be enumerable when checking for other keys.
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.
| const keys = Object.keys(moduleExports); | ||
| if (keys.length === 1 && keys[0] === 'default') { | ||
| return moduleExports.default; | ||
| } |
There was a problem hiding this comment.
To make the ESM-only-default check more robust, consider allowing __esModule to be enumerable. In some transpiled or custom environments, __esModule might be defined as an enumerable property, which would cause keys.length === 1 to fail even if default is the only actual export. Checking that there are no other keys besides default and __esModule is safer and more defensive.
| const keys = Object.keys(moduleExports); | |
| if (keys.length === 1 && keys[0] === 'default') { | |
| return moduleExports.default; | |
| } | |
| const keys = Object.keys(moduleExports); | |
| const hasOtherKeys = keys.some(key => key !== 'default' && key !== '__esModule'); | |
| if (!hasOtherKeys) { | |
| return moduleExports.default; | |
| } |
There was a problem hiding this comment.
Good catch — applied in 8175d2b8. The unwrap is now gated on "no own key other than default/__esModule" instead of keys.length === 1, so it stays correct even if a transpiler makes __esModule enumerable. Added a regression test covering an ESM-only-default module with an enumerable __esModule marker. (Note: in the Turbopack runtime __esModule is defined non-enumerable, so this is purely defensive.)
There was a problem hiding this comment.
No longer applicable. The PR has been reworked: the root cause is fixed upstream in @utoo/pack >= 1.4.16 (utooland/utoo#3185), so the in-repo _turbopack__runtime.js patch this comment refers to has been removed entirely. The defensive __esModule handling is therefore moot — there is no longer any runtime unwrap code in the bundler.
Deploying egg with
|
| Latest commit: |
dd53171
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://701123e3.egg-cci.pages.dev |
| Branch Preview URL: | https://fix-bundler-cjs-esm-interop.egg-cci.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5981 +/- ##
==========================================
+ Coverage 85.59% 85.91% +0.32%
==========================================
Files 669 669
Lines 19892 19907 +15
Branches 3942 3949 +7
==========================================
+ Hits 17026 17104 +78
+ Misses 2478 2425 -53
+ Partials 388 378 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
dd53171
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4b77fa63.egg-v3.pages.dev |
| Branch Preview URL: | https://fix-bundler-cjs-esm-interop.egg-v3.pages.dev |
killagu
force-pushed
the
fix/bundler-cjs-esm-interop
branch
from
d3cd2d6 to
95de624
Compare
killagu
changed the title
|
Dependency limit exceeded — report not shown. This pull request scan exceeded the 10,000-dependency limit applied to this scan, so the results are incomplete and may be inaccurate. To avoid reporting false positives, Socket has not posted a report. Upgrade your plan to raise the dependency limit and get complete reports, or view the partial scan in the dashboard. Socket is always free for open source. If this is a non-commercial open source project, contact us to request a free Team account. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/egg-bundler/test/cjsEsmInterop.realbuild.test.ts`:
- Line 1: The test file cjsEsmInterop.realbuild.test.ts uses camelCase naming
which violates the coding guidelines requiring all file names to use lowercase
with hyphens. Rename the file from cjsEsmInterop.realbuild.test.ts to
cjs-esm-interop.realbuild.test.ts to comply with the lowercase-with-hyphens
convention. This is a file-level change and does not require code modifications,
just the file name update in your version control system.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d4082ab-b357-4e09-a980-415902568292
📒 Files selected for processing (3)
pnpm-workspace.yamltools/egg-bundler/test/cjsEsmInterop.realbuild.test.tswiki/log.md
✅ Files skipped from review due to trivial changes (1)
- wiki/log.md
killagu
force-pushed
the
fix/bundler-cjs-esm-interop
branch
from
95de624 to
e0f90c1
Compare
The EGG-69 crash (`JSON5.parse is not a function` in a bundled app, where
@utoo/pack resolved a CJS `require('json5')` to json5's ESM default-only
`module` entry) is now fixed upstream in @utoo/pack (utooland/utoo#3185):
>= 1.4.16 resolves a CJS `require()` of such a dual package to its CommonJS
`main`, matching Node's own CommonJS resolution.
Drop the in-repo workaround (the post-build `_turbopack__runtime.js` patch
that unwrapped the lone `default`) and its synthetic-runtime tests, and bump
the `@utoo/pack` catalog range to `^1.4.16`. Add `@utoo/*` to
`minimumReleaseAgeExclude` so the freshly-published fixed version (and its
platform binaries) installs without waiting out the 24h maturity gate, matching
how the rest of the first-party toolchain is treated.
Keep `cjsEsmInterop.realbuild.test.ts` as a real @utoo/pack build regression
guard over a json5-shaped dual fixture (`main`+`module`, ESM exports only
`default`) required from authored CJS via a named member: the bundled worker
runs the CJS implementation with no crash, proving `require('pkg').member`
resolves like Node.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
killagu
force-pushed
the
fix/bundler-cjs-esm-interop
branch
from
e0f90c1 to
dd53171
Compare
Background
A bundled egg app crashed at runtime with
<path>/tsconfig.json is malformed JSON5.parse is not a function, and only started if the user passed--pack-alias json5=<lib/index.js>(EGG-69).Root cause:
@utoo/pack(Turbopack,target: node) resolved the CJSrequire('json5')insidetsconfig-pathsto json5's ESMmoduleentry (dist/index.mjs, default-only).commonJsRequirereturned the ESM namespace{ __esModule: true, default: JSON5 }, soJSON5.parsewasundefined. Node would instead resolve such dual packages to their CommonJSmain.This is already fixed upstream
@utoo/pack>= 1.4.16 fixes the root cause (utooland/utoo#3185): a CJSrequire()of such a dual package now resolves to its CommonJSmain, matching Node's own CommonJS resolution. The real fix lives upstream; this PR does not fix anything itself.An earlier version of this PR carried an in-repo workaround (a post-build patch of
_turbopack__runtime.js). That workaround is now removed in favor of the upstream fix.What this PR actually does
This is hygiene, not a functional fix:
@utoo/packfloor: catalog range^1.2.7→^1.4.16, so the fixed version is the documented minimum and nobody can pin a pre-fix version. (The repo ships no lockfile, so fresh installs already resolve to the latest1.4.x; this just makes the requirement explicit.)tools/egg-bundler/test/cjsEsmInterop.realbuild.test.tsruns a real@utoo/packbuild over a json5-shaped dual fixture (main+module, ESM exports onlydefault, noexportsfield) required from authored CJS via a named member, then executes the bundledworker.js. It prints the CJS implementation's output (cjs:ok) with no crash, provingrequire('pkg').memberresolves like Node. A future@utoo/packregression of that resolution fails this test.Test evidence
cjsEsmInterop.realbuild.test.tspasses against@utoo/pack1.4.16; the same fixture crashes (pkg.parse is not a function) on 1.4.14, confirming the test exercises the real behavior.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Chores