fix(core): auto-inject manifest into FileLoader/ContextLoader getters#5857
fix(core): auto-inject manifest into FileLoader/ContextLoader getters#5857
Conversation
📝 WalkthroughWalkthroughA new test suite validates manifest auto-injection in FileLoader, and the FileLoader constructor is modified to automatically resolve manifest from the application context when not explicitly provided, enabling manifest-driven file discovery. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: |
0fd269b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3c8d59da.egg-cci.pages.dev |
| Branch Preview URL: | https://feat-manifest-complete-cover.egg-cci.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5857 +/- ##
==========================================
- Coverage 85.28% 85.28% -0.01%
==========================================
Files 666 666
Lines 13247 13249 +2
Branches 1538 1539 +1
==========================================
+ Hits 11298 11299 +1
- Misses 1818 1819 +1
Partials 131 131 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
0fd269b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3b2bc86d.egg-v3.pages.dev |
| Branch Preview URL: | https://feat-manifest-complete-cover.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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/test/loader/manifest_coverage.test.ts (1)
45-74: Usetry/finallyfor per-test app cleanup.
await testApp.close()at the tail won’t run if an assertion throws. Wrapping each test body intry/finallymakes cleanup deterministic and reduces test leakage risk.As per coding guidelines, "Use Vitest as the standard test runner for all packages; test files follow pattern 'test/**/*.test.ts'".
Also applies to: 76-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/loader/manifest_coverage.test.ts` around lines 45 - 74, The test creates a testApp and calls await testApp.close() only at the end, which can be skipped if an assertion throws; update the test body that constructs testApp (the it block using createApp('context-loader') and variable testApp and loader/FileLoader usage) to wrap the test logic in a try/finally and call await testApp.close() in the finally so cleanup always runs; apply the same try/finally pattern to the other similar test block around lines 76-102 that also creates testApp.packages/core/src/loader/egg_loader.ts (1)
1687-1696: Consider caching getter-generated subclasses for stable class identity.Both getters create a new class per access. Caching once per
EggLoaderinstance avoids identity drift and is friendlier for plugin code that stores/reuses constructor references.Also applies to: 1702-1709
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/loader/egg_loader.ts` around lines 1687 - 1696, The generated subclass in the FileLoader getter (ManifestFileLoader) is recreated on every access causing unstable class identity; fix by caching the generated class on the EggLoader instance (e.g. add a private field like _ManifestFileLoader or _FileLoaderClass) and change the FileLoader getter to return the cached class if present, otherwise create, store, and return it; apply the same caching pattern to the other similar getter (the one around lines 1702-1709) so both getters return a stable subclass reference across accesses while still injecting the manifest in the constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/loader/egg_loader.ts`:
- Around line 1692-1693: The constructor currently calls super({ manifest,
...options }) which allows a caller-supplied options.manifest (including
undefined) to override the intended injected manifest; change the call so the
injected manifest wins (e.g., pass the spread options first and then manifest so
manifest is last: super({ ...options, manifest }) or equivalently ensure
manifest is assigned as options.manifest ?? manifest) and apply the same change
to the other occurrence around lines 1705-1706; update the constructor in
egg_loader.ts (constructor and the second call site) to ensure manifest cannot
be disabled by caller options.
---
Nitpick comments:
In `@packages/core/src/loader/egg_loader.ts`:
- Around line 1687-1696: The generated subclass in the FileLoader getter
(ManifestFileLoader) is recreated on every access causing unstable class
identity; fix by caching the generated class on the EggLoader instance (e.g. add
a private field like _ManifestFileLoader or _FileLoaderClass) and change the
FileLoader getter to return the cached class if present, otherwise create,
store, and return it; apply the same caching pattern to the other similar getter
(the one around lines 1702-1709) so both getters return a stable subclass
reference across accesses while still injecting the manifest in the constructor.
In `@packages/core/test/loader/manifest_coverage.test.ts`:
- Around line 45-74: The test creates a testApp and calls await testApp.close()
only at the end, which can be skipped if an assertion throws; update the test
body that constructs testApp (the it block using createApp('context-loader') and
variable testApp and loader/FileLoader usage) to wrap the test logic in a
try/finally and call await testApp.close() in the finally so cleanup always
runs; apply the same try/finally pattern to the other similar test block around
lines 76-102 that also creates testApp.
🪄 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: 95daa734-e150-4d08-9c44-ffc8a7dc3b10
📒 Files selected for processing (2)
packages/core/src/loader/egg_loader.tspackages/core/test/loader/manifest_coverage.test.ts
| constructor(options: FileLoaderOptions) { | ||
| super({ manifest, ...options }); |
There was a problem hiding this comment.
Manifest injection can be overridden by caller options.
Line 1693 and Line 1706 spread ...options after manifest, so options.manifest (including undefined) can disable the intended auto-injection.
✅ Proposed fix
return class ManifestFileLoader extends FileLoader {
constructor(options: FileLoaderOptions) {
- super({ manifest, ...options });
+ super({ ...options, manifest });
}
} as typeof FileLoader; return class ManifestContextLoader extends ContextLoader {
constructor(options: ContextLoaderOptions) {
- super({ manifest, ...options });
+ super({ ...options, manifest });
}
} as typeof ContextLoader;Also applies to: 1705-1706
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/loader/egg_loader.ts` around lines 1692 - 1693, The
constructor currently calls super({ manifest, ...options }) which allows a
caller-supplied options.manifest (including undefined) to override the intended
injected manifest; change the call so the injected manifest wins (e.g., pass the
spread options first and then manifest so manifest is last: super({ ...options,
manifest }) or equivalently ensure manifest is assigned as options.manifest ??
manifest) and apply the same change to the other occurrence around lines
1705-1706; update the constructor in egg_loader.ts (constructor and the second
call site) to ensure manifest cannot be disabled by caller options.
There was a problem hiding this comment.
Pull request overview
This PR fixes a manifest-collection gap in @eggjs/core where plugins creating loaders via app.loader.FileLoader / app.loader.ContextLoader were bypassing the manifest system, preventing directory scans and module resolutions from being captured for startup manifest generation.
Changes:
- Updated
EggLoader’sFileLoaderandContextLoadergetters to return subclasses that injectthis.manifestinto loader options. - Added a new test suite to validate that manifest
fileDiscovery/resolveCacheare populated and that keys are stored as relative paths. - Added targeted tests ensuring loaders instantiated via the getter APIs participate in manifest collection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/core/src/loader/egg_loader.ts | Wrapes FileLoader / ContextLoader getters with manifest-injecting subclasses to ensure manifest collection applies to plugin-created loaders. |
| packages/core/test/loader/manifest_coverage.test.ts | Adds coverage tests verifying manifest injection and that collected manifest paths are relative. |
| const manifest = this.manifest; | ||
| // Auto-inject manifest so plugins extending FileLoader | ||
| // automatically participate in manifest collection/caching | ||
| return class ManifestFileLoader extends FileLoader { | ||
| constructor(options: FileLoaderOptions) { | ||
| super({ manifest, ...options }); | ||
| } |
There was a problem hiding this comment.
The injected manifest can be overridden by callers because the spread order is { manifest, ...options }. This allows plugins to accidentally (or intentionally) pass manifest: undefined and bypass manifest collection again. To ensure the getter always injects the loader’s manifest, spread options first and then set manifest last.
| get FileLoader(): typeof FileLoader { | ||
| return FileLoader; | ||
| const manifest = this.manifest; | ||
| // Auto-inject manifest so plugins extending FileLoader | ||
| // automatically participate in manifest collection/caching | ||
| return class ManifestFileLoader extends FileLoader { | ||
| constructor(options: FileLoaderOptions) { | ||
| super({ manifest, ...options }); | ||
| } | ||
| } as typeof FileLoader; |
There was a problem hiding this comment.
get FileLoader() / get ContextLoader() currently creates a new subclass on every property access. That changes the API semantics vs returning a stable class reference, and can break consumer checks like instanceof app.loader.FileLoader (because subsequent getter calls return a different constructor). Consider caching the generated subclass on the loader instance so the getter returns the same class each time.
| const manifest = this.manifest; | ||
| return class ManifestContextLoader extends ContextLoader { | ||
| constructor(options: ContextLoaderOptions) { | ||
| super({ manifest, ...options }); | ||
| } |
There was a problem hiding this comment.
Same issue as FileLoader: { manifest, ...options } lets the caller override/clear the injected manifest, which undermines the guarantee that ContextLoader created via the getter participates in manifest collection. Prefer setting manifest after spreading options so it can’t be overridden.
![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 updates EggLoader to automatically inject the manifest into FileLoader and ContextLoader by returning subclasses from their getters, and adds corresponding test coverage. Feedback highlights that returning a new class definition on every access breaks class identity and instanceof checks, recommending memoization of the subclasses and safer merging of constructor options to prevent accidental manifest overrides.
| get FileLoader(): typeof FileLoader { | ||
| return FileLoader; | ||
| const manifest = this.manifest; | ||
| // Auto-inject manifest so plugins extending FileLoader | ||
| // automatically participate in manifest collection/caching | ||
| return class ManifestFileLoader extends FileLoader { | ||
| constructor(options: FileLoaderOptions) { | ||
| super({ manifest, ...options }); | ||
| } | ||
| } as typeof FileLoader; | ||
| } |
There was a problem hiding this comment.
Returning a new class definition on every access to the FileLoader getter breaks class identity (app.loader.FileLoader !== app.loader.FileLoader) and instanceof checks. This can lead to unexpected behavior in plugins or internal code that rely on class comparisons or type checking.
Additionally, the spread order { manifest, ...options } allows an explicit undefined in options.manifest to override the auto-injected manifest. It is safer to ensure the manifest is provided if missing.
Consider memoizing the subclass on the instance to maintain consistency.
get FileLoader(): typeof FileLoader {
if (!(this as any)._FileLoader) {
const manifest = this.manifest;
// Auto-inject manifest so plugins extending FileLoader
// automatically participate in manifest collection/caching
(this as any)._FileLoader = class ManifestFileLoader extends FileLoader {
constructor(options: FileLoaderOptions) {
super({ ...options, manifest: options.manifest ?? manifest });
}
} as typeof FileLoader;
}
return (this as any)._FileLoader;
}| get ContextLoader(): typeof ContextLoader { | ||
| return ContextLoader; | ||
| const manifest = this.manifest; | ||
| return class ManifestContextLoader extends ContextLoader { | ||
| constructor(options: ContextLoaderOptions) { | ||
| super({ manifest, ...options }); | ||
| } | ||
| } as typeof ContextLoader; | ||
| } |
There was a problem hiding this comment.
Similar to the FileLoader getter, returning a new class definition here breaks class identity and instanceof checks. The subclass should be memoized on the instance to ensure consistency across multiple accesses.
Also, the spread order should be adjusted to prevent options.manifest from accidentally overriding the injected manifest with null or undefined.
get ContextLoader(): typeof ContextLoader {
if (!(this as any)._ContextLoader) {
const manifest = this.manifest;
(this as any)._ContextLoader = class ManifestContextLoader extends ContextLoader {
constructor(options: ContextLoaderOptions) {
super({ ...options, manifest: options.manifest ?? manifest });
}
} as typeof ContextLoader;
}
return (this as any)._ContextLoader;
}
FileLoader now auto-resolves manifest from inject.loader?.manifest when not explicitly provided. This replaces the previous anonymous subclass approach in EggLoader getters, making the code simpler and more elegant while ensuring all file discovery and resolution is captured for snapshot/manifest generation. Co-Authored-By: Claude Opus 4.6 <[email protected]>
killagu
force-pushed
the
feat/manifest-complete-coverage
branch
from
0036281 to
0fd269b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/loader/file_loader.ts`:
- Around line 95-98: The code auto-assigns options.manifest from
options.inject.loader?.manifest without validating shape, which can cause
parse() (and its call to globFiles(...)) to fail; fix by checking the inferred
manifest is the expected manifest store before assignment—ensure
options.inject.loader?.manifest is an object and implements the required API
(e.g., has a callable globFiles method) and only then set options.manifest;
otherwise leave it undefined so parse() can handle missing manifest gracefully.
🪄 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: 994cda2a-9697-4585-b2af-c612a64e5eb8
📒 Files selected for processing (2)
packages/core/src/loader/file_loader.tspackages/core/test/loader/manifest_coverage.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/test/loader/manifest_coverage.test.ts
| // Auto-resolve manifest from inject (the app) when not explicitly provided | ||
| if (!options.manifest && options.inject) { | ||
| options.manifest = options.inject.loader?.manifest; | ||
| } |
There was a problem hiding this comment.
Guard inferred manifest shape before assigning it
At Line 97, options.inject.loader?.manifest is assigned without validating it is a real manifest store. If inject contains an unrelated loader.manifest value, parse() will later fail when calling globFiles(...).
Proposed fix
- if (!options.manifest && options.inject) {
- options.manifest = options.inject.loader?.manifest;
- }
+ if (!options.manifest && options.inject) {
+ const candidate = (options.inject as { loader?: { manifest?: unknown } }).loader?.manifest;
+ if (candidate && typeof (candidate as ManifestStore).globFiles === 'function') {
+ options.manifest = candidate as ManifestStore;
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/loader/file_loader.ts` around lines 95 - 98, The code
auto-assigns options.manifest from options.inject.loader?.manifest without
validating shape, which can cause parse() (and its call to globFiles(...)) to
fail; fix by checking the inferred manifest is the expected manifest store
before assignment—ensure options.inject.loader?.manifest is an object and
implements the required API (e.g., has a callable globFiles method) and only
then set options.manifest; otherwise leave it undefined so parse() can handle
missing manifest gracefully.
|
PR title/body 描述的方案是修改 实际方案比描述的更好（更简单、不需要改 getter、ContextLoader 自动继承生效），只是 title 和 body 需要同步更新以匹配实际实现，避免 reviewer 对着描述找不到对应代码。 |
Summary
FileLoaderinstances viaapp.loader.FileLoader(e.g., the schedule plugin) were bypassing the manifest system because the getter returned the raw class without injecting the manifest storeFileLoaderandContextLoadergetters return subclasses that auto-inject the manifest, ensuring all file discovery and resolution is captured for snapshot/manifest generationFileLoaderandContextLoadercreated through the getter APIProblem
When plugins do:
The
FileLoadergetter previously returned the rawFileLoaderclass, so the created instances had no manifest attached. This meant file discovery results from these loaders were lost and not included in the generated manifest.Solution
The
FileLoaderandContextLoadergetters now return anonymous subclasses that auto-injectthis.manifestinto the constructor options:This is transparent to callers — they use the same API, but the manifest is always injected.
Test plan
generateManifest()produces non-emptyfileDiscoveryandresolveCacheafter loadingFileLoadercreated viaapp.loader.FileLoadergetter auto-injects manifestContextLoadercreated viaapp.loader.ContextLoadergetter auto-injects manifest🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Bug Fixes