feat(bundler): remove @eggjs/* from auto-externals#5889
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughDocumentation update clarifying ChangesExternalsResolver Documentation Update
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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 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. Review rate limit: 5/8 reviews remaining, refill in 17 minutes and 29 seconds.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 modifies the ExternalsResolver to stop externalizing ESM-only packages and packages with the @eggjs/ prefix, as Turbopack is capable of bundling ESM natively. The isEsmOnly and hasRequireCondition helper methods were removed as they are no longer needed. One review comment identifies a redundant check for the 'egg' package name, which is already included in a set of always-external names.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Enables bundling of Egg framework packages by removing @eggjs/* from the auto-externalization rules and adjusting resolver behavior/tests accordingly.
Changes:
- Removed
@eggjs/*name-based externalization inExternalsResolver. - Dropped the ESM-only externalization logic (and removed the related private methods).
- Updated
ExternalsResolvertests to assert@eggjs/*and pure-ESM packages are no longer externalized.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/egg-bundler/src/lib/ExternalsResolver.ts | Removes @eggjs/* externalization rule and deletes ESM-only externalization helpers. |
| tools/egg-bundler/test/ExternalsResolver.test.ts | Updates expectations to reflect that ESM-only and @eggjs/* packages are no longer auto-externalized. |
Deploying egg-v3 with
|
| Latest commit: |
6c1d901
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://106a40b3.egg-v3.pages.dev |
| Branch Preview URL: | https://split-18-bundler-remove-eggj.egg-v3.pages.dev |
Deploying egg with
|
| Latest commit: |
6c1d901
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://acf547ea.egg-cci.pages.dev |
| Branch Preview URL: | https://split-18-bundler-remove-eggj.egg-cci.pages.dev |
killagu
force-pushed
the
split/18-bundler-remove-eggjs-externals
branch
from
343d9d9 to
ae7e799
Compare
killagu
force-pushed
the
split/17-egg-bin-bundle-cmd
branch
from
ec61bad to
5fb9bfc
Compare
killagu
force-pushed
the
split/18-bundler-remove-eggjs-externals
branch
from
ae7e799 to
76938be
Compare
killagu
force-pushed
the
split/17-egg-bin-bundle-cmd
branch
from
5fb9bfc to
c987d7c
Compare
killagu
force-pushed
the
split/18-bundler-remove-eggjs-externals
branch
from
ee2340d to
e07d75b
Compare
killagu
force-pushed
the
split/17-egg-bin-bundle-cmd
branch
2 times, most recently
from
d0f4b08 to
c987d7c
Compare
killagu
force-pushed
the
split/18-bundler-remove-eggjs-externals
branch
from
e07d75b to
9065d0c
Compare
killagu
force-pushed
the
split/17-egg-bin-bundle-cmd
branch
2 times, most recently
from
e340159 to
005b155
Compare
killagu
force-pushed
the
split/18-bundler-remove-eggjs-externals
branch
from
9065d0c to
07f9715
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5889 +/- ##
==========================================
- Coverage 85.04% 85.03% -0.02%
==========================================
Files 665 665
Lines 19100 19100
Branches 3716 3716
==========================================
- Hits 16244 16242 -2
- Misses 2463 2465 +2
Partials 393 393 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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 the current code and only fix it if needed.
Inline comments:
In `@tools/egg-bundler/src/lib/Bundler.ts`:
- Around line 196-214: The patching step in Bundler.ts currently rewrites files
(variable patched replaces content for filepath using buildRuntimeExpressions
and buildMetaFull) without updating or removing their stale source maps, so
consumers see incorrect debug locations; modify the code path that runs after
computing patched (where you check if (!urlMatches && metaMatches === 0)
continue and then write patched) to detect when patched !== content and then
either regenerate a correct sourcemap for the modified file or at minimum strip
any inline/sourceMappingURL reference from patched and delete the corresponding
.js.map file (use filepath to locate the .js.map and update write step to remove
or recreate it) so the emitted file no longer points to an out-of-date map.
🪄 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: 82cf3776-e631-4df3-8bca-94351a47cc1c
📒 Files selected for processing (5)
tools/egg-bundler/docs/output-structure.mdtools/egg-bundler/src/lib/Bundler.tstools/egg-bundler/src/lib/ExternalsResolver.tstools/egg-bundler/test/ExternalsResolver.test.tstools/egg-bundler/test/integration.test.ts
There was a problem hiding this comment.
Pull request overview
Updates @eggjs/egg-bundler externals resolution so @eggjs/* (and pure-ESM deps) can be bundled, enabling smaller external dependency sets and better Turbopack compatibility.
Changes:
- Stop auto-externalizing
@eggjs/*packages and remove the now-dead ESM-only detection helpers inExternalsResolver. - Update
ExternalsResolverunit tests and bundler docs to reflect the new bundling policy. - Add a post-build patch step (and an integration test) to rewrite Turbopack’s throwing
import.meta.urlgetter and injectdirname/filenamefor nested chunks.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/egg-bundler/src/lib/ExternalsResolver.ts | Removes @eggjs/* auto-external rule and deletes unused ESM-only detection helpers. |
| tools/egg-bundler/test/ExternalsResolver.test.ts | Flips expectations to ensure @eggjs/* and pure-ESM deps are not auto-externalized. |
| tools/egg-bundler/src/lib/Bundler.ts | Adds a post-build output patch to fix Turbopack import.meta.* behavior in emitted chunks. |
| tools/egg-bundler/test/integration.test.ts | Adds integration coverage for the import.meta.url patching behavior across nested chunks. |
| tools/egg-bundler/docs/output-structure.md | Updates documentation to reflect new externals vs bundled package behavior. |
killagu
force-pushed
the
split/18-bundler-remove-eggjs-externals
branch
from
07f9715 to
fb43c0a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/egg-bundler/docs/output-structure.md (1)
60-67: ⚡ Quick winClarify which “other rule” can externalize ESM-only /
@eggjs/*packages.The wording is correct at a high level (“bundled by default”), but “unless another rule externalizes them” is a bit vague. Consider explicitly naming the externalization sources already listed earlier (e.g., peer dependencies,
externals.force, always-external runtime packages likeegg, and native binaries) so readers know exactly what overrides the default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/docs/output-structure.md` around lines 60 - 67, Update the sentence about ESM-only and `@eggjs/`* packages in the documentation so it names the specific externalization sources that can override the default bundling: reference ExternalsResolver behavior and list peer dependencies, the externals.force configuration, always-external runtime packages (e.g., egg), and native addons/native binaries as the “other rules” that can externalize them so readers know exactly which sources can force externalization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/egg-bundler/docs/output-structure.md`:
- Around line 60-67: Update the sentence about ESM-only and `@eggjs/`* packages in
the documentation so it names the specific externalization sources that can
override the default bundling: reference ExternalsResolver behavior and list
peer dependencies, the externals.force configuration, always-external runtime
packages (e.g., egg), and native addons/native binaries as the “other rules”
that can externalize them so readers know exactly which sources can force
externalization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a400e91-3381-4353-abde-1fdbc1dd4ef7
📒 Files selected for processing (3)
tools/egg-bundler/docs/output-structure.mdtools/egg-bundler/src/lib/ExternalsResolver.tstools/egg-bundler/test/ExternalsResolver.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/egg-bundler/src/lib/ExternalsResolver.ts
- tools/egg-bundler/test/ExternalsResolver.test.ts
killagu
force-pushed
the
split/18-bundler-remove-eggjs-externals
branch
from
fb43c0a to
4ff6e74
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates @eggjs/egg-bundler’s externals resolution policy so framework-scoped and ESM-only packages are bundled by default (instead of being auto-externalized), while keeping peerDependency and native-addon externalization behavior.
Changes:
- Remove the
@eggjs/*auto-external rule and stop treating ESM-only packages as always-external inExternalsResolver. - Update
ExternalsResolverunit tests to reflect the new bundling policy. - Update bundler documentation describing which dependencies remain external vs bundled by default.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/egg-bundler/src/lib/ExternalsResolver.ts | Removes @eggjs/*/ESM-only externalization logic; keeps peerDeps + native binary detection + always-external set. |
| tools/egg-bundler/test/ExternalsResolver.test.ts | Flips assertions so ESM-only and @eggjs/* are no longer expected in externals. |
| tools/egg-bundler/docs/output-structure.md | Updates externals documentation to match the new default bundling behavior. |
## Summary - post-process Turbopack output chunks so import.meta.url/dirname/filename resolve to the runtime chunk path - resolve relative or missing process.argv[1] via a worker.js fallback and sanitize emitted chunk paths before code generation - strip stale sourceMappingURL comments, delete patched .js.map files, and re-enumerate output files for bundle-manifest.json ## Scope This PR is intentionally limited to the import.meta output patch. It does not include the #5889 externals policy changes. ## Tests - pnpm --filter @eggjs/egg-bundler test -- integration.test.ts - pnpm --filter @eggjs/egg-bundler typecheck - pnpm --filter @eggjs/egg-bundler lint <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Post-processes built JS to rewrite dynamic import metadata so runtime URL/dirname/filename resolve reliably, validates/sanitizes relative bundle paths before modifying output, and strips inline sourcemap directives to prevent shipping stale maps. Updated bundle manifest and file lists to exclude removed artifacts. * **Tests** * Added integration tests covering chunk post-processing, runtime metadata behavior and fallbacks, path validation, and cleanup/removal of stale sourcemap files and manifest entries. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
killagu
force-pushed
the
split/18-bundler-remove-eggjs-externals
branch
from
22bbebd to
6c1d901
Compare
| This includes the user's `externals.force` list, root `peerDependencies`, root | ||
| `optionalDependencies`, and native addons/native binaries. They must be |
| This includes the user's `externals.force` list, root `peerDependencies`, root | ||
| `optionalDependencies`, and native addons/native binaries. They must be | ||
| installed alongside the bundle — typically by copying the app's `package.json` |
Removes
@eggjs/*fromExternalsResolver's always-external list and thestartsWith('@eggjs/')check. Keeps native addon/peerDeps detection intact. ESM-only packages are now bundled instead of externalized, because Turbopack can bundle ESM natively while emitting CJSrequire()for external ESM-only packages would fail.Also cleans up the dead
#isEsmOnlyand#hasRequireConditionprivate methods (fixes typecheck error present in #5863). Flips ExternalsResolver test assertions accordingly.Result: framework packages can now be bundled. cnpmcore E2E externals 76 -> 12.
Part of #5863 split. Tracking: #5871.
Generated with Claude Code
Summary by CodeRabbit
peerDependencies,optionalDependencies, native addons, and forced externals are handled in the build output.