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 as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR extracts the LoaderFS interface and RealLoaderFS implementation into a new ChangesLoaderFS Package Extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: |
f646ac9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://97f7a3d7.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-fc575ee0.egg-cci.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5945 +/- ##
=======================================
Coverage 85.21% 85.21%
=======================================
Files 669 669
Lines 19304 19316 +12
Branches 3787 3789 +2
=======================================
+ Hits 16449 16461 +12
Misses 2463 2463
Partials 392 392 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
f646ac9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6079ee6b.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-fc575ee0.egg-v3.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/test/loader/loader_fs.test.ts (1)
31-34: ⚡ Quick winAvoid a tautological
loadFilecomparison.This now compares two paths to the same implementation (
CoreRealLoaderFSis a re-export), so regressions can slip through if both sides change together. Prefer one explicit re-export identity check plus a behavior assertion against an independent expected value/fixture.🤖 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/test/loader/loader_fs.test.ts` around lines 31 - 34, The test currently compares loaderFS.loadFile against new CoreRealLoaderFS().loadFile which is tautological because CoreRealLoaderFS is a re-export; instead, assert that loaderFS is the exact re-export (e.g., assert.strictEqual(loaderFS, CoreRealLoaderFS) or equivalent identity check) and then assert loadFile behavior against an independent expected value/fixture (e.g., call loaderFS.loadFile(path.join(baseDir, 'object.js')) and compare the result to a known fixture or fs.readFileSync of that file). Locate symbols loaderFS, CoreRealLoaderFS and the loadFile call to make these two changes.
🤖 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/test/loader/loader_fs.test.ts`:
- Around line 31-34: The test currently compares loaderFS.loadFile against new
CoreRealLoaderFS().loadFile which is tautological because CoreRealLoaderFS is a
re-export; instead, assert that loaderFS is the exact re-export (e.g.,
assert.strictEqual(loaderFS, CoreRealLoaderFS) or equivalent identity check) and
then assert loadFile behavior against an independent expected value/fixture
(e.g., call loaderFS.loadFile(path.join(baseDir, 'object.js')) and compare the
result to a known fixture or fs.readFileSync of that file). Locate symbols
loaderFS, CoreRealLoaderFS and the loadFile call to make these two changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbdac7e0-162c-4860-ac89-0e0425c7b78e
📒 Files selected for processing (22)
packages/core/package.jsonpackages/core/src/loader/egg_loader.tspackages/core/src/loader/file_loader.tspackages/core/src/loader/loader_fs.tspackages/core/test/loader/egg_loader.test.tspackages/core/test/loader/file_loader.test.tspackages/core/test/loader/loader_fs.test.tspackages/loader-fs/README.mdpackages/loader-fs/package.jsonpackages/loader-fs/src/index.tspackages/loader-fs/test/fixtures/loadfile/data.txtpackages/loader-fs/test/fixtures/loadfile/null.jspackages/loader-fs/test/fixtures/loadfile/object.jspackages/loader-fs/test/fixtures/loadfile/package.jsonpackages/loader-fs/test/loader_fs.test.tspackages/loader-fs/tsconfig.jsonpackages/loader-fs/tsdown.config.tstsconfig.jsonwiki/index.mdwiki/log.mdwiki/packages/core.mdwiki/packages/loader-fs.md
![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 extracts the LoaderFS filesystem abstraction into a new standalone package, @eggjs/loader-fs, to facilitate reuse across different runtimes. The @eggjs/core package has been updated to depend on this new package while maintaining backward compatibility through re-exports. Feedback was provided regarding the error handling in the loadFile method, specifically suggesting a more robust approach for handling non-Error exceptions and removing verbose tracing in favor of attaching the original cause to the rethrown error.
There was a problem hiding this comment.
Pull request overview
This PR extracts the loader-facing filesystem boundary (LoaderFS) into a new shared workspace package (@eggjs/loader-fs), then updates @eggjs/core loader internals to consume it while preserving the existing @eggjs/core re-exports and import paths for compatibility.
Changes:
- Added new workspace package
@eggjs/loader-fswithLoaderFS,LoaderFSGlobOptions, andRealLoaderFSplus focused tests/fixtures. - Updated
@eggjs/coreloader code (EggLoader,FileLoader) and tests to use@eggjs/loader-fs, while keepingpackages/core/src/loader/loader_fs.tsas a compatibility re-export. - Added/updated wiki documentation pages and changelog entries describing the new shared boundary.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| wiki/packages/loader-fs.md | New wiki page documenting the new shared @eggjs/loader-fs boundary and core compatibility story. |
| wiki/packages/core.md | Updates core wiki docs to point LoaderFS ownership to @eggjs/loader-fs while noting re-exports. |
| wiki/log.md | Adds a 2026-05-10 log entry for the extraction change. |
| wiki/index.md | Adds LoaderFS Package to the wiki index. |
| tsconfig.json | Adds packages/loader-fs to TS project references. |
| packages/loader-fs/tsdown.config.ts | Adds tsdown build entry for the new package. |
| packages/loader-fs/tsconfig.json | Adds package tsconfig extending root config. |
| packages/loader-fs/test/loader_fs.test.ts | New tests validating RealLoaderFS behavior (exists/stat/realpath/readJSON/glob/loadFile). |
| packages/loader-fs/test/fixtures/loadfile/package.json | Fixture to force CommonJS semantics for loadFile tests. |
| packages/loader-fs/test/fixtures/loadfile/object.js | Fixture module exporting an object. |
| packages/loader-fs/test/fixtures/loadfile/null.js | Fixture module exporting null (used for glob ignore test). |
| packages/loader-fs/test/fixtures/loadfile/data.txt | Fixture non-module file to verify buffer-loading behavior. |
| packages/loader-fs/src/index.ts | Implements LoaderFS interface and RealLoaderFS default implementation. |
| packages/loader-fs/README.md | Adds a concise README describing scope and backing implementation details. |
| packages/loader-fs/package.json | Defines the new package metadata/deps and workspace/publish export mappings. |
| packages/core/test/loader/loader_fs.test.ts | Updates tests to validate @eggjs/loader-fs usage and core compatibility re-export. |
| packages/core/test/loader/file_loader.test.ts | Updates imports to use @eggjs/loader-fs types/impls. |
| packages/core/test/loader/egg_loader.test.ts | Updates imports to use @eggjs/loader-fs for loaderFS injection tests. |
| packages/core/src/loader/loader_fs.ts | Converts previous implementation into a compatibility re-export from @eggjs/loader-fs. |
| packages/core/src/loader/file_loader.ts | Switches default loaderFS implementation import to @eggjs/loader-fs. |
| packages/core/src/loader/egg_loader.ts | Switches default loaderFS implementation import to @eggjs/loader-fs. |
| packages/core/package.json | Adds @eggjs/loader-fs as a workspace dependency. |
|
Addressed review feedback in f646ac9:
Re-ran focused typecheck, tests, lint, and |
Summary
@eggjs/loader-fsas a shared workspace package owningLoaderFS,LoaderFSGlobOptions, andRealLoaderFS.@eggjs/coreloader internals to consume@eggjs/loader-fswhile preserving existing core re-exports.Verification
pnpm --filter @eggjs/loader-fs typecheckpnpm --filter @eggjs/core typecheckpnpm exec vitest run packages/loader-fs/test/loader_fs.test.ts packages/core/test/loader/loader_fs.test.ts packages/core/test/loader/file_loader.test.ts packages/core/test/loader/egg_loader.test.ts --testTimeout 20000 --hookTimeout 20000pnpm exec vitest run packages/core/test/index.test.ts --testTimeout 20000 --hookTimeout 20000pnpm exec oxlint --type-aware --type-check packages/loader-fs/src/index.ts packages/loader-fs/test/loader_fs.test.ts packages/core/src/loader/loader_fs.ts packages/core/src/loader/egg_loader.ts packages/core/src/loader/file_loader.ts packages/core/test/loader/loader_fs.test.ts packages/core/test/loader/egg_loader.test.ts packages/core/test/loader/file_loader.test.ts(0 errors; 7 existing warnings inpackages/core/src/loader/egg_loader.ts)pnpm exec tsdown --filter @eggjs/loader-fsRefs EGG-102.
Summary by CodeRabbit
Chores
Documentation
Tests