fix: use @eggjs/onerror #5389
fix: use @eggjs/onerror #5389
Conversation
WalkthroughThis pull request updates various aspects of the Egg framework. It removes a deprecated import from type definitions and revises package dependencies and documentation references to align with the updated naming (e.g., changing Changes
Sequence Diagram(s)sequenceDiagram
participant Leader
participant Follower
participant RemoteServer
Note over Leader, Follower: Leader/Follower Communication Model
Leader->>RemoteServer: Establish socket connection
Follower->>Leader: Send operation request
Leader->>RemoteServer: Process request
RemoteServer-->>Leader: Return response
Leader-->>Follower: Forward response
sequenceDiagram
participant Agent
participant Boot
Agent->>Boot: Instantiate Boot class
Boot->>Boot: didLoad() - initialize clients
Boot->>Boot: willReady() - verify readiness
Boot-->>Agent: Signal boot completion
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (20)
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #5389 +/- ##
=======================================
Coverage 94.94% 94.94%
=======================================
Files 40 40
Lines 3739 3741 +2
Branches 354 354
=======================================
+ Hits 3550 3552 +2
Misses 189 189 ☔ View full report in Codecov by Sentry. |
|
New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected] |
![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
🔭 Outside diff range comments (2)
site/docs/advanced/cluster-client.md (2)
437-437: Syntax Error in APIClient Instantiation Example
There is a missing closing parenthesis in the example code snippet for instantiatingAPIClient(line 437). This can confuse readers copying the sample code.Suggested fix:
- app.apiClient = new APIClient(Object.assign({}, config, { cluster: app.cluster }); + app.apiClient = new APIClient(Object.assign({}, config, { cluster: app.cluster }));
320-321: Variable Mismatch in MockClient’ssubMethod
Within thesub(info, listener)method, the code incorrectly referencesreg.dataIdinstead of using the provided parameter nameinfo.Suggested fix:
- const key = reg.dataId; + const key = info.dataId;
🧹 Nitpick comments (5)
test/fixtures/apps/cluster_mod_app/agent.js (2)
10-19: Consider adding error handling for client initialization.If
agent.cluster(...)or any client constructor fails, the code will not propagate exceptions gracefully. Wrapping these calls in a try/catch block can provide more visibility and prevent unhandled rejections.
21-26: Optimize readiness with concurrency.Currently, readiness checks run sequentially. You can shorten startup time by awaiting them in parallel, for example:
- await agent.registryClient.ready(); - await agent.apiClient.ready(); - await agent.apiClient2.ready(); + await Promise.all([ + agent.registryClient.ready(), + agent.apiClient.ready(), + agent.apiClient2.ready(), + ]);test/fixtures/apps/cluster_mod_app/lib/registry_client.js (2)
1-2: Check for Node.js version compatibility.Using
url.parse()is fine for older Node.js versions. However, for modern Node.js (v10+), theURLconstructor is generally recommended. Confirm that you need backward compatibility before favoringurl.parse()over theURLclass.
46-46: Consider the modern URL constructor for clarity and maintainability.Replacing
parse(url, true)withnew URL(url)can improve code readability and align with current Node.js best practices. For instance:- this.emit(key, this._registered.get(key).map(url => parse(url, true))); + this.emit(key, this._registered.get(key).map(urlString => new URL(urlString)));site/docs/advanced/cluster-client.zh-CN.md (1)
556-557: Final APIClient Export Formatting
Minor formatting adjustments and a trailing newline change have been observed at the end of the file. No functional impact is detected; however, ensure adherence to the project’s code style guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
index-old.d.ts(0 hunks)package.json(2 hunks)site/docs/advanced/cluster-client.md(3 hunks)site/docs/advanced/cluster-client.zh-CN.md(4 hunks)site/docs/basics/plugin.md(1 hunks)site/docs/basics/plugin.zh-CN.md(1 hunks)site/docs/core/cookie-and-session.md(2 hunks)site/docs/core/cookie-and-session.zh-CN.md(1 hunks)site/docs/core/error-handling.md(1 hunks)site/docs/core/error-handling.zh-CN.md(1 hunks)site/docs/tutorials/socketio.md(1 hunks)site/docs/tutorials/socketio.zh-CN.md(1 hunks)src/config/plugin.ts(1 hunks)src/lib/egg.ts(1 hunks)src/lib/types.ts(1 hunks)test/cluster1/cluster-client.test.ts(3 hunks)test/fixtures/apps/cluster_mod_app/agent.js(1 hunks)test/fixtures/apps/cluster_mod_app/lib/registry_client.js(2 hunks)test/index.test-d.ts(3 hunks)test/lib/core/loader/load_plugin.test.ts(1 hunks)test/lib/core/messenger/local.test.ts(4 hunks)test/utils.ts(3 hunks)
💤 Files with no reviewable changes (1)
- index-old.d.ts
✅ Files skipped from review due to trivial changes (4)
- site/docs/basics/plugin.zh-CN.md
- site/docs/basics/plugin.md
- site/docs/core/error-handling.md
- site/docs/tutorials/socketio.md
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (macos-latest, 20)
- GitHub Check: Node.js / Test (macos-latest, 22)
- GitHub Check: Node.js / Test (macos-latest, 18)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
🔇 Additional comments (32)
test/fixtures/apps/cluster_mod_app/agent.js (1)
5-8: Clean class-based approach.Exporting a class constructor is a clear and maintainable approach. Storing the
agentinstance inthis.agentis straightforward and sets a good foundation for lifecycle methods.src/config/plugin.ts (1)
9-9: Verify the new package installation for @eggjs/onerror.Great switch to
@eggjs/onerror. Ensure the dependency is declared in your project and that you aren’t missing any required migrations. You can run this script to verify its presence and version:✅ Verification successful
@eggjs/onerror dependency verification succeeded
- The repository’s root package.json declares
"@eggjs/onerror": "^3.0.0".- No references to the old
egg-onerrorpackage were found in any package.json.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that @eggjs/onerror is listed in package.json with a valid version rg '@eggjs/onerror' package.json # Optionally, check for any references to the old egg-onerror package rg 'egg-onerror' package.jsonLength of output: 168
Script:
#!/bin/bash echo "Searching for package.json files in the repository..." fd package.json echo "" echo "Verifying @eggjs/onerror dependency in discovered package.json files:" for file in $(fd package.json); do echo "----- Checking $file -----" rg '@eggjs/onerror' "$file" echo "" echo "----- Checking for any mentions of egg-onerror in $file -----" rg 'egg-onerror' "$file" echo "" doneLength of output: 96173
test/cluster1/cluster-client.test.ts (3)
2-2: LGTM! Using node:timers/promises for better async handling.The import of
schedulerfromnode:timers/promisesis a good choice for handling delays in tests, replacing the callback-basedsetTimeout.
19-19: LGTM! Using assert.equal for better error messages.The change to use
assert.equalinstead of direct comparison improves error messages when assertions fail.Also applies to: 59-59
23-34: LGTM! Improved test readability with async/await.The refactoring to use async/await syntax makes the test cases more readable and maintainable by:
- Eliminating promise chaining
- Making the flow of asynchronous operations clearer
- Using
scheduler.wait(500)for delaysAlso applies to: 63-74, 76-81, 83-88
test/utils.ts (2)
16-16: LGTM! Enhanced type safety for single process mode.The addition of
SingleModeApplicationinterface properly types theagentproperty, improving type safety and developer experience.Also applies to: 28-30
62-62: LGTM! Improved return type accuracy.The return type update to
Promise<SingleModeApplication>ensures type consistency and better reflects the actual return value.Also applies to: 70-70
test/index.test-d.ts (3)
13-13: LGTM! Added type checks for messenger interface.The addition of IMessenger type checks ensures the messenger implementation maintains its contract.
Also applies to: 16-17
65-67: LGTM! Added type checks for onerror config.The type checks for onerror config properties ensure type safety for error handling configuration.
113-114: LGTM! Improved AppBoot instantiation clarity.Using a separate variable
app1for AppBoot instantiation improves code clarity and type safety.test/lib/core/messenger/local.test.ts (3)
4-4: LGTM! Enhanced type safety with SingleModeApplication.The update to use
SingleModeApplicationtype improves type safety and consistency.Also applies to: 7-7
147-148: LGTM! Maintained backward compatibility.The type casting and comments explain the compatibility handling for process.pid usage.
Also applies to: 151-151
205-205: LGTM! Appropriate type casting for onMessage.The type casting for onMessage call maintains compatibility while improving type safety.
src/lib/types.ts (1)
28-28: LGTM! Import added for @eggjs/onerror plugin types.The import is correctly placed alongside other plugin type imports.
test/lib/core/loader/load_plugin.test.ts (1)
57-59: LGTM! Test assertions updated for @eggjs/onerror.The test correctly verifies:
- The new ESM distribution path
- The updated package name
src/lib/egg.ts (1)
699-699: Verify the relationship with onerror migration.While the addition of the
messengerproperty is technically correct, it seems unrelated to the PR's objective of migrating to@eggjs/onerror. Please clarify if this change is necessary for the migration.site/docs/core/error-handling.zh-CN.md (1)
62-62: LGTM! Documentation updated to reference @eggjs/onerror.The documentation correctly reflects the new package name while maintaining the link to the GitHub repository.
package.json (2)
21-53: Dependency Updates in the Dependencies Block
The dependency versions have been updated across many packages (e.g.@eggjs/cluster,@eggjs/cookies,@eggjs/core, etc.), and the new dependency@eggjs/onerrorhas been added. Please ensure all these bump‐versions have been verified for compatibility with your framework’s internals and integration tests.
54-87: Updated DevDependencies
The devDependencies have been updated (e.g.@arethetypeswrong/cli,@eggjs/bin,mm,prettier,sdk-base, etc.) to reflect the latest versions. Double-check that these changes work seamlessly with your build and test workflow.site/docs/core/cookie-and-session.zh-CN.md (3)
186-188: Updated Redis Session Plugin Description
The descriptive text now references the updated plugin packages—@eggjs/redisand@eggjs/session-redis—which aligns with the new naming conventions. This clarification will help users correctly configure external session storage.
189-197: Plugin Configuration Block Update
The code block defining the plugin configuration has been updated to use the new package names. Verify that the installation scripts and runtime configurations are consistent with these changes.
286-287: Updated Reference Links
The markdown reference links for@eggjs/redisand@eggjs/session-redishave been updated to point directly to their GitHub repositories. This improves clarity and ensures users follow the correct documentation paths.site/docs/core/cookie-and-session.md (1)
1-120: No Significant Content Changes Detected
The English documentation appears to be consistent with the updated dependency naming conventions. It is recommended to verify that all package references in the text match the current configuration inpackage.jsonand related files.🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ...rmission of Cookie. -{Number} maxAge: set the lifetime of the cookie in milli...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...r specified lifetime. -{Date} expires: set the expiration time of the cookie. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...he time client closed. -{String} path: set the path of the cookie. By default ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~45-~45: Did you mean: “By default,”?
Context: ...ing} path: set the path of the cookie. By default it's on root path (/`), which means al...(BY_DEFAULT_COMMA)
[uncategorized] ~46-~46: Loose punctuation mark.
Context: ...ccess to the cookie. -{String} domain: set the domain of the cookie. By defaul...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~47-~47: Loose punctuation mark.
Context: ...ss to the cookie. -{Boolean} httpOnly: set whether the cookie can be accessed ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~47-~47: Did you mean: “By default,”?
Context: ...e cookie can be accessed by Javascript. By default it'strue, which means Javascript can...(BY_DEFAULT_COMMA)
[uncategorized] ~48-~48: Loose punctuation mark.
Context: ... access the cookie. -{Boolean} secure: set whether the cookie can only be acce...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~48-~48: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...e-secure-flag-work) for details. Egg.js auto sets this value to true if the current reque...(AUTO_HYPHEN)
[uncategorized] ~52-~52: Loose punctuation mark.
Context: ...ore parameters: -{Boolean} overwrite: set the way of handling same Cookie key...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~53-~53: Loose punctuation mark.
Context: ... with the same key. -{Boolean} signed: set whether the cookie should be signed...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~53-~53: Did you mean: “By default,”?
Context: ...event cookie values modified by client. By default it's true. -{Boolean} encrypt: set w...(BY_DEFAULT_COMMA)
[uncategorized] ~54-~54: Loose punctuation mark.
Context: ...default it's true. -{Boolean} encrypt: set whether the cookie should be encryp...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~54-~54: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...l be encrypted before sending to clients so user clients cannot get raw text of the...(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~54-~54: Did you mean: “By default,”?
Context: ...ents cannot get raw text of the cookie. By default it's false. When using Cookie, we need...(BY_DEFAULT_COMMA)
[typographical] ~57-~57: Except for inverted sentences, ‘can it’ requires a question mark at the end of the sentence.
Context: ...ssed by JS, can it be modified by client. **By default, Cookie is signed but not...(MD_PRP_QUESTION_MARK)
[style] ~62-~62: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...annot modify it (manually).** - If you need to allow JS to access and modify Cookie: ...(REP_NEED_TO_VB)
[style] ~93-~93: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e could have been modified by client. - Ifencryptis true whensetCookie but...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~94-~94: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...d text rather than the raw plain text. If you want to get the cookie set by front...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~120-~120: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...g.js iterates through all keys. If you need to update Cookie secret key and don't want...(REP_NEED_TO_VB)
site/docs/tutorials/socketio.zh-CN.md (1)
86-88: Clarification on Redis Configuration Separation
The note now explicitly states that when using@eggjs/redisin a project, separate configurations must be provided and they cannot be shared. This added clarity helps prevent misconfiguration in clustered environments.site/docs/advanced/cluster-client.zh-CN.md (5)
8-19: Added Diagram for Multi-Process Communication
A new diagram (in a bash code block) illustrates how long connections are managed between clients and servers in a multi-process architecture. This visual aid provides a clear depiction of the Leader/Follower setup.
72-73: Refactored Import in Client Example
The client example now importsBasefromsdk-baseusing destructuring (i.e.const { Base } = require('sdk-base');). This modernizes the code style and improves clarity regarding the required module’s interface.
168-169: Enhanced Module Import for URL Parsing
The RegistryClient example now uses the built-in Node.js URL parser via destructuring (i.e.const { parse } = require('node:url');). This is a best practice that improves readability and leverages current Node.js capabilities.
235-237: Improved Publish Method with URL Parsing
Within thepublishmethod, mapping over the registered URLs now uses theparsefunction to ensure correct URL formatting. This change reinforces type safety for the published data.
456-484: APIClient Base Implementation Update
TheAPIClientnow extends fromAPIClientBasewith clearly defined getters forDataClientandclusterOptions. This refactoring enforces a cleaner separation between asynchronous remote calls and local caching, making the API more robust in multi-process scenarios.site/docs/advanced/cluster-client.md (3)
73-73: Destructured Import for Base Module
The change toconst { Base } = require('sdk-base');follows modern import conventions and improves clarity.
171-172: Updated Destructured Imports in RegistryClient Example
Switching toconst { parse } = require('node:url');andconst { Base } = require('sdk-base');aligns with current Node.js best practices.
239-239: Correct Usage of URL Parsing in Publish Method
Usingthis._registered.get(key).map((url) => parse(url, true))is concise and leverages the updated import ofparse. Ensure that passingtruemeets the parsing needs by returning a URL object.
[skip ci] ## [4.0.5](v4.0.4...v4.0.5) (2025-02-03) ### Bug Fixes * use @eggjs/onerror ([#5389](#5389)) ([762e301](762e301))
Summary by CodeRabbit