fix(core): include plugin dynamic files in manifest#5922
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughPost-processes StartupManifest during manifest generation to precompute conventional module resolve aliases and directory-based middleware file-discovery metadata; updates egg-bundler worker entry generation to emit precomputed absolute/resolve-cache alias arrays and call startEgg with the bundle output as runtime baseDir and an explicit framework specifier. ChangesManifest Generation & Bundler Runtime Path Mapping
Sequence Diagram(s)sequenceDiagram
participant Bundler as Bundler (build-time)
participant Worker as Generated Worker (runtime)
participant ManifestStore as ManifestStore
participant Framework as Bundled Framework Module
participant StartEgg as startEgg
Bundler->>Worker: emit worker with __APP_ABSOLUTE_ALIASES & __APP_RESOLVE_CACHE_ALIASES
Worker->>ManifestStore: ManifestStore.fromBundle(__outputDir, bundleMap, aliases)
Worker->>Framework: import * as __frameworkModule from <framework-specifier>
Worker->>StartEgg: startEgg({ baseDir: __outputDir, framework: __framework, mode: 'single' })
StartEgg->>Framework: use __frameworkModule for framework wiring
StartEgg->>ManifestStore: consult precomputed resolveCache/fileDiscovery for module resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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. Comment |
Deploying egg with
|
| Latest commit: |
6a893cb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2a48a1d9.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-7e0d31d1.egg-cci.pages.dev |
Deploying egg-v3 with
|
| Latest commit: |
6a893cb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fc9e5f61.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-7e0d31d1.egg-v3.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5922 +/- ##
==========================================
+ Coverage 85.04% 85.08% +0.03%
==========================================
Files 667 667
Lines 19128 19185 +57
Branches 3725 3743 +18
==========================================
+ Hits 16268 16324 +56
- Misses 2467 2468 +1
Partials 393 393 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request enhances the EggLoader to collect conventional dynamic files—including agent/app boots, extends, and middleware—into the startup manifest, ensuring these entry points are available in bundled environments. It also updates the EntryGenerator in egg-bundler to support complex path mapping and aliasing, allowing bundled applications to resolve modules via relative keys, output-dir absolute paths, and original absolute paths. Feedback was provided to expand the file extension patterns for middleware discovery to include .mjs and .cjs and to verify that the middleware path is a directory before globbing to prevent potential runtime errors.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes bundled-worker startup manifests by ensuring convention-based plugin dynamic files (e.g., agent, app, app/extend/*, app/middleware) are captured even when manifests are generated via metadataOnly startup, so bundled single-mode workers can resolve these files at runtime.
Changes:
- Extend
EggLoader.generateManifest()to additionally record conventional plugin dynamic entrypoints intoresolveCache/fileDiscovery. - Enhance
EntryGenerator’s worker entry to treatoutputDiras runtimebaseDir, passframeworkexplicitly tostartEgg, and precompute alias keys for runtime module lookup (relKey, output-abs, original-app-abs, andresolveCacherequest aliases). - Add/adjust tests, fixtures, snapshots, and documentation to validate and describe the behavior.
Reviewed changes
Copilot reviewed 11 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| wiki/packages/egg-bundler.md | Documents worker runtime path mapping and lookup key strategy. |
| wiki/log.md | Records the documentation update in the wiki log. |
| tools/egg-bundler/test/deterministic.test.ts | Updates determinism checks to normalize documented embedded absolute aliases in worker runtime output. |
| tools/egg-bundler/test/snapshots/EntryGenerator.worker.canonical.snap | Updates canonical worker entry snapshot to reflect new runtime baseDir/framework/alias logic. |
| tools/egg-bundler/test/EntryGenerator.test.ts | Extends tests for alias keying, symlink behavior, and executing generated worker with explicit framework through bundle loader. |
| tools/egg-bundler/src/lib/EntryGenerator.ts | Implements outputDir-as-runtime-baseDir, explicit framework passing, and richer bundle-map aliasing (incl. resolveCache aliases). |
| tools/egg-bundler/docs/output-structure.md | Updates docs to reflect new startEgg({ baseDir: outputDir, framework, ... }) and aliasing behavior. |
| packages/core/test/loader/manifest_coverage.test.ts | Adds coverage test asserting plugin conventional dynamic files appear in the manifest under metadataOnly loading. |
| packages/core/test/fixtures/manifest-dynamic-plugin/package.json | Adds a new fixture app for manifest coverage. |
| packages/core/test/fixtures/manifest-dynamic-plugin/config/plugin.js | Enables the fixture @eggjs/security plugin. |
| packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/package.json | Adds a fixture plugin package with eggPlugin metadata. |
| packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/agent.js | Adds fixture plugin agent boot file. |
| packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/app.js | Adds fixture plugin app boot file. |
| packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/app/extend/agent.js | Adds fixture plugin agent extend file. |
| packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/app/extend/application.js | Adds fixture plugin application extend file. |
| packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/app/middleware/securities.js | Adds fixture plugin middleware file for discovery coverage. |
| packages/core/src/loader/egg_loader.ts | Augments manifest generation to include convention-based dynamic plugin files and middleware discovery. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@packages/core/src/loader/egg_loader.ts`:
- Line 11: Update the globby usage to match the current API: replace the default
import "globby" with the named import "globbySync" and update any calls that use
globby(...) to globbySync(...); also fix the glob syntax used (change
"**/*.(js|ts)" to the brace expansion "**/*.{js,ts}") so file matching works
correctly — search for the import statement and all usages around egg_loader.ts
(e.g., where the glob pattern is defined) and update the import symbol and the
pattern accordingly.
🪄 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: 9f6e1049-956c-414a-b275-5b6ecf69b34b
⛔ Files ignored due to path filters (7)
packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/agent.jsis excluded by!**/node_modules/**packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/app.jsis excluded by!**/node_modules/**packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/app/extend/agent.jsis excluded by!**/node_modules/**packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/app/extend/application.jsis excluded by!**/node_modules/**packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/app/middleware/securities.jsis excluded by!**/node_modules/**packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/package.jsonis excluded by!**/node_modules/**tools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
packages/core/src/loader/egg_loader.tspackages/core/test/fixtures/manifest-dynamic-plugin/config/plugin.jspackages/core/test/fixtures/manifest-dynamic-plugin/package.jsonpackages/core/test/loader/manifest_coverage.test.tstools/egg-bundler/docs/output-structure.mdtools/egg-bundler/src/lib/EntryGenerator.tstools/egg-bundler/test/EntryGenerator.test.tstools/egg-bundler/test/deterministic.test.tswiki/log.mdwiki/packages/egg-bundler.md
killagu
force-pushed
the
agent/egg-dev/7e0d31d1
branch
from
7aeaf80 to
db3a8ce
Compare
killagu
force-pushed
the
agent/egg-dev/7e0d31d1
branch
from
db3a8ce to
9167ab7
Compare
Contributor
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-bin/src/commands/bundle.ts`:
- Around line 38-48: getBundleFrameworkSpecifier currently returns
pkg.egg.framework or the --framework flag verbatim, which drops support for
custom/absolute framework paths; update it to reuse the existing normalization
logic from packages/utils/src/framework.ts (e.g. call the resolver function used
in lines 28-91, such as resolveFrameworkSpecifier) so that path-like values
(relative/absolute or custom locations) are resolved to the same canonical form
the bundler expects while non-path framework names are preserved; locate
getBundleFrameworkSpecifier, import and call the resolver with baseDir and the
raw framework value (falling back to 'egg') and return the resolved specifier to
maintain compatibility.
🪄 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: 02dcca4c-a5b0-4524-aae2-dc2f73c132f3
⛔ Files ignored due to path filters (7)
packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/agent.jsis excluded by!**/node_modules/**packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/app.jsis excluded by!**/node_modules/**packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/app/extend/agent.jsis excluded by!**/node_modules/**packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/app/extend/application.jsis excluded by!**/node_modules/**packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/app/middleware/securities.jsis excluded by!**/node_modules/**packages/core/test/fixtures/manifest-dynamic-plugin/node_modules/@eggjs/security/package.jsonis excluded by!**/node_modules/**tools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snapis excluded by!**/*.snap
📒 Files selected for processing (18)
packages/core/src/loader/egg_loader.tspackages/core/test/fixtures/manifest-dynamic-plugin/config/plugin.jspackages/core/test/fixtures/manifest-dynamic-plugin/package.jsonpackages/core/test/loader/manifest_coverage.test.tstools/egg-bin/README.mdtools/egg-bin/src/commands/bundle.tstools/egg-bin/test/commands/bundle.test.tstools/egg-bundler/README.mdtools/egg-bundler/docs/output-structure.mdtools/egg-bundler/src/index.tstools/egg-bundler/src/lib/Bundler.tstools/egg-bundler/src/lib/EntryGenerator.tstools/egg-bundler/src/lib/frameworkSpecifier.tstools/egg-bundler/test/Bundler.test.tstools/egg-bundler/test/EntryGenerator.test.tstools/egg-bundler/test/deterministic.test.tswiki/log.mdwiki/packages/egg-bundler.md
✅ Files skipped from review due to trivial changes (7)
- packages/core/test/fixtures/manifest-dynamic-plugin/config/plugin.js
- packages/core/test/fixtures/manifest-dynamic-plugin/package.json
- tools/egg-bin/README.md
- wiki/packages/egg-bundler.md
- tools/egg-bundler/src/index.ts
- wiki/log.md
- tools/egg-bundler/test/deterministic.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/loader/egg_loader.ts
- packages/core/test/loader/manifest_coverage.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/test/loader/manifest_coverage.test.ts (1)
104-134: ⚡ Quick win
testApp.close()is skipped when an assertion throws earlyBoth new tests call
await testApp.close()at the end without afinallyguard. A failing assertion aborts the test body and the app is never closed, potentially leaving file-watchers or open handles that can interfere with subsequent tests. This matches the pre-existing pattern in the file; consider fixing all affected tests together.♻️ Proposed pattern – use `try/finally` for guaranteed cleanup
it('should collect configured customLoader directories in metadataOnly startup', async () => { const testApp = createApp('custom-loader', { metadataOnly: true }); - await testApp.loader.loadPlugin(); - // ... assertions ... - await testApp.close(); + try { + await testApp.loader.loadPlugin(); + // ... assertions ... + } finally { + await testApp.close(); + } });Alternatively, use Vitest's
onTestFinishedhook:- it('should collect ...', async () => { - const testApp = createApp('custom-loader', { metadataOnly: true }); + it('should collect ...', async ({ onTestFinished }) => { + const testApp = createApp('custom-loader', { metadataOnly: true }); + onTestFinished(() => testApp.close()); // ... rest of body without manual close() ... - await testApp.close(); });Also applies to: 136-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/loader/manifest_coverage.test.ts` around lines 104 - 134, The tests create a testApp and call await testApp.close() at the end but do not guarantee cleanup if an assertion throws; update each affected it block (the ones that create testApp and call testApp.close(), e.g., the test using createApp('custom-loader', { metadataOnly: true }) and the other similar test) to declare testApp before the test body and wrap the test logic and assertions in a try/finally where the finally always awaits testApp.close(); alternatively you may register cleanup via Vitest's onTestFinished, but the direct fix is: move creation so testApp is in scope, put all load/manifest/assert calls inside try { ... } and do finally { await testApp.close(); } to ensure watchers/handles are closed on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/test/loader/manifest_coverage.test.ts`:
- Around line 112-131: The assertions call manifest.fileDiscovery[...]
.includes(...) directly which throws if the key is missing; change each includes
invocation to use optional chaining so the lookup becomes
manifest.fileDiscovery['app/adapter']?.includes('docker.js') (and similarly for
'app/util', 'app/repository', 'app/plugin', 'config/b/app/plugin' and the other
occurrences around the later assertions) so that assert.ok receives false and
the custom message instead of a TypeError; update all six shown includes and the
matching ones later in the file.
---
Nitpick comments:
In `@packages/core/test/loader/manifest_coverage.test.ts`:
- Around line 104-134: The tests create a testApp and call await testApp.close()
at the end but do not guarantee cleanup if an assertion throws; update each
affected it block (the ones that create testApp and call testApp.close(), e.g.,
the test using createApp('custom-loader', { metadataOnly: true }) and the other
similar test) to declare testApp before the test body and wrap the test logic
and assertions in a try/finally where the finally always awaits testApp.close();
alternatively you may register cleanup via Vitest's onTestFinished, but the
direct fix is: move creation so testApp is in scope, put all
load/manifest/assert calls inside try { ... } and do finally { await
testApp.close(); } to ensure watchers/handles are closed on failure.
🪄 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: c72bfb8b-951b-43d2-a797-e077df72e825
📒 Files selected for processing (1)
packages/core/test/loader/manifest_coverage.test.ts
| export function assertFrameworkPackageSpecifier(framework: string): void { | ||
| if (!framework || isPathLikeFrameworkSpecifier(framework)) { | ||
| throw new Error( | ||
| `[@eggjs/egg-bundler] framework must be a package specifier for bundled runtime, got path-like value: ${framework}`, |
killagu
force-pushed
the
agent/egg-dev/7e0d31d1
branch
from
ee9ce75 to
1a4d3cc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Notes: targeted oxlint on changed files exits 0 with existing warnings in pre-existing egg_loader.ts lines outside this patch.
Summary by CodeRabbit
New Features
Documentation
Tests & Chores