fix: relocate bundle-mode framework/plugin eggPaths to output node_modules#5972
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 ignored due to path filters (4)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesBundle-mode plugin and framework path re-resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 introduces support for bundle-mode plugin path relocation in the loader and framework. When in bundle mode, rewritten plugin and framework paths are detected and re-resolved to their package entry directories under the output node_modules directory, allowing the manifest-backed loader to locate bundled files. Feedback on these changes suggests improving the robustness of the node_modules path matching in egg_loader.ts by splitting path segments instead of using a broad substring check, and updating the test file imports to use .ts extensions instead of .js for consistency.
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 marker = `node_modules${path.sep}`; | ||
| const nmIdx = realDir.lastIndexOf(marker); | ||
| if (nmIdx !== -1) { | ||
| return path.join(this.options.baseDir, realDir.slice(nmIdx)); | ||
| } |
There was a problem hiding this comment.
To prevent incorrect matches on similarly named directories or files (such as my_node_modules), avoid using broad substring/lastIndexOf checks on node_modules. Instead, split the path and match against exact path segments to find the correct node_modules directory.
| const marker = `node_modules${path.sep}`; | |
| const nmIdx = realDir.lastIndexOf(marker); | |
| if (nmIdx !== -1) { | |
| return path.join(this.options.baseDir, realDir.slice(nmIdx)); | |
| } | |
| const segments = realDir.split(/[\/\\]/); | |
| const nmIdx = segments.lastIndexOf('node_modules'); | |
| if (nmIdx !== -1) { | |
| return path.join(this.options.baseDir, ...segments.slice(nmIdx)); | |
| } |
References
- When filtering file paths, avoid broad substring checks like
includes('node_modules'). Instead, match against exact path segments to prevent incorrect matches on similarly named directories or files.
There was a problem hiding this comment.
Good catch — applied in d63bb51. Switched to exact path-segment matching (realDir.split(/[/\\]/) + segments.lastIndexOf('node_modules'), then path.join(baseDir, ...segments.slice(nmIdx))) so directories like my_node_modules are no longer mistaken for the package-root marker.
| import { EggLoader, ManifestStore } from '../../src/index.js'; | ||
| import { getFilepath } from '../helper.js'; |
There was a problem hiding this comment.
In this repository, TypeScript source and test files should use .ts extensions in import paths to maintain consistency with the existing convention.
| import { EggLoader, ManifestStore } from '../../src/index.js'; | |
| import { getFilepath } from '../helper.js'; | |
| import { EggLoader, ManifestStore } from '../../src/index.ts'; | |
| import { getFilepath } from '../helper.ts'; |
References
- In this repository, use
.tsextensions in import/export paths for TypeScript source files to maintain consistency with the existing convention across source and test files.
There was a problem hiding this comment.
Applied in d63bb51 — imports now use .ts extensions (../../src/index.ts, ../helper.ts) to match the source-file convention.
Deploying egg with
|
| Latest commit: |
b18d14a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c9ea2976.egg-cci.pages.dev |
| Branch Preview URL: | https://fix-bundle-eggpaths-relocate.egg-cci.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/test/loader/bundle_plugin_path.test.ts (1)
1-1: ⚡ Quick winRename this test file to hyphen-case for repo consistency.
Line 1 is in a newly added file named
bundle_plugin_path.test.ts; repo convention requires lowercase filenames with hyphens.As per coding guidelines,
**/*: Keep file names lowercase with hyphens.🤖 Prompt for 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. In `@packages/core/test/loader/bundle_plugin_path.test.ts` at line 1, The test file bundle_plugin_path.test.ts uses underscores in the filename but the repository convention requires lowercase filenames with hyphens. Rename the file from bundle_plugin_path.test.ts to bundle-plugin-path.test.ts to comply with the coding guidelines for file naming across the repository.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/core/test/loader/bundle_plugin_path.test.ts`:
- Line 1: The test file bundle_plugin_path.test.ts uses underscores in the
filename but the repository convention requires lowercase filenames with
hyphens. Rename the file from bundle_plugin_path.test.ts to
bundle-plugin-path.test.ts to comply with the coding guidelines for file naming
across the repository.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0900cbf7-2331-4ff4-9476-e9db0e4fd0d3
📒 Files selected for processing (4)
packages/core/src/loader/egg_loader.tspackages/core/test/fixtures/bundle-plugin-paths/package.jsonpackages/core/test/loader/bundle_plugin_path.test.tspackages/egg/src/lib/egg.ts
Deploying egg-v3 with
|
| Latest commit: |
b18d14a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://47a95293.egg-v3.pages.dev |
| Branch Preview URL: | https://fix-bundle-eggpaths-relocate.egg-v3.pages.dev |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #5972 +/- ##
==========================================
+ Coverage 85.57% 85.58% +0.01%
==========================================
Files 669 669
Lines 19849 19880 +31
Branches 3923 3934 +11
==========================================
+ Hits 16985 17014 +29
- Misses 2475 2477 +2
Partials 389 389 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
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. |
killagu
force-pushed
the
fix/bundle-eggpaths-relocate
branch
from
0355c50 to
b57cfe8
Compare
killagu
force-pushed
the
fix/bundle-eggpaths-relocate
branch
from
b57cfe8 to
d63bb51
Compare
|
Thanks for the reviews — pushed
The docstring-coverage pre-merge warning is on the test/fixture files; the two new loader methods ( |
|
Thanks for the update, @killagu. The switch to exact path-segment matching for |
|
Tip For best results, initiate chat on the files or code changes.
(╯°□°）╯彡🐇 just kidding — 🐰✨ ✏️ Learnings added
|
killagu
force-pushed
the
fix/bundle-eggpaths-relocate
branch
from
d63bb51 to
bf4785f
Compare
|
CI fix ( |
In bundle mode the bundler rewrites `import.meta.dirname` to the bundle output directory rather than the egg package directory, so the framework can no longer locate its `config/*` files. When a bundle store for this app is registered, rebase the framework dir to `<output>/node_modules/egg/dist` so the manifest-backed loader fs resolves the bundled framework config. The rebase is gated twice so it never disturbs non-bundle (or unrelated) apps: - Store ownership: the active bundle store is shared via globalThis across @eggjs/core copies, so only rebase when `bundleStore.baseDir` resolves to this app's `baseDir` (mirroring `ManifestStore.load()`'s gate). A store registered for a different app must not redirect this app's framework path. - Existence: a real bundler output physically copies egg to `<output>/node_modules/egg/dist`, but a bundle-mode app booted without the copied framework (e.g. integration tests that only inject a manifest-backed loaderFS) does not. Fall back to `import.meta.dirname` when the rebased dir is absent — which, when not actually rewritten by a bundler, still points at the real egg package dir. Without a matching bundle store the original `import.meta.dirname` logic applies unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A plugin declared via `definePluginFactory({ path: import.meta.dirname })`
has its `path` rewritten by the bundler to the bundle output dir (= baseDir),
which does not contain the plugin's own files. Detect that artifact and
re-resolve the plugin to its package entry directory, rebased under the
output `node_modules`, so the manifest-backed loader fs finds the bundled
plugin config/extend/app files — mirroring how the framework eggPaths are
rebased.
Built-in framework plugins carry only a `name` (no `package`), so the
re-resolution falls back to the conventional `@eggjs/<name>` package name in
addition to the bare name. The output `node_modules` segment is matched by
exact path segment (not a substring) so directories like `my_node_modules`
are not mistaken for the package-root marker.
Artifact detection is gated on store ownership: the bundle store is shared
via globalThis across @eggjs/core copies, so a path is only treated as a
bundle artifact when the active store's `baseDir` resolves to this app's
`baseDir`. Without a matching bundle store the original `plugin.path`
short-circuit and `#resolvePluginPath` logic apply verbatim.
Adds a fixture with a built-in (name-only) plugin and a package-declared
plugin under a bundle-output `node_modules`, with regression coverage
asserting both resolve to the correct rebased directory, that a store
registered for a different app does not reinterpret this app's paths, and
that non-bundle paths are returned verbatim.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
killagu
force-pushed
the
fix/bundle-eggpaths-relocate
branch
from
bf4785f to
b18d14a
Compare
|
Pushed Root cause: Two real defects fixed:
Verified locally: full |
Motivation
In bundle mode the bundler rewrites
import.meta.dirnameto the bundle output directory rather than the originating package directory. Two path-resolution sites depend onimport.meta.dirnameand therefore break under bundling:config/*—EggApplicationCore.customEggPaths()derives the framework dir fromimport.meta.dirname, which now points at the output dir instead of theeggpackage.definePluginFactory({ path: import.meta.dirname })carries apathrewritten to the output dir (=baseDir), which does not contain the plugin's files.Root cause is unified: bundler-rewritten paths must be rebased under the output
node_modules, where the manifest-backed loader fs (keyed relative to the outputbaseDir) can resolve the bundled assets.Scope
packages/egg/src/lib/egg.ts(fix(egg)) — under a registered bundle store,customEggPaths()returns<bundleStore.baseDir>/node_modules/egg/distahead ofsuper.customEggPaths().packages/core/src/loader/egg_loader.ts(fix(core)) —getPluginPath()detects the bundle-rewritten artifact (#isBundlePluginPathArtifact: path equalsbaseDirwhile a bundle store is registered) and re-resolves via#resolveBundlePluginPath, which resolves the plugin package entry directory and rebases it under the outputnode_modules. Built-in plugins carry only aname, so resolution falls back to@eggjs/<name>in addition to the bare name.The two commits are split by package, so a reviewer can land them independently if preferred (the higher-risk loader change in
corecarries the fixture + tests).Non-bundle safety
Every new branch is gated on
ManifestStore.getBundleStore(). When no bundle store is registered, both files fall through to the original, byte-for-byte unchanged logic (import.meta.dirnamefor the framework dir; theplugin.pathshort-circuit and#resolvePluginPathfor plugins). This is asserted explicitly in the new tests.Test evidence
New fixture
packages/core/test/fixtures/bundle-plugin-paths/simulates a bundle outputnode_modulescontaining a built-in (name-only) plugin and apackage-declared plugin. New suitepackages/core/test/loader/bundle_plugin_path.test.ts:<baseDir>/node_modules/@eggjs/bundle-builtin/dist<baseDir>/node_modules/bundle-plugin-pkg/distbaseDir) is returned verbatim@eggjs/coreandeggtypecheck (tsgo --noEmit),oxlint --type-aware, andoxfmt --checkall pass on the changed files.🤖 Generated with Claude Code
Summary by CodeRabbit
pathequals the appbaseDir) are re-resolved to the runtimenode_moduleslocation.<bundle baseDir>/node_modules/egg/distwhen present; otherwise falling back to existing behavior.name,name+package, and cross-app bundle-store non-impact), plus non-bundle behavior coverage.