feat(egg): skip loadRouter in metadataOnly mode#5847
Conversation
AppWorkerLoader.load() now gates loadRouter() behind !metadataOnly, allowing manifest-only startup to skip route registration entirely. This serves the egg-bin manifest generate command. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Deploying egg with
|
| Latest commit: |
07e9277
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6c410714.egg-cci.pages.dev |
| Branch Preview URL: | https://feat-tegg-metadata-only.egg-cci.pages.dev |
📝 WalkthroughWalkthroughThe changes introduce a "metadata-only" mode to the application loader that conditionally skips router initialization based on a flag. A new test fixture app and expanded test suite verify that metadata-only mode prevents router registration and agent creation while maintaining proper lifecycle hook execution. Changes
Sequence DiagramsequenceDiagram
participant Loader as AppWorkerLoader
participant Boot as Boot Hooks
participant Router as Router
participant Agent as Agent
rect rgba(100, 200, 100, 0.5)
Note over Loader,Agent: Metadata-Only Mode (metadataOnly: true)
Loader->>Boot: configWillLoad()
Boot-->>Loader: done
Loader->>Boot: configDidLoad()
Boot-->>Loader: done
Loader->>Boot: loadMetadata()
Boot-->>Loader: done
Note over Router,Agent: Router & Agent initialization skipped
end
rect rgba(100, 150, 200, 0.5)
Note over Loader,Agent: Normal Mode (metadataOnly: false)
Loader->>Boot: configWillLoad()
Boot-->>Loader: done
Loader->>Boot: configDidLoad()
Boot-->>Loader: done
Loader->>Boot: didLoad()
Boot-->>Loader: done
Loader->>Router: loadRouter()
Router-->>Loader: routes registered
Loader->>Agent: create agent
Agent-->>Loader: agent ready
Loader->>Boot: willReady()
Boot-->>Loader: done
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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-v3 with
|
| Latest commit: |
07e9277
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f3e69eb3.egg-v3.pages.dev |
| Branch Preview URL: | https://feat-tegg-metadata-only.egg-v3.pages.dev |
![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 a metadataOnly mode to the AppWorkerLoader, which prevents the router from being loaded when enabled. The changes include a new test fixture and a comprehensive test suite in start.test.ts to verify that lifecycle hooks and route registrations behave correctly in both metadata-only and normal operation modes. A review comment suggests increasing the precision of a test assertion regarding the number of registered routes.
| }); | ||
|
|
||
| it('should register routes', () => { | ||
| assert(app.router.stack.length > 0); |
There was a problem hiding this comment.
你好，这里的断言可以更精确。我们明确知道 metadata-only-app 这个测试应用只注册了一个路由，所以可以直接判断 app.router.stack.length 是否等于 1，而不是 > 0。这样可以让测试用例更加健壮。
| assert(app.router.stack.length > 0); | |
| assert.strictEqual(app.router.stack.length, 1); |
![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
🤖 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/egg/test/start.test.ts`:
- Around line 15-17: The teardown in the afterAll blocks calls await app.close()
unguarded and will throw if beforeAll failed to set app; update both afterAll
handlers to check that the test App instance exists before calling close (e.g.,
if (app) await app.close() or use optional chaining) so teardown is skipped when
setup failed; locate the afterAll closures in this test file (referencing the
app variable used in beforeAll/afterAll) and add the existence guard in both
places.
🪄 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: 12f5d8b3-65c8-4486-afb2-45216789329a
📒 Files selected for processing (6)
packages/egg/src/lib/loader/AppWorkerLoader.tspackages/egg/test/fixtures/apps/metadata-only-app/app.jspackages/egg/test/fixtures/apps/metadata-only-app/app/controller/home.jspackages/egg/test/fixtures/apps/metadata-only-app/app/router.jspackages/egg/test/fixtures/apps/metadata-only-app/package.jsonpackages/egg/test/start.test.ts
| afterAll(async () => { | ||
| await app.close(); | ||
| }); |
There was a problem hiding this comment.
Guard teardown against setup failure in afterAll.
If beforeAll fails, app can be unset and app.close() throws, which can mask the original failure (Line 16 and Line 40).
🛠️ Suggested fix
- let app: SingleModeApplication;
+ let app: SingleModeApplication | undefined;
@@
afterAll(async () => {
- await app.close();
+ if (app) await app.close();
});
@@
- let app: SingleModeApplication;
+ let app: SingleModeApplication | undefined;
@@
afterAll(async () => {
- await app.close();
+ if (app) await app.close();
});Also applies to: 39-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/egg/test/start.test.ts` around lines 15 - 17, The teardown in the
afterAll blocks calls await app.close() unguarded and will throw if beforeAll
failed to set app; update both afterAll handlers to check that the test App
instance exists before calling close (e.g., if (app) await app.close() or use
optional chaining) so teardown is skipped when setup failed; locate the afterAll
closures in this test file (referencing the app variable used in
beforeAll/afterAll) and add the existence guard in both places.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5847 +/- ##
==========================================
+ Coverage 85.39% 85.45% +0.05%
==========================================
Files 666 666
Lines 13171 13171
Branches 1522 1522
==========================================
+ Hits 11248 11255 +7
+ Misses 1791 1785 -6
+ Partials 132 131 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR extends Egg’s metadataOnly startup path (used for manifest generation) by preventing unnecessary route registration during metadata-only boot, and adds tests/fixtures to validate the behavior against normal startup.
Changes:
- Skip
AppWorkerLoader.loadRouter()whenmetadataOnlyis enabled. - Add a dedicated fixture app (
metadata-only-app) with lifecycle hooks and routes for comparison. - Add 6 tests covering metadataOnly vs normal mode: lifecycle hooks, router stack, and agent creation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/egg/src/lib/loader/AppWorkerLoader.ts | Guard loadRouter() behind !options.metadataOnly to avoid route registration during metadata-only boot. |
| packages/egg/test/start.test.ts | New test coverage comparing metadataOnly vs normal mode (lifecycle, router stack, agent). |
| packages/egg/test/fixtures/apps/metadata-only-app/package.json | New fixture app package metadata. |
| packages/egg/test/fixtures/apps/metadata-only-app/app/router.js | Fixture routes used to verify router registration is skipped in metadataOnly mode. |
| packages/egg/test/fixtures/apps/metadata-only-app/app/controller/home.js | Fixture controller for the route. |
| packages/egg/test/fixtures/apps/metadata-only-app/app.js | Fixture boot hook implementation recording lifecycle calls into bootLog. |
| it('should not create agent', () => { | ||
| assert.strictEqual(app.agent, undefined); | ||
| }); |
There was a problem hiding this comment.
In this test app is typed as SingleModeApplication (from test/utils.ts), where agent is declared as a required property. But here metadataOnly mode explicitly expects app.agent to be undefined, so the type annotation is misleading and reduces type-safety for future edits. Consider using a local type with agent?: for metadataOnly tests, or update the shared SingleModeApplication typing to reflect the metadataOnly case (e.g. conditional/union type or separate SingleModeApplicationMetadataOnly).
Summary
AppWorkerLoader.load()在metadataOnly模式下跳过loadRouter()，避免不必要的路由注册egg-bin manifest generate命令的 metadata-only 启动服务Context
PR #5842 manifest 系列的第三部分。lifecycle 层的
loadMetadata/triggerLoadMetadata/metadataOnlyflag 已在 #5844 和 #5846 中合入，本 PR 补全 AppWorkerLoader 侧的守卫。Test plan
loadMetadata，跳过正常生命周期钩子router.stack为空（loadRouter 被跳过）pnpm --filter=egg run test test/start.test.ts6/6 通过🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests