fix: use lifecycle instead of beforeStart#297
Conversation
WalkthroughThis pull request changes how lifecycle events are registered by replacing the direct call to Changes
Sequence Diagram(s)sequenceDiagram
participant EC as EggCore
participant LS as Lifecycle
participant CB as Callback
EC->>LS: registerBeforeStart(callback, `${name}-singleton-init`)
LS->>CB: Schedule async callback for initPromise
CB-->>LS: Execute callback (await initPromise)
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
src/egg.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. test/singleton.test.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. src/singleton.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the deprecated beforeStart API with lifecycle.registerBeforeStart and updates an error message in the singleton module.
- Replaces beforeStart with lifecycle.registerBeforeStart in the EggCore class.
- Updates the error message in the Singleton module.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/egg.ts | Replaced the beforeStart hook with lifecycle.registerBeforeStart, adding a label parameter. |
| src/singleton.ts | Modified an error message regarding supported creation modes. |
Comments suppressed due to low confidence (1)
src/singleton.ts:110
- The error message was updated to say 'only support asynchronous creation', which contradicts the assertion logic that disallows asynchronous creation. Please verify the intended behavior and update the error message to accurately reflect that only synchronous creation is supported.
assert(!isAsyncFunction(this.create),
commit: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #297 +/- ##
=======================================
Coverage 97.17% 97.17%
=======================================
Files 11 11
Lines 3578 3578
Branches 632 632
=======================================
Hits 3477 3477
Misses 101 101 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/egg.ts (1)
242-244: Consider adding tests for lifecycle event registrationThe static analysis indicates these lines aren't covered by tests. Consider adding tests to ensure the singleton lifecycle registration works correctly.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 242-242: src/egg.ts#L242
Added line #L242 was not covered by tests
[warning] 244-244: src/egg.ts#L244
Added line #L244 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/egg.ts(1 hunks)src/singleton.ts(1 hunks)test/singleton.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/egg.ts
[warning] 242-242: src/egg.ts#L242
Added line #L242 was not covered by tests
[warning] 244-244: src/egg.ts#L244
Added line #L244 was not covered by tests
🔇 Additional comments (3)
src/singleton.ts (1)
111-111: LGTM: Error message correction for createInstanceThe error message has been properly updated to indicate that the Singleton class only supports asynchronous creation through createInstanceAsync. This matches the actual behavior and is more accurate than before.
test/singleton.test.ts (1)
360-360: LGTM: Updated test to match new error messageThe test expectation has been correctly updated to match the new error message from the Singleton class, ensuring consistency between implementation and tests.
src/egg.ts (1)
242-245:Details
❓ Verification inconclusive
LGTM: Using lifecycle manager instead of direct beforeStart
The code now properly uses the lifecycle manager to register the before start event, which aligns with the PR objective to replace direct beforeStart calls with lifecycle management.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining direct beforeStart calls in the codebase rg "this\.beforeStart\(" --type tsLength of output: 36
Action Required: Confirm Complete Removal of Direct
beforeStartCallsThe refactoring to use the lifecycle manager (via
this.lifecycle.registerBeforeStart) appears correct and aligns with the PR objective. However, the shell script used to search for directbeforeStartcalls returned no output, which is ambiguous. Please manually verify that there are no remaining direct invocations ofbeforeStartin the codebase.
- Location under review:
src/egg.ts(Lines: 242-245)- Action: Double-check the entire repository to ensure all direct calls to
this.beforeStart()have been replaced with lifecycle manager usage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 242-242: src/egg.ts#L242
Added line #L242 was not covered by tests
[warning] 244-244: src/egg.ts#L244
Added line #L244 was not covered by tests
[skip ci] ## [6.4.1](v6.4.0...v6.4.1) (2025-03-22) ### Bug Fixes * use lifecycle instead of beforeStart ([#297](#297)) ([26c95a7](26c95a7))
Summary by CodeRabbit