fix: mv single to @eggjs/core #5387
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 22 minutes and 20 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 (3)
WalkthroughThis pull request introduces significant changes to the Egg.js framework's testing infrastructure, workflow configurations, and singleton management. The modifications include updating GitHub Actions workflows, adding new CI scripts, removing the existing singleton implementation, and restructuring test files. The changes aim to enhance testing capabilities, improve code organization, and streamline the continuous integration process across different Node.js versions and environments. Changes
Possibly related issues
Possibly related PRs
Suggested labels
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 (
|
|
New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/egg@4.0.0 |
![coderabbitai[bot]](/~https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/index.test-d.ts (1)
171-171: Consider improving type inference instead of using double type assertion.The double type assertion
as unknown as Redissuggests a potential type inference issue with the singleton implementation. This could be improved to maintain better type safety.Consider investigating why the type inference isn't working correctly and fixing it at the source instead of using type assertions.
.github/workflows/nodejs-other.yml (1)
15-15: Standardize Node.js version selection.The version list mixes a specific version (18.19.0) with major versions (18, 20, 22). Consider either:
- Using only major versions for simpler maintenance
- Using specific versions for all entries if exact versions are required
- version: '18.19.0, 18, 20, 22' + version: '18, 20, 22'.github/workflows/nodejs-lib-plugins.yml (1)
1-18: Consider consolidating duplicate workflows.All five workflows are nearly identical, only differing in their test commands. Consider:
- Creating a single reusable workflow in this repository
- Parameterizing the test command
- Calling this workflow from simplified workflow files for each module
This would:
- Reduce duplication
- Simplify maintenance
- Ensure consistency
Example structure:
# .github/workflows/reusable-test.yml name: Reusable Test Workflow on: workflow_call: inputs: test_command: required: true type: string jobs: test: name: Node.js runs-on: ${{ matrix.os }} strategy: matrix: os: [ubuntu-latest, macos-latest, windows-latest] node: [18, 20, 22] steps: - uses: actions/checkout@v4 - name: Run Tests run: ${{ inputs.test_command }}Usage:
# .github/workflows/nodejs-lib-core.yml name: CI lib/core on: push: branches: [ master, next ] pull_request: branches: [ master, next ] jobs: test: uses: ./.github/workflows/reusable-test.yml with: test_command: npm run ci:lib/core secrets: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/nodejs-lib-cluster.yml(1 hunks).github/workflows/nodejs-lib-core.yml(1 hunks).github/workflows/nodejs-lib-other.yml(1 hunks).github/workflows/nodejs-lib-plugins.yml(1 hunks).github/workflows/nodejs-other.yml(1 hunks).github/workflows/nodejs.yml(2 hunks)package.json(2 hunks)src/index.ts(1 hunks)src/lib/core/singleton.ts(0 hunks)src/lib/egg.ts(0 hunks)test/app/extend/agent.test.ts(1 hunks)test/app/extend/application.test.ts(1 hunks)test/index.test-d.ts(1 hunks)test/lib/core/singleton.test.ts(0 hunks)test/typescript.test.ts(1 hunks)
💤 Files with no reviewable changes (3)
- src/lib/egg.ts
- test/lib/core/singleton.test.ts
- src/lib/core/singleton.ts
✅ Files skipped from review due to trivial changes (1)
- test/typescript.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (19)
- 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 (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (windows-latest, 18.19.0)
- GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
- GitHub Check: Node.js / Test (windows-latest, 18)
- 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, 22)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
- 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.19.0)
- GitHub Check: Node.js / Test (macos-latest, 18)
🔇 Additional comments (13)
test/app/extend/agent.test.ts (1)
32-34: LGTM! Clean error handling refactor.The change from try-catch to
assert.throws()is a good improvement that makes the test more concise and idiomatic. The updated error message is also more precise in describing the synchronous vs asynchronous creation distinction.Let's verify the consistency of this error message across the codebase:
✅ Verification successful
Error message is consistent across test files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar error messages to ensure consistency rg "support (sync|synchronous) creation" -g '!test/app/extend/agent.test.ts'Length of output: 210
src/index.ts (1)
28-32: Verify the impact of moving singleton exports to @eggjs/core.The singleton functionality has been moved from the local implementation to @eggjs/core package. This is a breaking change that could affect downstream dependencies.
✅ Verification successful
Moving singleton exports to @eggjs/core appears safe
The codebase analysis shows no direct usage of the Singleton class or types from the old path. The singleton functionality is primarily used through the
addSingletonAPI, which remains unchanged. The test fixtures demonstrate this pattern and don't depend on the internal implementation details.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for direct usage of the old singleton path rg -l "from ['\"].*?/lib/core/singleton\.js['\"]" # Check for any remaining references to the old singleton implementation rg "singleton\.js"Length of output: 76
Script:
#!/bin/bash # Check for any singleton-related files fd singleton # Search for Singleton type/class usage ast-grep --pattern 'import { Singleton } from' # Search for any singleton-related imports rg "import.*Singleton.*from" -A 1 # Check for Singleton class usage in code ast-grep --pattern 'class $_ extends Singleton'Length of output: 184
Script:
#!/bin/bash # Check contents of singleton demo test files find test/fixtures/apps/singleton-demo -type f -exec cat {} \; # Search for any remaining @eggjs/core imports in test files rg "from '@eggjs/core'" test/Length of output: 1615
test/app/extend/application.test.ts (2)
142-143: LGTM! Improved assertion readability.The change from
assert()toassert.equal()provides better error messages when assertions fail.Also applies to: 147-147, 151-151, 154-155, 163-163
157-159: LGTM! Better error handling for async singleton creation.The error message clearly indicates that synchronous creation is not supported and guides users to use the async method instead.
.github/workflows/nodejs.yml (1)
1-1: LGTM! Clear CI workflow configuration.The workflow is well configured with:
- Descriptive name that indicates its purpose
- Comprehensive test matrix across different Node.js versions (18.19.0, 18, 20, 22) and operating systems
- Specific test command for app-related tests
Also applies to: 16-16
.github/workflows/nodejs-lib-core.yml (1)
12-12: Same issues as in nodejs-other.yml.This workflow has the same issues regarding:
- Unpinned workflow reference using
@master- Inconsistent Node.js version specification
Also applies to: 15-15
.github/workflows/nodejs-lib-other.yml (1)
12-12: Same issues as in nodejs-other.yml.This workflow has the same issues regarding:
- Unpinned workflow reference using
@master- Inconsistent Node.js version specification
Also applies to: 15-15
.github/workflows/nodejs-lib-cluster.yml (1)
12-12: Same issues as in nodejs-other.yml.This workflow has the same issues regarding:
- Unpinned workflow reference using
@master- Inconsistent Node.js version specification
Also applies to: 15-15
.github/workflows/nodejs-lib-plugins.yml (2)
12-12: Same issues as in nodejs-other.yml.This workflow has the same issues regarding:
- Unpinned workflow reference using
@master- Inconsistent Node.js version specification
Also applies to: 15-15
1-1: Verify the completeness of the PR.The PR objectives mention "fix: mv single to @eggjs/core", but no code changes related to moving singleton functionality are visible in the provided files. Please ensure all necessary changes are included in the PR.
Run this script to check for singleton-related changes:
✅ Verification successful
PR changes are verified and complete ✅
The singleton functionality has been properly moved to @eggjs/core:
- Singleton types are now imported from @eggjs/core
- Old singleton definitions are only present in deprecated files (index-old.d.ts)
- No local singleton implementations remain in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for singleton-related files and changes echo "Searching for singleton-related files:" fd -t f -e ts -e js single echo -e "\nSearching for singleton-related code:" rg -l "singleton|single" --type ts --type js echo -e "\nChecking for @eggjs/core dependency:" rg "@eggjs/core" package.jsonLength of output: 889
Script:
#!/bin/bash # Check for singleton implementation and recent changes echo "Searching for singleton class or type definitions:" ast-grep --pattern 'class $_ { $$$ singleton($$$) { $$$ } $$$ }' echo -e "\nSearching for singleton type definitions:" rg "type.*Singleton|interface.*Singleton" -A 2 echo -e "\nChecking git diff for singleton-related changes:" git diff --name-status HEAD~1 | grep -i "single"Length of output: 748
package.json (3)
101-106: LGTM! Well-structured CI test organization.The new CI scripts provide a clear separation of test suites by module, which improves test organization and allows for more targeted test runs. This is particularly useful for:
- Faster feedback on specific module changes
- Easier debugging when tests fail
- Better parallel execution capabilities
97-97: LGTM! Minor formatting fix.Removed extra space in the preci script.
24-24: Verify @eggjs/core version compatibility.The update to @eggjs/core ^6.3.0 appears to be related to moving singleton functionality. Let's verify this version exists and check for any breaking changes.
✅ Verification successful
Version update to @eggjs/core ^6.3.0 is correct and necessary
The version exists on npm and was released on 2025-01-20. This update specifically adds the Singleton functionality being moved from egg, which aligns with the PR's objective. No breaking changes are reported.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify @eggjs/core version and check for breaking changes # Check if version exists on npm npm view @eggjs/core versions --json | jq 'contains(["6.3.0"])' # Check the changelog or release notes for breaking changes gh api repos/eggjs/core/releases | jq '.[] | select(.tag_name == "v6.3.0") | .body'Length of output: 557
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
test/agent.test.ts (3)
39-44: Avoid using fixed delays in testsUsing
setTimeoutwith a fixed delay can lead to flaky tests and slow down the test suite. Consider using event-based mechanisms, such as watching for file changes or polling the file until the expected content appears, to determine when the log file has been updated.
53-58: Replace fixed delay with reliable synchronizationAgain, using
setTimeoutmay cause the test to be unreliable or slower than necessary. It's better to implement a method that waits for the log file to contain the expected content before proceeding.
67-72: Eliminate fixed timeouts for improved test stabilityUsing fixed timeouts in tests can result in flaky behavior. Replace
setTimeoutwith a more robust approach to ensure the test only proceeds once the log file is ready..github/workflows/nodejs-lib.yml (1)
10-10: Use a descriptive and lowercase job IDThe job ID
Jobshould be lowercase and descriptive for clarity and consistency. Consider renaming it to something liketest_liborci_lib..github/workflows/nodejs-cluster1.yml (1)
10-10: Rename the job ID to be more descriptive and lowercaseFor better readability and adherence to best practices, change the job ID from
Jobto a lowercase, descriptive identifier liketest_cluster1orci_cluster1..github/workflows/nodejs-cluster2.yml (1)
3-7: Consider enabling branch protection rules.The workflow is correctly configured to run on push and pull requests to master and next branches. To ensure code quality, consider enabling branch protection rules that require this workflow to pass before merging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/nodejs-cluster1.yml(1 hunks).github/workflows/nodejs-cluster2.yml(1 hunks).github/workflows/nodejs-lib.yml(1 hunks).github/workflows/nodejs-other.yml(1 hunks)package.json(2 hunks)test/agent.test.ts(1 hunks)test/application.test.ts(1 hunks)test/cluster1/app_worker.test.ts(2 hunks)test/cluster1/cluster-client-error.test.ts(1 hunks)test/cluster1/cluster-client.test.ts(1 hunks)test/cluster2/master.test.ts(1 hunks)test/egg.test.ts(1 hunks)test/lib/agent.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- test/lib/agent.test.ts
✅ Files skipped from review due to trivial changes (5)
- test/egg.test.ts
- test/cluster1/app_worker.test.ts
- test/cluster1/cluster-client.test.ts
- test/cluster1/cluster-client-error.test.ts
- test/cluster2/master.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/nodejs-other.yml
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (macos-latest, 18)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (ubuntu-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 (ubuntu-latest, 22)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (windows-latest, 22)
- 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 (ubuntu-latest, 22)
🔇 Additional comments (5)
test/application.test.ts (3)
10-10: LGTM! Test suite description updated correctly.The test suite description has been updated to match the new file location, maintaining consistency between the test description and its actual path.
Line range hint
1-324: LGTM! Test coverage remains comprehensive.The test file maintains thorough coverage of critical application features, including:
- Application lifecycle
- Error handling
- Configuration management
- HTTP functionality
- Event handling
All test cases remain intact and maintain backward compatibility after the directory restructure.
7-8: Verify import paths after directory restructure.The import paths have been updated to reflect the new directory structure. Let's verify that these paths are correct.
✅ Verification successful
Import paths are correctly structured and valid
The verification confirms that:
Applicationis properly exported fromsrc/index.jsMockApplicationand other test utilities are available intest/utils.js- All imported symbols exist in their respective source files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the imported files exist at their new locations # Check if the source file exists fd -t f "index.js" src/ # Check if utils.js exists in the test directory fd -t f "utils.js" test/ # Verify the imports are used correctly ast-grep --pattern 'import { $_ } from "../src/index.js"' ast-grep --pattern 'import { $_ } from "./utils.js"'Length of output: 163
Script:
#!/bin/bash # Check if files exist and show their content echo "=== Checking index.js ===" rg --type ts --files-with-matches "export.*Application" src/ rg --type ts "export.*Application" src/ echo -e "\n=== Checking utils.js ===" rg --type ts --files-with-matches "export.*MockApplication" test/ rg --type ts "export.*MockApplication" test/ echo -e "\n=== Checking actual imports in test file ===" rg --type ts -A 2 "^import.*index\.js" test/application.test.ts rg --type ts -A 2 "^import.*utils\.js" test/application.test.tsLength of output: 2521
.github/workflows/nodejs-cluster2.yml (2)
14-16: Great job on the comprehensive test matrix!The configuration ensures thorough testing across:
- Multiple operating systems (Ubuntu and Windows)
- Multiple Node.js versions (18, 20, 22)
16-16: Verify the npm script exists.Ensure that
ci:cluster2script is properly configured in package.json.✅ Verification successful
The
ci:cluster2script is properly configured🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the ci:cluster2 script exists in package.json jq '.scripts["ci:cluster2"]' package.jsonLength of output: 87
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/cluster2/master.test.ts (2)
19-24: Handle exceptions explicitly instead of suppressing themThe
try...catchblocks in these tests suppress exceptions without any handling or logging. Silently swallowing exceptions can make debugging difficult.
- Suggestion: Add comments explaining why exceptions are ignored or handle them appropriately.
Apply this diff to add comments explaining the rationale:
try { await app.httpRequest() .get('/exit'); } catch (_) { - // do nothing + // Exception is expected here as the worker process exits intentionally. }Repeat similar changes for the other
try...catchblocks.Also applies to: 37-42, 63-68, 78-83
27-28: Optimize test wait times to improve efficiencyFixed delays of 20 seconds and 5 seconds can significantly slow down the test suite.
- Suggestion: Implement event-based synchronization or polling with shorter intervals to detect worker restarts more efficiently.
Example approach:
- await scheduler.wait(20000); + // Wait until the new worker process has started + await app.ready();Ensure that the test waits only as long as necessary for the worker to restart.
Also applies to: 45-46, 71-72, 86-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/nodejs-cluster1.yml(1 hunks).github/workflows/nodejs-cluster2.yml(1 hunks).github/workflows/nodejs-lib.yml(1 hunks).github/workflows/nodejs.yml(2 hunks)package.json(2 hunks)test/cluster1/master.test.ts(1 hunks)test/cluster2/master.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/nodejs-lib.yml
- .github/workflows/nodejs-cluster1.yml
- .github/workflows/nodejs-cluster2.yml
- package.json
- .github/workflows/nodejs.yml
⏰ Context from checks skipped due to timeout of 90000ms (19)
- 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 (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (macos-latest, 22)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (macos-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (macos-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Deploy-Preview
🔇 Additional comments (3)
test/cluster1/master.test.ts (2)
3-3: Import path updated to reflect new directory structureThe import path for
MockApplication,cluster, andgetFilepathhas been correctly updated to match the new directory structure.
5-5: Test suite description updated to match file locationThe test suite description now accurately reflects the file's new location, ensuring clarity in test reports.
test/cluster2/master.test.ts (1)
1-1: Verify compatibility of 'node:timers/promises' across Node.js versionsThe
schedulermodule from'node:timers/promises'is used to manage timing. Please ensure that this module is supported in all Node.js versions targeted by the project to prevent potential runtime issues.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #5387 +/- ##
==========================================
- Coverage 95.15% 94.93% -0.22%
==========================================
Files 41 40 -1
Lines 3903 3734 -169
Branches 433 354 -79
==========================================
- Hits 3714 3545 -169
Misses 189 189 ☔ View full report in Codecov by Sentry. |
[skip ci] ## [4.0.3](v4.0.2...v4.0.3) (2025-01-21) ### Bug Fixes * mv single to @eggjs/core ([#5387](#5387)) ([b223c7a](b223c7a))
eggjs/core#288
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
New Features
Dependency Updates
@eggjs/corefrom version 6.2.13 to 6.3.0Testing Improvements
Code Refactoring
Chores