fix: use @eggjs/logrotator #5390
Conversation
WalkthroughThis pull request removes a deprecated GitHub Actions workflow and an unused module import while updating the logging implementation. The dependency for log rotation has been shifted from Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant ML as MyLogRotator
participant LR as LogRotator (base)
Test->>ML: Instantiate MyLogRotator
Test->>ML: Invoke getRotateFiles()
ML-->>Test: Return empty Map
Poem
✨ Finishing Touches
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 #5390 +/- ##
=======================================
Coverage 94.94% 94.95%
=======================================
Files 40 40
Lines 3741 3746 +5
Branches 354 353 -1
=======================================
+ Hits 3552 3557 +5
Misses 189 189 ☔ 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: 0
🧹 Nitpick comments (1)
test/lib/plugins/logrotator.test.ts (1)
22-22: Remove debug console.log statement.This console.log statement appears to be for debugging purposes and should be removed before merging.
- console.log(files);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/pr-contributor-welcome.yml(0 hunks)index-old.d.ts(0 hunks)package.json(1 hunks)site/docs/basics/plugin.md(1 hunks)site/docs/basics/plugin.zh-CN.md(1 hunks)site/docs/core/logger.md(2 hunks)site/docs/core/logger.zh-CN.md(2 hunks)src/config/plugin.ts(1 hunks)src/lib/types.ts(1 hunks)test/index.test-d.ts(2 hunks)test/lib/plugins/logrotator.test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- index-old.d.ts
- .github/workflows/pr-contributor-welcome.yml
✅ Files skipped from review due to trivial changes (4)
- site/docs/basics/plugin.zh-CN.md
- site/docs/basics/plugin.md
- site/docs/core/logger.md
- site/docs/core/logger.zh-CN.md
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (macos-latest, 22)
- GitHub Check: Node.js / Test (macos-latest, 20)
- GitHub Check: Node.js / Test (macos-latest, 18)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (windows-latest, 20)
- 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 (5)
test/lib/plugins/logrotator.test.ts (1)
17-17: LGTM!The import path has been correctly updated to use the new
@eggjs/logrotatorpackage.src/config/plugin.ts (1)
86-86: LGTM!The package name has been correctly updated to use the new
@eggjs/logrotatorpackage, maintaining consistency with other@eggjspackages in the configuration.test/index.test-d.ts (1)
3-3: LGTM!The changes enhance type safety by:
- Adding proper import for the LogRotator class
- Adding a test class extending LogRotator
- Adding comprehensive type expectations for logrotator configuration properties
Also applies to: 73-86
src/lib/types.ts (1)
29-29: LGTM!The import for
@eggjs/logrotatorhas been correctly added, following the same pattern as other plugin imports.package.json (1)
28-28: Updated Logrotator DependencyThe dependency for log rotation has been updated to use
@eggjs/logrotatorwith version^4.0.0, replacing the deprecatedegg-logrotatorpackage. This change aligns with the updated logging implementation and ensures the project uses the supported package going forward.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/egg.ts (1)
132-137: Consider enhancing the JSDoc for the Boot property.The documentation could be more explicit about
Bootbeing an alias ofBaseHookClass. Consider adding more context about when to useBootvsBaseHookClass./** * Retrieve base boot - * @member {Boot} - * @alias BaseHookClass + * @member {Boot} Boot - An alias of BaseHookClass, used for boot lifecycle hooks + * @see BaseHookClass */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/egg.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (windows-latest, 20)
- 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, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (macos-latest, 22)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (macos-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-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, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
🔇 Additional comments (3)
src/lib/egg.ts (3)
119-124: LGTM! Well-documented subscription property.The
Subscriptionproperty is well-documented with version information and clear purpose.Also applies to: 701-701
126-130: LGTM! Clear base hook class implementation.The
BaseHookClassproperty is properly implemented and documented.Also applies to: 702-702
686-704: LGTM! Interface declaration is consistent.The interface declaration in the module augmentation correctly mirrors the class properties.
[skip ci] ## [4.0.6](v4.0.5...v4.0.6) (2025-02-03) ### Bug Fixes * use @eggjs/logrotator ([#5390](#5390)) ([351a022](351a022))
Summary by CodeRabbit