fix: route loader runtime through manifest fs#5946
Conversation
📝 WalkthroughWalkthroughThis PR adds a global bundled text-file loader, implements ManifestLoaderFS to virtualize manifest/bundled paths, extends LoaderFS with readFile support and readFileWithLoaderFS, refactors EggLoader to use LoaderFS for filesystem/module ops, and updates the bundler to embed package.json-like text files and expose a runtime file loader. ChangesBundle File Loader Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: |
e9eff2b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://377b74f5.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-c7776d34.egg-cci.pages.dev |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #5946 +/- ##
==========================================
+ Coverage 85.21% 85.23% +0.02%
==========================================
Files 669 669
Lines 19304 19412 +108
Branches 3787 3821 +34
==========================================
+ Hits 16449 16546 +97
- Misses 2463 2473 +10
- Partials 392 393 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
e9eff2b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d7336cd4.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-c7776d34.egg-v3.pages.dev |
![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 virtual file system abstraction (LoaderFS) to support loading application files from a manifest in bundled environments, including updates to EggLoader and egg-bundler. Feedback identifies critical issues with the initialization timing of ManifestLoaderFS, incorrect encoding logic in readFile, and behavioral inconsistencies in loadFile for JSON. Furthermore, the reviewer noted performance and correctness gaps in directory resolution and the synthetic Stats implementation.
There was a problem hiding this comment.
Pull request overview
This PR improves bundled-runtime compatibility by introducing a manifest-backed filesystem abstraction and routing Egg’s loader reads through it, enabling bundled workers to satisfy certain file reads (notably package.json) without replaying bundle-time filesystem paths.
Changes:
- Add
ManifestLoaderFSto serveexists/stat/realpath/readFile/readJSON/loadFilefrom manifest data and bundler-provided runtime hooks. - Update
EggLoaderto preferloaderFSfor environment/config/plugin/package metadata reads and to auto-wrap withManifestLoaderFSwhen no customloaderFSis provided. - Extend
egg-bundlerworker entry generation to embed resolvedpackage.jsontext and expose it viaglobalThis.__EGG_BUNDLE_FILE_LOADER__, with tests/snapshots updated accordingly.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/egg-bundler/src/lib/EntryGenerator.ts | Embed package.json contents into generated worker entry and expose a text-file loader hook. |
| tools/egg-bundler/test/EntryGenerator.test.ts | Add coverage ensuring package.json targets are emitted via the bundled text loader (no static module imports). |
| tools/egg-bundler/test/snapshots/EntryGenerator.worker.canonical.snap | Update canonical worker entry snapshot to include text-file maps/aliases and the new global hook. |
| packages/core/src/loader/loader_fs.ts | Add readFile to LoaderFS; implement ManifestLoaderFS that can serve manifest-known files and bundled text/module loaders. |
| packages/core/src/loader/egg_loader.ts | Route multiple filesystem reads through loaderFS and wrap default FS with ManifestLoaderFS for manifest/bundle-aware behavior. |
| packages/core/src/global.ts | Declare __EGG_BUNDLE_FILE_LOADER__ global type for bundled text file support. |
| packages/core/test/loader/loader_fs.test.ts | Add test verifying manifest-known paths do not hit delegate stat/exists. |
| packages/core/test/loader/egg_loader.test.ts | Add test ensuring EggLoader.loadFile() uses loaderFS.loadFile(). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tools/egg-bundler/src/lib/EntryGenerator.ts (2)
119-129: 💤 Low valueSync
readFileSyncinside an async pipeline — acceptable at bundle time, but worth a thought.
#collectBundleTextFilesruns at bundle generation time withingenerate()(which isasync). UsingreadFileSynchere is fine and keeps ordering deterministic, but if any of the resolved text files are unexpectedly missing, the failure surfaces as a genericENOENTfromreadFileSyncwith no context (which package.json, which relKey). Wrapping this in a small try/catch that throws a more descriptive error (relKey + absolute path attempted) would make manifest/bundle drift dramatically easier to debug.🤖 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 `@tools/egg-bundler/src/lib/EntryGenerator.ts` around lines 119 - 129, The helper `#collectBundleTextFiles` currently calls readFileSync (from within the async generate() pipeline) without context on failure; wrap the readFileSync call in a try/catch inside `#collectBundleTextFiles` and, on error, throw a new Error that includes the relKey and the absolute path produced by this.#absFromRelKey(relKey) (and preserve the original error.message or stack) so consumers of BundleTextFile failures can see which file (relKey and abs path) failed to read; keep the rest of the sorting/mapping logic unchanged.
119-133: 💤 Low valueTighten the contract of
#isTextFileEntry(or rename it to reflect package.json scope).
#isTextFileEntryis package.json-specific (endsWith('/package.json') || === 'package.json'), but the surrounding type (BundleTextFile), helper names (#collectBundleTextFiles,__BUNDLE_TEXT_FILE_REL,__setBundleTextFile,__getBundleTextFile,__APP_TEXT_FILE_ALIASES), and the runtime hook (__EGG_BUNDLE_FILE_LOADER__) all read as a generic text-file mechanism. If only package.json is intended, consider renaming the predicate to e.g.#isPackageJsonEntryso the constraint is visible at every call site. If text files beyond package.json are planned, this predicate is the central place to extend.🤖 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 `@tools/egg-bundler/src/lib/EntryGenerator.ts` around lines 119 - 133, The predicate `#isTextFileEntry` is actually package.json-specific; either tighten its name to `#isPackageJsonEntry` and update all call sites (e.g., `#collectBundleTextFiles`, and any runtime helpers/aliases referenced by __BUNDLE_TEXT_FILE_REL, __setBundleTextFile, __getBundleTextFile, __APP_TEXT_FILE_ALIASES, __EGG_BUNDLE_FILE_LOADER__) so the contract is explicit, or broaden the predicate to include other text-file patterns (e.g., .json/.txt/.md by extension or path checks) and document that change; implement the chosen approach consistently by renaming the private method and replacing its usages, or by extending its logic to return true for additional text-file patterns.packages/core/src/loader/loader_fs.ts (1)
102-111: 💤 Low value
loadFilereturningBuffer.from(text)for bundle text is semantically off for a "load a module" API.
LoaderFS.loadFileis consumed byEggLoader.requireFileto obtain a module value (object/function/class). If a caller ever passes a path that hits__EGG_BUNDLE_FILE_LOADER__(today onlypackage.json), they will receive aBufferrather than a parsed module. In the current call graph this branch is unreachable —EggLoaderonly usesreadJSONfor package.json — but the asymmetry betweenreadFile(Buffer/string) andloadFile(module) is worth tightening so future callers can't trip over it. Two clean options:
- Drop the bundle-text branch from
loadFileand let it fall through todelegate.loadFile, leaving text-file access toreadFile/readJSON.- If a JSON-shaped result is desired, parse it: return
JSON.parse(text)(matches whatreadJSONwould return).Either keeps
loadFile's contract consistent with its callers.🤖 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 102 - 111, LoaderFS.loadFile currently returns Buffer.from(text) when `#bundleFileText` hits, which violates the "module value" contract used by EggLoader.requireFile; change loadFile to drop the bundle-text branch (remove the Buffer.from(text) return) so that when `#bundleFileText` returns text it falls through to this.#delegate.loadFile(filepath) instead; keep the __EGG_BUNDLE_MODULE_LOADER__ handling and the this.#unwrapDefault(bundleHit) behavior, and ensure callers that need raw text/JSON still use readFile/readJSON (or, if you prefer JSON semantics, replace the Buffer return with JSON.parse(text) instead — but do one or the other to keep loadFile returning module-shaped values).
🤖 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 `@packages/core/src/global.ts`:
- Around line 4-6: The core package's global declarations are missing
__EGG_BUNDLE_MODULE_LOADER__, causing a type gap when loader_fs.ts calls
globalThis.__EGG_BUNDLE_MODULE_LOADER__;. Add a declaration alongside the
existing __EGG_BUNDLE_STORE__ and __EGG_BUNDLE_FILE_LOADER__ in
packages/core/src/global.ts by declaring var __EGG_BUNDLE_MODULE_LOADER__:
((filepath: string) => unknown) | undefined; so the global symbol used by
loader_fs.ts is typed within the core package.
In `@packages/core/src/loader/loader_fs.ts`:
- Around line 119-134: The manifest lookup in `#manifestEntryType` incorrectly
uses this.#manifest.data.fileDiscovery[rel] which can hit prototype properties;
change that check to use Object.hasOwn(this.#manifest.data.fileDiscovery, rel)
(preserving the existing behavior that empty rel means 'directory'), so the
first conditional becomes "if (rel === '' ||
Object.hasOwn(this.#manifest.data.fileDiscovery, rel)) return 'directory'";
leave the subsequent loops (including the
Object.values(this.#manifest.data.resolveCache) check and the
path.posix.relative test) unchanged and keep using this.#toRelative and
this.#normalize as before.
---
Nitpick comments:
In `@packages/core/src/loader/loader_fs.ts`:
- Around line 102-111: LoaderFS.loadFile currently returns Buffer.from(text)
when `#bundleFileText` hits, which violates the "module value" contract used by
EggLoader.requireFile; change loadFile to drop the bundle-text branch (remove
the Buffer.from(text) return) so that when `#bundleFileText` returns text it falls
through to this.#delegate.loadFile(filepath) instead; keep the
__EGG_BUNDLE_MODULE_LOADER__ handling and the this.#unwrapDefault(bundleHit)
behavior, and ensure callers that need raw text/JSON still use readFile/readJSON
(or, if you prefer JSON semantics, replace the Buffer return with
JSON.parse(text) instead — but do one or the other to keep loadFile returning
module-shaped values).
In `@tools/egg-bundler/src/lib/EntryGenerator.ts`:
- Around line 119-129: The helper `#collectBundleTextFiles` currently calls
readFileSync (from within the async generate() pipeline) without context on
failure; wrap the readFileSync call in a try/catch inside
`#collectBundleTextFiles` and, on error, throw a new Error that includes the
relKey and the absolute path produced by this.#absFromRelKey(relKey) (and
preserve the original error.message or stack) so consumers of BundleTextFile
failures can see which file (relKey and abs path) failed to read; keep the rest
of the sorting/mapping logic unchanged.
- Around line 119-133: The predicate `#isTextFileEntry` is actually
package.json-specific; either tighten its name to `#isPackageJsonEntry` and update
all call sites (e.g., `#collectBundleTextFiles`, and any runtime helpers/aliases
referenced by __BUNDLE_TEXT_FILE_REL, __setBundleTextFile, __getBundleTextFile,
__APP_TEXT_FILE_ALIASES, __EGG_BUNDLE_FILE_LOADER__) so the contract is
explicit, or broaden the predicate to include other text-file patterns (e.g.,
.json/.txt/.md by extension or path checks) and document that change; implement
the chosen approach consistently by renaming the private method and replacing
its usages, or by extending its logic to return true for additional text-file
patterns.
🪄 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: 4490d3d2-ab20-475f-bcbe-ed18da5d1ad6
⛔ Files ignored due to path filters (1)
tools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
packages/core/src/global.tspackages/core/src/loader/egg_loader.tspackages/core/src/loader/loader_fs.tspackages/core/test/loader/egg_loader.test.tspackages/core/test/loader/loader_fs.test.tstools/egg-bundler/src/lib/EntryGenerator.tstools/egg-bundler/test/EntryGenerator.test.ts
|
Addressed the review feedback in follow-up commits:
Latest local checks pass. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/loader/loader_fs.ts (1)
120-131: ⚡ Quick winMirror
#bundleFileText's two-key lookup inloadFilefor consistency and robustness.
#bundleFileTextqueries the bundle file loader with both#normalize(filepath)and the relative form (#toRelative).loadFileonly passesthis.#normalize(filepath)to__EGG_BUNDLE_MODULE_LOADER__. While the bundler initializes the module map with dual-key registration when possible (line 327–328 inEntryGenerator.ts: if input is relative, it stores under both relative and absolute), this guarantee doesn't hold for all paths. Mirroring the two-key fallback pattern from#bundleFileTextensures consistency and guards against edge cases where a module is keyed only by relative path.Suggested fix (mirrors `#bundleFileText` pattern)
async loadFile(filepath: string): Promise<unknown> { const normalized = this.#normalize(filepath); - const bundleHit = globalThis.__EGG_BUNDLE_MODULE_LOADER__?.(normalized); - if (bundleHit !== undefined) return this.#unwrapDefault(bundleHit); + const moduleLoader = globalThis.__EGG_BUNDLE_MODULE_LOADER__; + if (moduleLoader) { + const direct = moduleLoader(normalized); + if (direct !== undefined) return this.#unwrapDefault(direct); + const rel = this.#toRelative(filepath); + if (rel !== normalized) { + const fallback = moduleLoader(rel); + if (fallback !== undefined) return this.#unwrapDefault(fallback); + } + }🤖 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 120 - 131, loadFile currently calls __EGG_BUNDLE_MODULE_LOADER__ with only this.#normalize(filepath) but `#bundleFileText` does a two-key lookup (normalized and this.#toRelative(filepath)); update loadFile to mirror that pattern: first call globalThis.__EGG_BUNDLE_MODULE_LOADER__ with normalized, and if it returns undefined call it again with this.#toRelative(filepath) before falling back to `#bundleFileText` and `#delegate.loadFile`; use the same unwrap logic (this.#unwrapDefault) for any hit to ensure consistent behavior between loadFile and `#bundleFileText`.
🤖 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/loader_fs.ts`:
- Around line 120-131: loadFile currently calls __EGG_BUNDLE_MODULE_LOADER__
with only this.#normalize(filepath) but `#bundleFileText` does a two-key lookup
(normalized and this.#toRelative(filepath)); update loadFile to mirror that
pattern: first call globalThis.__EGG_BUNDLE_MODULE_LOADER__ with normalized, and
if it returns undefined call it again with this.#toRelative(filepath) before
falling back to `#bundleFileText` and `#delegate.loadFile`; use the same unwrap
logic (this.#unwrapDefault) for any hit to ensure consistent behavior between
loadFile and `#bundleFileText`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9ec137b-cfa0-4364-85df-db6c534b8ba6
⛔ Files ignored due to path filters (1)
packages/core/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
packages/core/src/global.tspackages/core/src/loader/egg_loader.tspackages/core/src/loader/loader_fs.tspackages/core/test/loader/egg_loader.test.tspackages/core/test/loader/loader_fs.test.tstools/egg-bundler/src/lib/EntryGenerator.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/test/loader/egg_loader.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/egg-bundler/src/lib/EntryGenerator.ts
- packages/core/src/loader/egg_loader.ts
Summary
ManifestLoaderFSthat can answer bundled file existence/stat/read/load calls from manifest data and generated bundle file hooksloaderFSinstead of directfs/utils.loadFilepaths where possiblepackage.jsonwithout filesystem path replayValidation
pnpm exec vitest run packages/core/test/loader/loader_fs.test.ts packages/core/test/loader/egg_loader.test.tspnpm --filter @eggjs/egg-bundler test -- EntryGenerator.test.tspnpm --filter @eggjs/core typecheckpnpm --filter @eggjs/egg-bundler typecheckpnpm exec oxlint --type-aware packages/core/src/global.ts packages/core/src/loader/egg_loader.ts packages/core/src/loader/loader_fs.ts packages/core/test/loader/egg_loader.test.ts packages/core/test/loader/loader_fs.test.ts tools/egg-bundler/src/lib/EntryGenerator.ts tools/egg-bundler/test/EntryGenerator.test.tsSummary by CodeRabbit
New Features
Refactor
Tests