feat: mv Singleton from egg #288
Conversation
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces several enhancements to the Egg.js framework, focusing on singleton management and logging capabilities. Key changes include the addition of a new Changes
Sequence DiagramsequenceDiagram
participant App as EggCore
participant Singleton
participant Client as Singleton Instance
App->>Singleton: addSingleton(name, createMethod)
Singleton-->>App: Register singleton initialization
App->>Singleton: init()
alt Sync Creation
Singleton->>Client: createInstance()
else Async Creation
Singleton->>Client: createInstanceAsync()
end
Singleton-->>App: Initialized Client
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
commit: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #288 +/- ##
==========================================
- Coverage 97.76% 97.51% -0.26%
==========================================
Files 10 11 +1
Lines 3363 3544 +181
Branches 599 630 +31
==========================================
+ Hits 3288 3456 +168
- Misses 75 88 +13 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](/~https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/singleton.ts (2)
32-61: Refactor duplicated code ininitSync()andinitAsync()methodsThe
initSync()andinitAsync()methods contain duplicated logic for handlingoptions.clientandoptions.clients. Refactoring the common code into a shared private method would improve maintainability and reduce redundancy.Consider creating a private method like
initClients()to handle the shared logic. For example:private initClients(clientGetter: (config: Record<string, any>, name: string) => T | Promise<T>) { const options = this.options; if (options.client) { const client = clientGetter(options.client, options.name); this.#setClientToApp(client); this.#extendDynamicMethods(client); return; } if (options.clients) { const clientEntries = Object.entries(options.clients); const clientsPromise = clientEntries.map(([id, config]) => Promise.resolve(clientGetter(config as Record<string, any>, id)) .then(client => this.clients.set(id, client as T)) ); return Promise.all(clientsPromise) .then(() => { this.#setClientToApp(this); }); } // No config.clients and config.client this.#setClientToApp(this); }Then,
initSync()andinitAsync()can utilize this method:initSync() { assert(!isAsyncFunction(this.create), `[egg:singleton] ${this.name} create method must be synchronous`); this.initClients(this.createInstance.bind(this)); } async initAsync() { assert(isAsyncFunction(this.create), `[egg:singleton] ${this.name} create method must be asynchronous`); await this.initClients(this.createInstanceAsync.bind(this)); }Also applies to: 63-88
94-99: Deprecation ofget(id: string)methodThe
get(id: string)method is marked as deprecated in favor ofgetSingletonInstance(id). To enforce this deprecation:
- Use the
@deprecatedJSDoc tag properly so that IDEs and compilers can warn users.- If possible, consider removing the deprecated method in a future major release to clean up the API.
Here's how you can update the JSDoc:
/** - * @deprecated please use `getSingletonInstance(id)` instead + * @deprecated Use `getSingletonInstance(id)` instead. */test/singleton.test.ts (1)
26-29: Consider using undefined assignment instead of delete operator.The delete operator can impact performance. Consider using undefined assignment instead:
afterEach(() => { - delete (DataService as any).prototype.createInstance; - delete (DataService as any).prototype.createInstanceAsync; + (DataService as any).prototype.createInstance = undefined; + (DataService as any).prototype.createInstanceAsync = undefined; });🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 28-28: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/egg.ts(3 hunks)src/index.ts(1 hunks)src/singleton.ts(1 hunks)test/index.test.ts(1 hunks)test/singleton.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/egg.ts
[warning] 192-193: src/egg.ts#L192-L193
Added lines #L192 - L193 were not covered by tests
[warning] 196-197: src/egg.ts#L196-L197
Added lines #L196 - L197 were not covered by tests
[warning] 205-217: src/egg.ts#L205-L217
Added lines #L205 - L217 were not covered by tests
🪛 Biome (1.9.4)
test/singleton.test.ts
[error] 27-27: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 28-28: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Node.js / Test (windows-latest, 23)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18.19.0)
- GitHub Check: Node.js / Test (windows-latest, 18)
🔇 Additional comments (5)
src/index.ts (1)
8-8: ExposeSingletonin module exportsThe addition of
export * from './singleton.js';properly re-exports theSingletonclass and its associated types, making them available for external use.test/index.test.ts (1)
32-32: Confirm inclusion of 'Singleton' in exported propertiesThe test has been updated to include
'Singleton'in the list of expected keys fromEggCore. This change ensures that theSingletonclass is correctly exported and available for use.Please run the test suite to confirm that all assertions pass and that
Singletonis accessible as intended.test/singleton.test.ts (3)
172-172: Verify error handling for createInstanceAsync.The test calls createInstanceAsync but doesn't verify error cases. Consider adding tests for:
- Invalid config parameters
- Initialization failures
- Timeout scenarios
Also applies to: 201-201
358-365: LGTM! Good error handling test.The test properly verifies that synchronous creation is rejected when only asynchronous creation is supported.
150-176: LGTM! Comprehensive edge case testing.Excellent test coverage for unextensible and frozen objects. The tests verify that the singleton functionality works correctly with Object.preventExtensions and Object.freeze.
Also applies to: 178-205
| addSingleton(name: string, create: SingletonCreateMethod) { | ||
| const options: SingletonOptions = { | ||
| name, | ||
| create, | ||
| app: this, | ||
| }; | ||
| const singleton = new Singleton(options); | ||
| const initPromise = singleton.init(); | ||
| if (initPromise) { | ||
| this.beforeStart(async () => { | ||
| await initPromise; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for addSingleton method in egg.test.ts
While the Singleton class and beforeStart hook are well tested individually, the addSingleton method needs dedicated test coverage to verify:
- Integration between Singleton creation and beforeStart hook registration
- Error handling for invalid inputs
- Proper initialization flow
- Cases where init returns a promise vs no promise
🔗 Analysis chain
Add test coverage for addSingleton method.
The addSingleton method implementation looks correct but lacks test coverage. The method:
- Creates a new Singleton instance
- Initializes it
- Registers a beforeStart hook if initialization is asynchronous
However, we should verify:
- Error handling for invalid inputs
- The beforeStart hook registration
- The initialization promise handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing addSingleton tests
rg -l "addSingleton" test/
Length of output: 26
Script:
#!/bin/bash
# Find all test files
echo "=== All test files ==="
fd -e test.ts -e spec.ts -e test.js -e spec.js
echo -e "\n=== Files containing 'singleton' (case-insensitive) ==="
rg -il "singleton"
echo -e "\n=== Test patterns for initialization ==="
rg "beforeStart|init\(" test/ || true
Length of output: 5955
Script:
#!/bin/bash
# Check singleton test content
echo "=== Singleton test content ==="
rg "addSingleton|describe\('Singleton" -A 5 test/singleton.test.ts
echo -e "\n=== Egg test singleton content ==="
rg "addSingleton|describe\('singleton" -A 5 test/egg.test.ts
Length of output: 293
Script:
#!/bin/bash
# View complete singleton test content
echo "=== Complete singleton test content ==="
cat test/singleton.test.ts
echo -e "\n=== Relevant egg test sections ==="
rg -B 2 -A 10 "singleton|Singleton" test/egg.test.ts
Length of output: 11116
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 205-217: src/egg.ts#L205-L217
Added lines #L205 - L217 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/singleton.ts (2)
25-25: Fix grammatical error in assertion messageThe assertion message on line 25 contains a grammatical error. It should read "already exists" instead of "is already exists".
Apply this diff to correct the message:
- assert(!(options.name in options.app), `[@eggjs/core/singleton] ${options.name} is already exists in app`); + assert(!(options.name in options.app), `[@eggjs/core/singleton] ${options.name} already exists in app`);
111-111: Fix grammatical error in assertion message increateInstance()The assertion message in
createInstance()contains a grammatical error. It should read "only supports synchronous creation" instead of "only support synchronous creation".Apply this diff to correct the message:
- `[@eggjs/core/singleton] ${this.name} only support synchronous creation, please use createInstanceAsync`); + `[@eggjs/core/singleton] ${this.name} only supports synchronous creation; please use createInstanceAsync`);test/singleton.test.ts (1)
27-28: Avoid using thedeleteoperator for performance reasonsUsing the
deleteoperator can negatively impact performance because it forces the engine to reconfigure the object's property map. It's recommended to set the property toundefinedinstead.Apply this diff to avoid the use of
delete:- delete (DataService as any).prototype.createInstance; - delete (DataService as any).prototype.createInstanceAsync; + (DataService as any).prototype.createInstance = undefined; + (DataService as any).prototype.createInstanceAsync = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 28-28: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/singleton.ts(1 hunks)test/singleton.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
test/singleton.test.ts
[error] 27-27: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 28-28: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Node.js / Test (windows-latest, 23)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18.19.0)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
- GitHub Check: Node.js / Test (macos-latest, 18.19.0)
🔇 Additional comments (1)
src/singleton.ts (1)
135-146: Avoid modifying__proto__and extending non-extensible objectsIn the
#extendDynamicMethods()method, modifying__proto__is discouraged due to performance and maintainability concerns. This can lead to hard-to-debug issues.Consider alternative approaches:
- Avoid modifying the prototype chain. Instead of using
client.__proto__, consider refactoring to avoid changing the object's prototype.- Wrap the client object. If the client is not extensible or is frozen, you might wrap it in another object that provides the additional methods.
- Access methods directly. Document how to access
createInstancemethods directly from the singleton without extending the client.
[skip ci] ## [6.3.0](v6.2.13...v6.3.0) (2025-01-20) ### Features * mv Singleton from egg ([#288](#288)) ([b5ebf68](b5ebf68))
eggjs/core#288 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit Based on the comprehensive summary, here are the release notes: - **New Features** - Added new CI workflows for different testing clusters and library testing - Enhanced continuous integration configuration - **Dependency Updates** - Updated `@eggjs/core` from version 6.2.13 to 6.3.0 - **Testing Improvements** - Added new test scripts for specific test clusters - Expanded error handling and logging test coverage - Restructured test file organization - **Code Refactoring** - Removed singleton implementation from core library - Updated type assertions and import paths - Simplified error handling in test cases - **Chores** - Updated GitHub Actions workflows - Reorganized test directory structure <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Singletonclass, validating various creation scenarios.