feat(tegg): load modules through loader fs#5954
Open
killagu wants to merge 5 commits into
Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughThreads an injectable LoaderFS through loader creation and controller discovery, refactors LoaderUtil module import and export normalization, updates package deps, and adds tests plus a CJS fixture. ChangesLoaderFS Abstraction Implementation
EggControllerLoader Filesystem Integration
EggModuleLoader LoaderFS Propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors file discovery across the loaders by replacing direct globby usage with a customizable LoaderFS abstraction from @eggjs/loader-fs and integrating manifest-backed discovery. It also updates module loading to utilize @eggjs/utils. The feedback highlights a potential issue in EggControllerLoader.ts where a missing globFiles method on a defined manifest object would cause a silent failure due to an overly broad try-catch block; a defensive check is recommended to prevent this.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #5954 +/- ##
==========================================
+ Coverage 85.30% 85.32% +0.02%
==========================================
Files 670 670
Lines 19552 19569 +17
Branches 3863 3872 +9
==========================================
+ Hits 16678 16697 +19
+ Misses 2481 2480 -1
+ Partials 393 392 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg with
|
| Latest commit: |
adbd7b0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://94eb4ae0.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-0f525f9a.egg-cci.pages.dev |
killagu
force-pushed
the
agent/egg-dev/0f525f9a
branch
from
b02d496 to
13fa530
Compare
Deploying egg-v3 with
|
| Latest commit: |
adbd7b0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://314c0cfb.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-0f525f9a.egg-v3.pages.dev |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR routes tegg module and controller discovery through the shared LoaderFS abstraction so bundled/manifest-backed apps can reuse Egg’s injected loader filesystem instead of direct globby access.
Changes:
- Adds optional
loaderFSplumbing throughLoaderFactory,ModuleLoader, tegg plugin module loading, and controller loading. - Updates
LoaderUtil.loadFileto use@eggjs/utilsimport semantics while preserving bundle-loader fallback behavior. - Adds tests covering loaderFS-backed module/controller discovery and manifest-backed controller discovery.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
tegg/plugin/tegg/src/lib/EggModuleLoader.ts |
Passes app.loader.loaderFS into tegg module loading paths. |
tegg/plugin/controller/src/lib/EggControllerLoader.ts |
Uses LoaderFS and optional manifest-backed discovery for controllers. |
tegg/plugin/controller/src/app.ts |
Registers controller loader with app loaderFS and manifest. |
tegg/core/loader/src/LoaderFactory.ts |
Adds loader options and forwards loaderFS to created loaders. |
tegg/core/loader/src/impl/ModuleLoader.ts |
Replaces direct globby use with injected LoaderFS.glob. |
tegg/core/loader/src/LoaderUtil.ts |
Switches normal imports to importModule and preserves bundle fallback import behavior. |
tegg/core/loader/package.json |
Adds loader-fs/utils dependencies and removes direct globby dependency. |
tegg/plugin/controller/package.json |
Adds loader-fs dependency and removes direct globby dependency. |
tegg/core/loader/test/ModuleLoaderManifest.test.ts |
Adds coverage for ModuleLoader loaderFS discovery. |
tegg/core/loader/test/LoaderFactoryManifest.test.ts |
Adds coverage for LoaderFactory passing loaderFS. |
tegg/plugin/controller/test/lib/EggControllerLoader.test.ts |
Adds coverage for controller loaderFS and manifest-backed discovery. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tegg/core/loader/src/LoaderUtil.ts`:
- Around line 84-87: The imported value from importModule(filePath, {
importDefaultOnly: false }) can be a bare CommonJS default (a function/class),
so before the subsequent Object.keys(exports) in loadFile(), normalize exports:
if exports is null or not an object (typeof exports !== 'object') or if it has
only a non-object default, wrap it into an object like { default: exports } (or
{ default: exports.default } as appropriate) so that default-exported protos are
discoverable; adjust the code around the exports assignment (the importModule
branch in LoaderUtil.ts) to perform this normalization before any
Object.keys(exports) usage.
🪄 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: 345d6fd8-8ae0-4ea0-8a5d-e0a58baaf742
📒 Files selected for processing (11)
tegg/core/loader/package.jsontegg/core/loader/src/LoaderFactory.tstegg/core/loader/src/LoaderUtil.tstegg/core/loader/src/impl/ModuleLoader.tstegg/core/loader/test/LoaderFactoryManifest.test.tstegg/core/loader/test/ModuleLoaderManifest.test.tstegg/plugin/controller/package.jsontegg/plugin/controller/src/app.tstegg/plugin/controller/src/lib/EggControllerLoader.tstegg/plugin/controller/test/lib/EggControllerLoader.test.tstegg/plugin/tegg/src/lib/EggModuleLoader.ts
killagu
force-pushed
the
agent/egg-dev/0f525f9a
branch
from
13fa530 to
6070490
Compare
Contributor
|
Actionable comments posted: 0 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tegg/plugin/controller/test/lib/EggControllerLoader.test.ts (1)
10-10: ⚡ Quick winConsider importing global types for consistency.
Line 10 uses
(globalThis as any)to access__EGG_BUNDLE_MODULE_LOADER__, while the ModuleLoaderManifest test file imports@eggjs/typings/globalfor type safety. Consider adding the same type import here for consistency:import type {} from '`@eggjs/typings/global`';This eliminates the need for the
as anycast and ensures TypeScript is aware of the global declaration.🤖 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 `@tegg/plugin/controller/test/lib/EggControllerLoader.test.ts` at line 10, Add a global type import to the top of the test file by adding `import type {} from '`@eggjs/typings/global`';` and then remove the unsafe cast by replacing `(globalThis as any).__EGG_BUNDLE_MODULE_LOADER__ = undefined;` with a direct access `globalThis.__EGG_BUNDLE_MODULE_LOADER__ = undefined;` so TypeScript recognizes the global declaration for `__EGG_BUNDLE_MODULE_LOADER__`.
🤖 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 `@tegg/core/loader/test/ModuleLoaderManifest.test.ts`:
- Line 91: The test assertion compares requestedFiles (which
LoaderUtil.loadFile() normalizes to use forward slashes) against serviceFile and
repoFile (built with path.join, which on Windows uses backslashes), causing
platform-dependent failures; update the test so serviceFile and repoFile are
normalized to the same form as requestedFiles before asserting (e.g., call the
same normalization used by LoaderUtil.loadFile() or replace backslashes with
forward slashes) so that the comparison of requestedFiles, serviceFile, and
repoFile succeeds cross-platform.
In `@tegg/plugin/controller/test/lib/EggControllerLoader.test.ts`:
- Line 8: The test suite description string is wrong: change the describe(...)
call that currently reads 'plugin/controller/test/lib/EggModuleLoader.test.ts'
to 'plugin/controller/test/lib/EggControllerLoader.test.ts' so the describe
matches the actual test file/name and the class under test
(EggControllerLoader); update the describe argument where it appears in the test
file.
---
Nitpick comments:
In `@tegg/plugin/controller/test/lib/EggControllerLoader.test.ts`:
- Line 10: Add a global type import to the top of the test file by adding
`import type {} from '`@eggjs/typings/global`';` and then remove the unsafe cast
by replacing `(globalThis as any).__EGG_BUNDLE_MODULE_LOADER__ = undefined;`
with a direct access `globalThis.__EGG_BUNDLE_MODULE_LOADER__ = undefined;` so
TypeScript recognizes the global declaration for `__EGG_BUNDLE_MODULE_LOADER__`.
🪄 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: b527eebc-4fc4-45a1-b979-7475b99fc9f0
📒 Files selected for processing (2)
tegg/core/loader/test/ModuleLoaderManifest.test.tstegg/plugin/controller/test/lib/EggControllerLoader.test.ts
Contributor
|
Actionable comments posted: 0 |
Contributor
|
Actionable comments posted: 0 |
Comment on lines
+43
to
+45
| const moduleDescriptors = await LoaderFactory.loadApp(this.app.moduleReferences, loadAppManifest, { | ||
| loaderFS: this.app.loader.loaderFS, | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests
Summary by CodeRabbit
Tests
Refactor
Bug Fixes
Chores