feat(core): add manifest-backed loader fs#5943
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 ignored due to path filters (1)
📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds ManifestLoaderFS as a manifest-backed LoaderFS, integrates it into EggLoader initialization and plugin package.json reads, records discovered package.json entries into startup manifest.fileDiscovery, adds multimatch for manifest-aware globbing, and introduces tests covering manifest-backed behavior and fallback. ChangesManifest-Based Filesystem Loading
Sequence Diagram(s)sequenceDiagram
participant Client as EggLoader
participant ManifestStore
participant ManifestLoaderFS
participant RealLoaderFS
Client->>ManifestStore: check for manifest for baseDir
alt manifest exists
Client->>ManifestLoaderFS: construct with manifest + fallback RealLoaderFS
else no manifest
Client->>RealLoaderFS: use RealLoaderFS or provided loaderFS
end
Client->>ManifestLoaderFS: loaderFS.exists(package.json)
Client->>ManifestLoaderFS: loaderFS.readJSON(package.json)
Client->>Client: collectConventionFile -> 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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: |
44f3c9a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f0af3aee.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-1539d7a6.egg-cci.pages.dev |
Deploying egg-v3 with
|
| Latest commit: |
44f3c9a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1cf5af81.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-1539d7a6.egg-v3.pages.dev |
killagu
force-pushed
the
agent/egg-dev/1539d7a6
branch
from
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. |
killagu
force-pushed
the
agent/egg-dev/1539d7a6
branch
from
f7dc977 to
aba2f64
Compare
killagu
force-pushed
the
agent/egg-dev/1539d7a6
branch
from
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.26% +0.04%
==========================================
Files 669 669
Lines 19304 19498 +194
Branches 3787 3849 +62
==========================================
+ Hits 16449 16624 +175
- Misses 2463 2482 +19
Partials 392 392 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
killagu
force-pushed
the
agent/egg-dev/1539d7a6
branch
from
5aff2fd to
44f3c9a
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