-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(core): add snapshot mode to lifecycle (stop before configDidLoad phase) #5852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
4d5c5fd
f43d47c
b425d9b
b7b7679
3235abe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -69,6 +69,12 @@ export interface LifecycleOptions { | |||||||||||||||
| baseDir: string; | ||||||||||||||||
| app: EggCore; | ||||||||||||||||
| logger: EggConsoleLogger; | ||||||||||||||||
| /** | ||||||||||||||||
| * When true, the lifecycle stops after didLoad phase completes. | ||||||||||||||||
| * willReady, didReady, and serverDidReady hooks are NOT called. | ||||||||||||||||
| * Used for V8 startup snapshot construction. | ||||||||||||||||
|
||||||||||||||||
| * When true, the lifecycle stops after didLoad phase completes. | |
| * willReady, didReady, and serverDidReady hooks are NOT called. | |
| * Used for V8 startup snapshot construction. | |
| * When true, the lifecycle's normal flow stops after the didLoad phase | |
| * completes, and willReady, didReady, and serverDidReady hooks are not | |
| * invoked automatically. External callers may still trigger those hooks | |
| * manually if needed. Used for V8 startup snapshot construction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is by design. The JSDoc documents what the lifecycle flow does automatically — it does not call serverDidReady. External callers invoking triggerServerDidReady() directly would be doing so intentionally and at their own risk.
Making it a no-op would prevent legitimate use cases where snapshot consumers need to selectively trigger later phases. The current wording ("hooks are NOT called") is accurate for the lifecycle's own behavior. Won't change, but appreciate the thoroughness.
Copilot
AI
Mar 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description says snapshot mode “stops after didLoad”, but the implementation (and this JSDoc) now short-circuits immediately after configWillLoad (before configDidLoad). Please update the PR description (or rename/clarify the feature wording) so reviewers/users don’t get conflicting semantics.
Outdated
Copilot
AI
Mar 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snapshot-mode behavior (marking app ready after didLoad and skipping willReady/didReady) isn’t covered by tests in packages/core/test. Since this alters lifecycle semantics, please add a unit test that asserts the executed hook sequence in snapshot mode and verifies willReady/didReady are not invoked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that tests are needed. Snapshot mode tests will be included in the integration test PR (feat/snapshot-scripts, PR #5853) which exercises the full snapshot build + restore flow end-to-end. Adding isolated lifecycle unit tests here would duplicate that coverage. The core change is minimal (3 lines of control flow) and well-understood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
snapshotoption comment here claims the app loads metadata only and avoids starting servers/timers/connections, but in this PR snapshot mode still runs the normal lifecycle throughdidLoad(includingconfigWillLoad/configDidLoad,didLoadhooks, andregisterBeforeStarttasks). That code can still start timers/connections. Please adjust the doc to match the actual semantics (stop afterdidLoad/ skip ready phases), or enforce the stronger guarantee in code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggested 8-line JSDoc is too verbose for a simple boolean option. The key information developers need is: (1) lifecycle stops after didLoad, (2) later phases are skipped, (3) purpose is snapshot construction.
Will update to a concise version that accurately reflects the behavior without over-documenting edge cases that are better covered in framework docs. See reply to CodeRabbit's similar comment above.