feat(core): add manifest-backed loader fs#5943
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:
📝 WalkthroughWalkthroughAdds ManifestLoaderFS (manifest-backed LoaderFS) with manifest-resolution, bundled-module loading, and multimatch-based globbing; integrates it into EggLoader initialization, switches plugin package.json reads to loaderFS APIs, records discovered package.json into manifest.fileDiscovery, and adds tests and a re-export. ChangesManifest-Based Filesystem Loading
Sequence Diagram(s)sequenceDiagram
participant EggLoader
participant ManifestStore
participant ManifestLoaderFS
participant RealLoaderFS
EggLoader->>ManifestStore: lookup manifest for baseDir
alt manifest exists
EggLoader->>ManifestLoaderFS: construct with manifest + RealLoaderFS fallback
else no manifest
EggLoader->>RealLoaderFS: use RealLoaderFS or provided loaderFS
end
EggLoader->>ManifestLoaderFS: loaderFS.exists(package.json)
EggLoader->>ManifestLoaderFS: loaderFS.readJSON(package.json)
EggLoader->>ManifestStore: collectConventionFile -> update manifest.fileDiscovery
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 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 ManifestLoaderFS, a filesystem abstraction that enables loading files from a manifest or bundle map with a fallback to the physical filesystem. It integrates this into EggLoader to facilitate bundled application support. The review feedback identifies critical limitations in the custom globToRegExp implementation, specifically regarding character classes and wildcards within groups. Additionally, it is recommended to enhance createVirtualStats by including standard fs.Stats date properties to prevent potential runtime errors in external libraries.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/loader/egg_loader.ts (1)
653-660:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep plugin export-path probing on
loaderFStoo.This now reads
package.jsonthrough the manifest abstraction, but#formatPluginPathFromPackageJSON()still probesexports.import/requirewithutility.exists(). In manifest-only startup, that check sees no real file and can wrongly fall back toeggPlugin.exports.typescript, so bundled plugins resolve to a path that is not actually present in the manifest.Suggested fix
- if (exports.typescript && isSupportTypeScript() && !(await exists(realPluginPath))) { + if (exports.typescript && isSupportTypeScript() && !this.loaderFS.exists(realPluginPath)) { // if require/import path not exists, use typescript path for development stage realPluginPath = path.join(pluginPath, exports.typescript); debug('[formatPluginPathFromPackageJSON] use typescript path %o', realPluginPath); }🤖 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 653 - 660, The plugin export-path probing currently uses a plain filesystem check inside `#formatPluginPathFromPackageJSON` which calls utility.exists() and thus misses manifest-only files; update `#formatPluginPathFromPackageJSON` to use the loaderFS abstraction (this.loaderFS.exists) when probing exports.import/exports.require and any path checks so that package.json -> plugin.path resolution honors the manifest-only startup. Ensure the method still handles both import/require keys and returns the same plugin.path behavior, referencing plugin.path, pkg, and `#formatPluginPathFromPackageJSON` in your change.
🤖 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/loader/loader_fs.ts`:
- Around line 88-99: Summary: ManifestLoaderFS.glob returns relative paths when
options.absolute is true if cwd is relative. Fix: keep using cwdRel =
this.#toRelative(...) for manifest lookup but resolve the caller-supplied cwd to
an absolute path before joining when options?.absolute is set; e.g., compute an
absolute cwd (path.resolve(cwd)) and use that in the matched.map((file) =>
path.join(absCwd, file)) so behaviour matches RealLoaderFS.glob; update the glob
method (symbols: glob, cwd, cwdRel, `#toRelative`, filterManifestGlob,
`#fallback.glob`, options.absolute) accordingly.
---
Outside diff comments:
In `@packages/core/src/loader/egg_loader.ts`:
- Around line 653-660: The plugin export-path probing currently uses a plain
filesystem check inside `#formatPluginPathFromPackageJSON` which calls
utility.exists() and thus misses manifest-only files; update
`#formatPluginPathFromPackageJSON` to use the loaderFS abstraction
(this.loaderFS.exists) when probing exports.import/exports.require and any path
checks so that package.json -> plugin.path resolution honors the manifest-only
startup. Ensure the method still handles both import/require keys and returns
the same plugin.path behavior, referencing plugin.path, pkg, and
`#formatPluginPathFromPackageJSON` in your change.
🪄 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: 5895165b-2f77-4cc2-a6b4-459e7e37142e
📒 Files selected for processing (4)
packages/core/src/loader/egg_loader.tspackages/core/src/loader/loader_fs.tspackages/core/test/loader/manifest_coverage.test.tspackages/core/test/loader/manifest_loader_fs.test.ts
Deploying egg with
|
| Latest commit: |
9400d37
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e43f68d8.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-1539d7a6.egg-cci.pages.dev |
Deploying egg-v3 with
|
| Latest commit: |
9400d37
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4c57997e.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-1539d7a6.egg-v3.pages.dev |
6fc9b6c to
f7dc977
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a manifest-first filesystem abstraction for Egg’s loader pipeline so bundled or manifest-backed startups can resolve/scan/load files using StartupManifest data (and the bundle module loader) with a real filesystem fallback.
Changes:
- Introduces
ManifestLoaderFS(manifest-firstLoaderFS) supporting manifest-backedexists/stat/realpath/glob/readJSON/loadFile. - Wires
EggLoaderto selectManifestLoaderFSautomatically for bundle stores and for manifest-backed startup flows, while keeping a real FS fallback. - Extends manifest generation to include
package.jsonin conventional discovery, plus adds/updates tests for manifest-backed behavior and plugin metadata inclusion.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/core/src/loader/loader_fs.ts | Adds ManifestLoaderFS implementation and manifest-backed glob/matching helpers. |
| packages/core/src/loader/egg_loader.ts | Integrates ManifestLoaderFS selection/wrapping and collects package.json into manifest discovery; switches plugin package.json reads to loaderFS. |
| packages/core/test/loader/manifest_loader_fs.test.ts | Adds new unit tests covering manifest-backed FS behavior and fallback/priority rules. |
| packages/core/test/loader/manifest_coverage.test.ts | Asserts plugin package metadata (package.json) is included in manifest fileDiscovery. |
f7dc977 to
aba2f64
Compare
aba2f64 to
5aff2fd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/core/src/loader/loader_fs.ts (1)
89-102:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
absolute: truestill returns relative paths when caller passes a relativecwd.At Line 90,
cwdkeeps the caller-supplied value verbatim. Whenoptions.absoluteis set on Line 98,path.join(cwd, file)on Line 99 yields a relative result, diverging fromRealLoaderFS.glob()which goes throughglobby.sync(...)and returns absolute paths under the same options. This is a behavioral mismatch in what's intended to be a drop-in replacement.Resolve
cwdto an absolute path at the source so both manifest lookup and the join produce consistent results.🔧 Suggested fix
glob(patterns: string | string[], options?: LoaderFSGlobOptions): string[] { - const cwd = options?.cwd === undefined ? process.cwd() : String(options.cwd); + const cwd = path.resolve(options?.cwd === undefined ? process.cwd() : String(options.cwd)); const cwdRel = this.#toRelative(cwd);🤖 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 89 - 102, The glob method currently uses the caller-supplied cwd verbatim which causes path.join(cwd, file) in the absolute branch to produce relative paths; change glob to resolve cwd to an absolute path up-front (e.g., using path.resolve or similar) before calling this.#toRelative and this.#listManifestFilesUnder so that both manifest lookup and the matched.map((file) => path.join(cwd, file)) produce absolute paths; update references in glob that use cwd (including the cwdRel assignment and the manifest lookup) so the rest of the method and filterManifestGlob behavior remain correct and consistent with RealLoaderFS.glob.
🤖 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.
Duplicate comments:
In `@packages/core/src/loader/loader_fs.ts`:
- Around line 89-102: The glob method currently uses the caller-supplied cwd
verbatim which causes path.join(cwd, file) in the absolute branch to produce
relative paths; change glob to resolve cwd to an absolute path up-front (e.g.,
using path.resolve or similar) before calling this.#toRelative and
this.#listManifestFilesUnder so that both manifest lookup and the
matched.map((file) => path.join(cwd, file)) produce absolute paths; update
references in glob that use cwd (including the cwdRel assignment and the
manifest lookup) so the rest of the method and filterManifestGlob behavior
remain correct and consistent with RealLoaderFS.glob.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec452c67-5f10-4098-a746-998b4a94c070
📒 Files selected for processing (5)
packages/core/package.jsonpackages/core/src/loader/egg_loader.tspackages/core/src/loader/loader_fs.tspackages/core/test/loader/manifest_coverage.test.tspackages/core/test/loader/manifest_loader_fs.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/test/loader/manifest_coverage.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/test/loader/manifest_loader_fs.test.ts
- packages/core/src/loader/egg_loader.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #5943 +/- ##
==========================================
+ Coverage 85.21% 85.29% +0.08%
==========================================
Files 669 670 +1
Lines 19320 19549 +229
Branches 3792 3868 +76
==========================================
+ Hits 16464 16675 +211
- Misses 2464 2482 +18
Partials 392 392 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5aff2fd to
44f3c9a
Compare
44f3c9a to
356c006
Compare
356c006 to
0d758fe
Compare
0d758fe to
21d3094
Compare
21d3094 to
b4a598b
Compare
Summary
ManifestLoaderFS, a manifest-firstLoaderFSimplementation backed byStartupManifestdiscovery/resolve data and the bundled module loader.EggLoaderto useManifestLoaderFSfor bundled stores and manifest-backed startup while keeping real filesystem fallback.Tests
pnpm exec vitest run packages/core/test/loader/loader_fs.test.ts packages/core/test/loader/manifest_loader_fs.test.ts packages/core/test/loader/file_loader.test.ts packages/core/test/loader/manifest_coverage.test.ts packages/core/test/loader/manifest_roundtrip.test.ts --testTimeout 20000pnpm --filter @eggjs/core typecheckpnpm exec oxlint packages/core/src/loader/loader_fs.ts packages/core/test/loader/manifest_loader_fs.test.ts --type-awareIssue: EGG-98
Summary by CodeRabbit
New Features
Refactor
Tests