feat(bundler): inject manifest loader fs into worker#5944
feat(bundler): inject manifest loader fs into worker#5944
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 (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds ManifestLoaderFS (manifest-backed LoaderFS), threads an injectable ChangesManifest-Backed Loader Filesystem
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
Deploying egg with
|
| Latest commit: |
e153fd4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://31eb13e9.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-8cf5cc33.egg-cci.pages.dev |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #5944 +/- ##
==========================================
+ Coverage 85.21% 85.25% +0.04%
==========================================
Files 669 669
Lines 19304 19508 +204
Branches 3787 3847 +60
==========================================
+ Hits 16449 16631 +182
- Misses 2463 2483 +20
- Partials 392 394 +2 ☔ 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}
There was a problem hiding this comment.
Code Review
This pull request introduces ManifestLoaderFS, a new filesystem abstraction that allows the Egg loader to resolve and load files using a manifest, prioritizing bundled modules over the real filesystem. The implementation is integrated into EggCore and EggLoader, and the egg-bundler tool is updated to leverage this for generated entries. Feedback focuses on performance and logic improvements within ManifestLoaderFS and EggLoader. Specifically, it is recommended to replace linear scans (O(N)) with pre-calculated data structures like Set to improve efficiency in methods like isManifestFile and isManifestDirectory. Additionally, the reviewer suggests using exact path segment matching instead of startsWith to avoid over-broad directory matching and optimizing the manifest generation loop in EggLoader to prevent O(N^2) complexity.
Deploying egg-v3 with
|
| Latest commit: |
e153fd4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b96f1b8e.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-8cf5cc33.egg-v3.pages.dev |
There was a problem hiding this comment.
Pull request overview
This PR wires a manifest-backed LoaderFS through the single-process startEgg() path and updates the egg-bundler worker entry to construct and pass a ManifestLoaderFS instance derived from the inlined bundled ManifestStore. This enables bundled workers to perform loader filesystem operations against the startup manifest/bundle map instead of relying on the host filesystem.
Changes:
- Add
loaderFSas an option onstartEgg()/EggCore, and pass it intoEggLoader. - Introduce
ManifestLoaderFS(manifest-firstLoaderFS) and haveEggLoaderdefault to it when a manifest/bundle store is present. - Update egg-bundler’s generated worker entry + tests/snapshots to create
ManifestLoaderFSfrom the inlined manifest store and pass it tostartEgg().
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tools/egg-bundler/test/EntryGenerator.test.ts | Updates expectations/runtime test to validate ManifestLoaderFS creation and loaderFS threading into startEgg(). |
| tools/egg-bundler/test/snapshots/EntryGenerator.worker.canonical.snap | Updates canonical worker entry snapshot to include ManifestLoaderFS and loaderFS option. |
| tools/egg-bundler/src/lib/EntryGenerator.ts | Generates worker entry code that instantiates ManifestLoaderFS from the bundled manifest store and passes it to startEgg(). |
| packages/egg/src/lib/start.ts | Adds loaderFS?: LoaderFS to StartEggOptions and threads it through Agent/Application construction. |
| packages/core/test/loader/manifest_loader_fs.test.ts | Adds coverage for manifest-backed LoaderFS behavior (stat/realpath/glob/readJSON/loadFile + fallback). |
| packages/core/test/loader/manifest_coverage.test.ts | Extends coverage assertions to ensure plugin package.json metadata is captured in manifest discovery. |
| packages/core/test/egg.test.ts | Adds a regression test asserting EggCore({ loaderFS }) passes through to EggLoader. |
| packages/core/test/snapshots/index.test.ts.snap | Updates public export snapshot to include ManifestLoaderFS. |
| packages/core/src/loader/loader_fs.ts | Implements ManifestLoaderFS and supporting glob/filter utilities. |
| packages/core/src/loader/egg_loader.ts | Defaults/wraps loaderFS with ManifestLoaderFS, switches plugin metadata reads to loaderFS, and collects package.json into manifest discovery. |
| packages/core/src/egg.ts | Adds loaderFS?: LoaderFS to EggCoreOptions and forwards into EggLoader options. |
| packages/core/package.json | Adds multimatch dependency used for manifest-backed glob matching. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/core/src/loader/egg_loader.ts (2)
98-101: 💤 Low valueConstructor + post-manifest wiring is consistent — minor note on bundle vs. local-manifest divergence.
When
bundleStore?.baseDir === this.options.baseDir,loaderFSbecomesnew ManifestLoaderFS(bundleStore)and the post-manifest wrap is skipped (instanceof check). That means in bundled mode the FS layer reads frombundleStore, whilethis.manifest(loaded/created locally) is used byresolveModule. This is fine if the loaded manifest mirrors the bundle store, but worth a brief inline comment so future readers don’t assumethis.manifestand the FS layer share a single manifest source in this branch.Also applies to: 182-184
🤖 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/src/loader/egg_loader.ts` around lines 98 - 101, The code sets this.loaderFS to new ManifestLoaderFS(bundleStore) when ManifestStore.getBundleStore().baseDir === this.options.baseDir, which means the FS layer will read from the bundle store while this.manifest (used by resolveModule) may be the locally loaded/created manifest; add a brief inline comment next to the assignment (near ManifestStore.getBundleStore, ManifestLoaderFS, RealLoaderFS and the post-manifest instanceof check around resolveModule) explaining that in bundled mode the loaderFS and this.manifest can diverge and that resolveModule relies on this.manifest while ManifestLoaderFS reads from the bundle store so readers don’t assume a single shared manifest source.
1848-1858: 💤 Low valueLGTM — minor: the resolveCache scan is O(n) per call.
Object.values(manifest.resolveCache).includes(fileKey)is linear in resolveCache size and runs once per load unit. This only affects manifest generation (not request hot paths), so it's fine, but aSet<string>of resolveCache values built once in#collectConventionalDynamicFileswould avoid the repeated scan if this list grows.♻️ Optional: precompute the value set once
`#collectConventionalDynamicFiles`(manifest: StartupManifest): void { + const resolveCacheTargets = new Set(Object.values(manifest.resolveCache).filter((v): v is string => typeof v === 'string')); for (const unit of this.getLoadUnits()) { - this.#collectConventionFile(manifest, path.join(unit.path, 'package.json')); + this.#collectConventionFile(manifest, path.join(unit.path, 'package.json'), resolveCacheTargets); ...🤖 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/src/loader/egg_loader.ts` around lines 1848 - 1858, The current check in `#collectConventionFile` uses Object.values(manifest.resolveCache).includes(fileKey) which is O(n) per call; change the approach by precomputing a Set of manifest.resolveCache values once in `#collectConventionalDynamicFiles` (e.g., const resolvedSet = new Set(Object.values(manifest.resolveCache))) and pass that Set into `#collectConventionFile` (or attach it to the manifest under a temporary property) so `#collectConventionFile` can do resolvedSet.has(fileKey) instead of a repeated linear scan; update all call sites of `#collectConventionFile` in `#collectConventionalDynamicFiles` to provide the Set and remove the Object.values(...).includes usage.packages/core/src/loader/loader_fs.ts (1)
178-204: ⚖️ Poor tradeoffOptional: precompute the set of manifest directories.
#isManifestDirectoryruns an O(D × F) scan overfileDiscovery(and another O(R) overresolveCache) on every call, and it is reached from everyexists/stat/realpath/loadFile/readJSONinvocation plusglobvia#getManifestEntryand#listManifestFilesUnder. This is a hot path with repeated scans for typical loader operations.If you want to keep it O(1), precompute a
Set<string>of every directory that the manifest covers in the constructor (fileDiscoverykeys + parent directories of everydir/filecombination and each non-nullresolveCachetarget), then this method becomes a singleset.has(rel)lookup. Defer until profiling shows it matters.🤖 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/src/loader/loader_fs.ts` around lines 178 - 204, Precompute a Set of manifest directories in the class constructor and make `#isManifestDirectory` do a single lookup: build a Set<string> (e.g., manifestDirs) from this.#manifest.data.fileDiscovery keys plus the parent directories of every path.posix.join(dir, file) and every non-null target in this.#manifest.data.resolveCache; update the constructor to populate that Set and change `#isManifestDirectory`(rel: string) to return this.manifestDirs.has(rel) (with the empty-string case still delegating to this.#hasManifestData() if needed) so you avoid the O(D×F) scans on each call.
🤖 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/src/loader/egg_loader.ts`:
- Around line 98-101: The code sets this.loaderFS to new
ManifestLoaderFS(bundleStore) when ManifestStore.getBundleStore().baseDir ===
this.options.baseDir, which means the FS layer will read from the bundle store
while this.manifest (used by resolveModule) may be the locally loaded/created
manifest; add a brief inline comment next to the assignment (near
ManifestStore.getBundleStore, ManifestLoaderFS, RealLoaderFS and the
post-manifest instanceof check around resolveModule) explaining that in bundled
mode the loaderFS and this.manifest can diverge and that resolveModule relies on
this.manifest while ManifestLoaderFS reads from the bundle store so readers
don’t assume a single shared manifest source.
- Around line 1848-1858: The current check in `#collectConventionFile` uses
Object.values(manifest.resolveCache).includes(fileKey) which is O(n) per call;
change the approach by precomputing a Set of manifest.resolveCache values once
in `#collectConventionalDynamicFiles` (e.g., const resolvedSet = new
Set(Object.values(manifest.resolveCache))) and pass that Set into
`#collectConventionFile` (or attach it to the manifest under a temporary property)
so `#collectConventionFile` can do resolvedSet.has(fileKey) instead of a repeated
linear scan; update all call sites of `#collectConventionFile` in
`#collectConventionalDynamicFiles` to provide the Set and remove the
Object.values(...).includes usage.
In `@packages/core/src/loader/loader_fs.ts`:
- Around line 178-204: Precompute a Set of manifest directories in the class
constructor and make `#isManifestDirectory` do a single lookup: build a
Set<string> (e.g., manifestDirs) from this.#manifest.data.fileDiscovery keys
plus the parent directories of every path.posix.join(dir, file) and every
non-null target in this.#manifest.data.resolveCache; update the constructor to
populate that Set and change `#isManifestDirectory`(rel: string) to return
this.manifestDirs.has(rel) (with the empty-string case still delegating to
this.#hasManifestData() if needed) so you avoid the O(D×F) scans on each call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ead44421-7024-4838-a71f-a368b7fd3a69
⛔ Files ignored due to path filters (2)
packages/core/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snaptools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
packages/core/package.jsonpackages/core/src/egg.tspackages/core/src/loader/egg_loader.tspackages/core/src/loader/loader_fs.tspackages/core/test/egg.test.tspackages/core/test/loader/manifest_coverage.test.tspackages/core/test/loader/manifest_loader_fs.test.tspackages/egg/src/lib/start.tstools/egg-bundler/src/lib/EntryGenerator.tstools/egg-bundler/test/EntryGenerator.test.ts
Summary
loaderFSthroughstartEggandEggCoreintoEggLoaderManifestLoaderFSfrom the inline manifest store and pass it via loader optionsTests
pnpm exec vitest run tools/egg-bundler/test/EntryGenerator.test.ts packages/core/test/egg.test.ts packages/core/test/loader/manifest_loader_fs.test.ts -u(root run picked up the two core files)pnpm exec vitest run test/EntryGenerator.test.ts -ufromtools/egg-bundlerpnpm --filter @eggjs/core typecheckpnpm --filter egg typecheckpnpm --filter @eggjs/egg-bundler typecheckpnpm exec oxlint --type-aware packages/core/src/egg.ts packages/core/test/egg.test.ts packages/egg/src/lib/start.ts tools/egg-bundler/src/lib/EntryGenerator.ts tools/egg-bundler/test/EntryGenerator.test.ts(0 errors; existing warnings in untouched sections of core files)Summary by CodeRabbit