feat: upgrade to egg v4 #126
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257
WalkthroughRepository examples are migrated to newer EggJS beta versions with Node.js 20+ requirement, CI workflows updated to matrix-based testing, and projects converted from CommonJS to ES modules with TypeScript enhancements. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Rationale: Broad scope affecting multiple example directories with repetitive patterns (version updates, import migrations, ESM conversion). Changes are largely consistent and mechanical across projects—dependency updates, test framework migrations to @eggjs/mock, and TypeScript additions. However, logic density is low and patterns repeat, reducing per-file review complexity. Some heterogeneity in foo controller behavior change (status 400 vs. 200) and configuration factory pattern introduction warrants focused attention. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
![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 (4)
hello-tegg/app/middleware/trace_method.ts (1)
5-5: Consider using a more specific type instead ofany.Using
(ctx.body as any)bypasses TypeScript's type checking. While this works, it loses the benefits of type safety. If possible, consider defining or importing the appropriate interface forctx.bodystructure.- (ctx.body as any).data.message += ` (${ctx.method})`; + interface ResponseBody { + data: { + message: string; + }; + } + (ctx.body as ResponseBody).data.message += ` (${ctx.method})`;Alternatively, if this is a common pattern in the codebase, consider extending the EggContext type to include the body property with the right structure.
bin/test.sh (1)
8-9: Consider making the registry configurable.The script hardcodes a specific npm registry (npmmirror.com), which might be region-specific. Consider making this configurable through an environment variable to improve flexibility.
- npm install --registry=https://registry.npmmirror.com + NPM_REGISTRY=${NPM_REGISTRY:-"https://registry.npmmirror.com"} + npm install --registry=$NPM_REGISTRYREADME.md (1)
22-22: Consider using proper heading syntaxThe linter flagged line 22 for using emphasis instead of a heading format. Consider changing:
- **Recommend to use Node >= 18** + ## Recommend to use Node >= 18This maintains consistent document structure and improves accessibility.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
22-22: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
helloworld/test/index.test.ts (1)
11-18: Good practice using async/awaitThe second test correctly uses async/await pattern for testing the /foo endpoint. Consider applying the same pattern to the first test for consistency.
- it('should GET / 200', () => { - return app.httpRequest() + it('should GET / 200', async () => { + await app.httpRequest()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.github/PULL_REQUEST_TEMPLATE.md(0 hunks).github/workflows/ci.yml(1 hunks)README.md(2 hunks)bin/test.sh(1 hunks)hello-tegg/.gitignore(1 hunks)hello-tegg/app/middleware/trace_method.ts(1 hunks)hello-tegg/config/config.default.ts(1 hunks)hello-tegg/config/plugin.ts(1 hunks)hello-tegg/package.json(1 hunks)hello-tegg/test/biz/HelloService.test.ts(1 hunks)hello-tegg/test/controller/HelloController.test.ts(1 hunks)hello-tegg/tsconfig.json(1 hunks)helloworld/.eslintrc(1 hunks)helloworld/.gitignore(1 hunks)helloworld/app/controller/foo.js(0 hunks)helloworld/app/controller/foo.ts(1 hunks)helloworld/app/controller/home.js(0 hunks)helloworld/app/controller/home.ts(1 hunks)helloworld/app/router.ts(1 hunks)helloworld/config/config.default.js(0 hunks)helloworld/config/config.default.ts(1 hunks)helloworld/package.json(1 hunks)helloworld/test/index.test.js(0 hunks)helloworld/test/index.test.ts(1 hunks)helloworld/tsconfig.json(1 hunks)package.json(2 hunks)
💤 Files with no reviewable changes (5)
- helloworld/app/controller/home.js
- helloworld/app/controller/foo.js
- .github/PULL_REQUEST_TEMPLATE.md
- helloworld/test/index.test.js
- helloworld/config/config.default.js
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
22-22: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
🪛 Shellcheck (0.10.0)
bin/test.sh
[warning] 5-5: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🔇 Additional comments (29)
hello-tegg/.gitignore (1)
22-22: Good addition to .gitignoreAdding
.egg/to the ignore list is appropriate as part of the upgrade to Egg v4. This directory typically contains framework-generated files that shouldn't be tracked in version control.helloworld/app/controller/home.ts (1)
1-8: Clean TypeScript implementationGood job migrating this controller from JavaScript to TypeScript. The implementation follows best practices with proper class extension, async/await usage, and clean syntax.
helloworld/config/config.default.ts (1)
1-5: Well-typed configuration fileGood implementation of the TypeScript configuration file with proper type imports and assertions. The use of the
EggAppConfigtype ensures type safety for your configuration.helloworld/app/controller/foo.ts (1)
1-11: Clean controller implementation with question about status codeThe TypeScript implementation of this controller looks good with proper class extension and async/await usage.
One observation: the controller always returns a 400 (Bad Request) status code without any conditional validation. Is this intentional behavior, or should there be some error condition checking?
helloworld/.gitignore (1)
1-2:Details
✅ Verification successful
Verify the impact of ignoring all JavaScript files.
The
.gitignorefile is set to ignore all JavaScript files (*.js), which suggests this project is transitioning to TypeScript where JS files are generated from TS sources. This is generally appropriate when working with TypeScript, but could have unintended consequences if any source JavaScript files still exist.
🏁 Script executed:
#!/bin/bash # Check if there are any source JavaScript files that might be accidentally ignored echo "Checking for JavaScript source files in the helloworld directory..." find helloworld -type f -name "*.js" | grep -v "node_modules" || echo "No JavaScript source files found that would be ignored" # Check for presence of TypeScript files to confirm TypeScript usage echo "Verifying TypeScript files exist..." find helloworld -type f -name "*.ts" || echo "No TypeScript files found - the *.js ignore pattern might be unnecessary"Length of output: 603
JavaScript Ignore Pattern Verified
The verification confirms that there are no JavaScript source files in the repository and that multiple TypeScript files are present. This indicates that the*.jsignore pattern is appropriate for this project. No action is required unless source JavaScript files are introduced in the future.helloworld/.eslintrc (1)
1-6: LGTM! TypeScript linting configuration looks appropriate.The ESLint configuration properly extends the recommended TypeScript rules for the Egg framework, which aligns with the project's upgrade to egg v4 and transition to TypeScript.
helloworld/app/router.ts (1)
1-3: Clean migration to TypeScript and ES modulesThe transition from CommonJS to ES modules with proper TypeScript typing is well-implemented. This change aligns with modern JavaScript practices and the egg v4 framework upgrade.
hello-tegg/config/plugin.ts (1)
1-4: Verify type handling for plugin configurationThe removal of the
EggPluginimport and type annotation appears to be part of adapting to egg v4's typing system. Consider:
- If egg v4 provides type inference for plugins automatically, this change is appropriate
- If type safety is still needed, consider using the framework's updated type definitions
Can you verify that this approach maintains type safety under egg v4?
README.md (1)
3-10: Documentation correctly updated for egg v4The README updates properly reflect the new requirements:
- Updated CI badge to GitHub Actions
- Specified Node.js >= 18 requirement
- Updated Egg version to >= 4
These changes align well with the PR objective and clearly communicate the breaking change regarding Node.js version requirements.
Also applies to: 22-22
helloworld/tsconfig.json (1)
1-14: Well-configured TypeScript setupThis TypeScript configuration is appropriately set up for the egg v4 upgrade:
- Extends the base egg TypeScript configuration
- Enables strict type checking for better code quality
- Uses ES2022 target which is compatible with Node.js 18+
- Configures modern module resolution with NodeNext
- Properly excludes test files from compilation
These settings provide a solid foundation for TypeScript development with egg v4.
hello-tegg/test/biz/HelloService.test.ts (1)
1-3: Updated imports align with egg v4 migrationThe import changes properly reflect the migration to egg v4, switching from 'egg-mock/bootstrap' to '@eggjs/mock/bootstrap' and using Node.js built-in modules with the 'node:' prefix.
helloworld/test/index.test.ts (2)
1-2: Import from correct egg v4 packageThe import correctly uses '@eggjs/mock/bootstrap' which is consistent with the egg v4 migration.
3-9: Well-structured test for root endpointThis test properly verifies that the application root endpoint returns a 200 status code with the expected "Hello World" response.
.github/workflows/ci.yml (1)
12-15: Updated CI configuration for Node.js requirementsThe workflow changes correctly:
- Update the repository source to 'node-modules/github-actions'
- Update Node.js versions to '18, 20, 22', aligning with the PR objective to drop support for Node.js versions earlier than 18.19.0
This ensures that the CI pipeline will test against the supported Node.js versions only.
hello-tegg/config/config.default.ts (2)
1-2: Simplified import statementThe import statement has been simplified to only include what's needed after the egg v4 migration.
5-5:Details
❓ Verification inconclusive
Updated type assertion for configuration
The type assertion has been changed from
PowerPartial<EggAppConfig>toEggAppConfig, suggesting a change in how configurations are handled in egg v4.Verify that this change maintains type safety for optional configuration properties.
🏁 Script executed:
#!/bin/bash # Check for the definition of EggAppConfig in the egg package rg --type-add 'ts:*.{ts,d.ts}' --type ts -A 10 "export (type|interface) EggAppConfig" ./node_modules/eggLength of output: 165
Action Required: Verify EggAppConfig Optional Properties
The initial verification script targeted the
./node_modules/eggdirectory but returned an error (directory not found). This prevents us from automatically confirming that the updated type assertion—fromPowerPartial<EggAppConfig>toEggAppConfig—preserves the optional properties’ type safety.To investigate further, please run the following shell script to search for the definition of
EggAppConfigwithin the repository (or its custom type declarations):#!/bin/bash # Search for the definition of interface EggAppConfig in the repository (including .d.ts and .ts files) rg --type-add 'ts:*.{ts,d.ts}' --type ts -A 20 "interface EggAppConfig" .Once you locate the definition, manually verify that any configuration properties intended to be optional are marked as optional (using “?” in the type declaration) so that replacing
PowerPartial<EggAppConfig>withEggAppConfigdoes not enforce unexpected required fields.Please confirm that this change maintains type safety for optional configuration properties.
hello-tegg/tsconfig.json (2)
4-10: Enhanced TypeScript configuration options.
The updated compiler options (setting"baseUrl": ".","strict": true,"noImplicitAny": true,"target": "ES2022","module": "NodeNext","moduleResolution": "NodeNext", and"declaration": false) enforce stricter type checking and modern ECMAScript module resolution. These settings improve both code quality and future maintainability.
12-14: Exclude the test directory from compilation.
Adding the"exclude": ["test"]directive ensures that test files are not compiled by TypeScript, which can speed up the build process and avoid potential type conflicts.package.json (3)
13-13: Bump dependency version for npminstall.
Upgrading"npminstall"from a caret version to a fixed version"7"ensures consistency in dependency resolution. Please verify that version 7 is compatible with your overall dependency tree.
19-21: Streamline test and CI scripts.
Changing the test script to run./bin/test.shand simplifying the CI command to"npm test"helps standardize the testing workflow. Make sure that thetest.shscript in thebin/directory is complete and correctly orchestrates the tests.
33-33: Update Node.js engine requirement.
The engine requirement is now"node": ">= 18.19.0", which reflects the breaking change and aligns with Egg v4’s minimum requirement. Ensure that this change is clearly communicated in documentation to users upgrading the project.hello-tegg/test/controller/HelloController.test.ts (1)
1-2: Modernize import statements.
The import of the strict assertion module fromnode:assertand the update of the app mock source to@eggjs/mock/bootstrapare in line with the project's upgrade. These changes promote consistency and utilize namespaced Node.js modules for better clarity.hello-tegg/package.json (3)
6-7: Adopt eggctl for process management.
Replacing the old start/stop commands with"eggctl start --daemon"and"eggctl stop"modernizes process management and is expected for Egg v4. Confirm that theeggctlCLI tool is available in all deployment environments.
10-15: Refine test and CI related scripts.
The new scripts (pretest,test,preci,ci, andpostci) add improved cleaning and linting steps around testing and CI operations. This creates a more robust workflow. Please verify that these commands execute as intended in both local and CI contexts.
17-18: Enhance build cleanup routines.
The addition of"clean": "tsc -b --clean"and"prepublishOnly": "npm run clean && tsc"ensures that TypeScript build artifacts are managed consistently.helloworld/package.json (4)
4-6: Upgrade core dependencies for modern Egg support.
By adding"@eggjs/scripts": "^4.0.0"and updating"egg"to"beta", the project now aligns with Egg v4. Verify that the beta release offers the necessary stability and performance improvements.
8-16: Standardize devDependencies across the project.
The updates to devDependencies (including"@eggjs/bin": "7","@eggjs/mock": "6","@eggjs/tsconfig": "1","@types/mocha": "10","@types/node": "22","eslint": "8","eslint-config-egg": "14", and"typescript": "5") reflect a coherent move towards pinned, modern versions. This ensures consistent development and CI environments.
17-30: Revise npm scripts for refined development and CI workflows.
The new scripts—such as"start","stop","lint","predev","pretest","preci","ci","postci","clean", and"prepublishOnly"—are well structured. They leverage tools likeeggctlandegg-bin, which are expected in the Egg v4 ecosystem. Double-check that these commands integrate smoothly with your build and deployment pipelines.
31-33: Add repository metadata.
The inclusion of a"repository"field helps with project metadata and discoverability. This is a useful addition that improves overall project transparency.
| npm run ci | ||
| cd .. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for npm commands.
The script currently doesn't preserve exit codes from the npm commands. If npm run ci fails, the script will still continue to the next test. This could hide test failures.
- npm run ci
- cd ..
+ if ! npm run ci; then
+ echo "Tests failed in $1"
+ cd ..
+ return 1
+ fi
+ cd ..📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| npm run ci | |
| cd .. | |
| if ! npm run ci; then | |
| echo "Tests failed in $1" | |
| cd .. | |
| return 1 | |
| fi | |
| cd .. |
| let ctx: any; | ||
| // let ctx: EggContext; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using any type
Using any for the ctx variable bypasses TypeScript's type checking benefits, which can lead to runtime errors. The commented alternative suggests EggContext might be a better option.
Consider uncommenting and using the proper type:
- let ctx: any;
- // let ctx: EggContext;
+ // Import EggContext at the top
+ let ctx: EggContext;If EggContext is not available in egg v4, check the documentation for the appropriate type to use instead of any.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let ctx: any; | |
| // let ctx: EggContext; | |
| // Import EggContext at the top | |
| let ctx: EggContext; |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
bin/test.sh (1)
9-9: 🛠️ Refactor suggestion
⚠️ Potential issueEnhance Error Handling for npm run ci
The command
npm run ciis not currently checked for failure, which means that if it fails, the script will still proceed tocd ..and potentially mask test failures. Consider wrapping it with a conditional check as follows:- npm run ci + if ! npm run ci; then + echo "Tests failed in $1" + cd .. + return 1 + fiThis change ensures that if
npm run cifails, the function will return a non-zero exit code immediately.
🧹 Nitpick comments (1)
bin/test.sh (1)
3-11: Suggestion: Avoid Using “test” as a Function NameThe function is named
test, which conflicts with the built-in POSIX commandtestand may lead to unexpected behavior in more complex scripts. Consider renaming it to something more descriptive, such asrun_testsorexecute_test, to avoid potential ambiguities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/test.sh(1 hunks)
🔇 Additional comments (2)
bin/test.sh (2)
5-5: Good Error Handling for Directory ChangeThe use of
cd "$1" || { echo "Error: Directory $1 not found"; return 1; }properly handles the case when the directory change fails. This ensures that the script does not continue in an unexpected directory.
13-14: Solid Exit Handling for Test CallsThe function calls at the end (
test helloworld || exit 1andtest hello-tegg || exit 1) correctly propagate errors by exiting if any test fails. This helps maintain the integrity of the testing process.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
bin/test.sh (2)
7-7: 🛠️ Refactor suggestionEnhance Directory Change Error Handling
Althoughset -ecauses the script to terminate on errors, it is helpful to provide an explicit error message if changing into the directory fails. Consider revising the command as follows:- cd "$1" + cd "$1" || { echo "Error: Directory '$1' not found"; exit 1; }
11-11: 🛠️ Refactor suggestionImprove Error Reporting for npm Command
Whileset -ewill exit on a failure innpm run ci, adding an explicit check with an informative error message can make debugging easier. For example:- npm run ci + npm run ci || { echo "Error: 'npm run ci' failed in $1"; exit 1; }
🧹 Nitpick comments (1)
bin/test.sh (1)
15-17: Validate Test Invocation Order and Informative Logging
The script calls thetestfunction sequentially for multiple directories. Withset -e, any failure will abort the script, which is good. However, consider enhancing the logging—for example, by including which test directory is starting—to aid in troubleshooting if a failure occurs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bin/test.sh(1 hunks)body-parser-example/app/controller/home.js(1 hunks)body-parser-example/config/config.default.js(0 hunks)body-parser-example/package.json(1 hunks)body-parser-example/test/home.test.js(1 hunks)package.json(2 hunks)
💤 Files with no reviewable changes (1)
- body-parser-example/config/config.default.js
✅ Files skipped from review due to trivial changes (1)
- body-parser-example/app/controller/home.js
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
🔇 Additional comments (7)
body-parser-example/test/home.test.js (2)
1-1: Import package updated to match Egg v4 requirements.The import statement has been updated from
egg-mock/bootstrapto@eggjs/mock/bootstrap, which is necessary for compatibility with Egg v4. This change is part of the framework upgrade and follows the pattern of using scoped npm packages.
3-3: Test description updated to match actual file path.The test description has been updated from
'test/app/controller/home.test.js'to'test/home.test.js'to accurately reflect the actual file path. This improves code readability and maintainability without affecting functionality.body-parser-example/package.json (5)
2-2: Project Name Updated
The project name has been updated to"body-parser-example". Please ensure that this naming change is reflected consistently in the repository documentation and any related scripts.
5-5: Upgraded Egg Dependency to v4
The Egg framework dependency is now set to version"4". Make sure that the breaking changes (including the minimum required Node.js version) are clearly documented in the upgrade guide or release notes.
9-9: Updated Dev Dependency: @eggjs/bin
The legacy"egg-bin"has been replaced with"@eggjs/bin"at version"7". Verify that any CLI scripts or tooling that rely on this package work as expected with the new version.
10-10: Updated Dev Dependency: @eggjs/mock
The mock dependency has been updated from"egg-mock"to"@eggjs/mock"at version"6". Ensure that all tests utilizing this package are updated accordingly.
15-15: Renamed Test Script Key
The test script key is now"ci"(renaming"cov"), even though the command remains"egg-bin cov". Please confirm that this naming aligns with your CI conventions and update documentation if necessary.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/test.js (1)
11-16: Remove unused constructor options.The
-cflag options are defined but never used since therun()method no longer acceptsargvand the flag logic on line 41 was removed.Apply this diff to remove the unused options:
constructor(rawArgs) { super(rawArgs); - - this.options = { - c: { - type: 'boolean', - description: 'in China', - }, - }; }
🧹 Nitpick comments (1)
README.md (1)
22-22: Use proper Markdown heading instead of emphasis.Bold text is being used where a proper heading would be more semantic and accessible.
Apply this diff:
-**Recommend to use Node >= 20** +## Recommended Node.js Version + +Node.js >= 20
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci.yml(1 hunks)README.md(2 hunks)bin/test.js(2 hunks)bin/test.sh(1 hunks)package.json(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
22-22: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (6)
bin/test.js (1)
41-41: LGTM! Simplified to use new test tooling.The change from conditional
npmupdateto unconditionalut updatealigns with the new utoo-based test infrastructure introduced in this PR..github/workflows/ci.yml (1)
12-16: LGTM! CI updated for Egg v4 requirements.The workflow correctly:
- Updates Node.js versions to 20, 22, 24 (aligning with >= 20 requirement)
- Adds utoo tooling installation
- References the updated workflow path
bin/test.sh (2)
3-3: Strict error handling is in place.The
set -eoption ensures the script exits immediately if any command fails, which addresses the error handling concerns raised in past review comments. However, explicit error messages (as suggested in past comments) would provide clearer feedback when failures occur.The current approach relies on shell-level error handling. Consider whether more explicit error messages would improve the developer experience when tests fail.
9-9: Verify the intent behind removing node_modules.The script removes
node_modulesandpackage-lock.jsonbefore each test. This ensures a clean install but increases CI time significantly.Is the clean install necessary for every test run, or could this be optimized for faster CI execution?
package.json (2)
21-21: Verify that lint is intentionally skipped in CI.The
ciscript previously rannpm run lint && npm testbut now only runsnpm test. The lint step is still defined (line 18) but always passes withecho 'ignore'.Confirm this is intentional. If linting should still be part of CI, restore it:
- "ci": "npm test", + "ci": "npm run lint && npm test",
33-33: LGTM! Node version requirement aligned with Egg v4.The Node >= 20 requirement is consistent with the PR objectives and aligns with the CI configuration.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
bin/test.sh (3)
7-7: Add explicit error handling forcdcommand.The
cdcommand should explicitly return/exit on failure to prevent subsequent commands from running in the wrong directory, even withset -e.Apply this diff:
- cd "$1" + cd "$1" || return 1
11-12: Ensurenpm run cifailure propagates and stopscd ..execution.If
npm run cifails, thecd ..command still executes, masking the failure. Use explicit error handling to stop execution and prevent directory state from being obscured.Apply this diff to make the error explicit:
- npm run ci - cd .. + if ! npm run ci; then + echo "Tests failed in $1" + cd .. + return 1 + fi + cd ..Alternatively, with
set -e, you can simplify by ensuringcd ..is not treated as part of a command group that suppresses errors. Consider restructuring to keepcd ..in a trap or ensure it's called from a parent context.
15-17: Add error handling for test function calls.The script calls
test()three times without checking exit status. If the first or second test fails, the script continues and exits with success.Apply this diff:
- test body-parser-example - test helloworld - test hello-tegg + test body-parser-example || exit 1 + test helloworld || exit 1 + test hello-tegg || exit 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/ci.yml(1 hunks)README.md(2 hunks)bin/test.sh(1 hunks)body-parser-example/package.json(1 hunks)hello-tegg/app/controller/HelloController.ts(1 hunks)hello-tegg/app/middleware/trace_method.ts(1 hunks)hello-tegg/config/config.default.ts(1 hunks)hello-tegg/config/plugin.ts(1 hunks)hello-tegg/package.json(1 hunks)hello-tegg/test/biz/HelloService.test.ts(1 hunks)hello-tegg/test/controller/HelloController.test.ts(1 hunks)helloworld/config/config.default.ts(1 hunks)helloworld/package.json(1 hunks)package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- hello-tegg/app/controller/HelloController.ts
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
22-22: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (9)
.github/workflows/ci.yml (2)
10-22: LGTM!The matrix-based CI workflow is well-structured. It correctly tests across supported Node versions (20, 22, 24) and multiple OSes, aligns with the egg v4 upgrade and Node >= 18.19.0 requirement, and includes proper concurrency control to prevent redundant runs.
24-37: Explicit workflow steps are clear and correct.Checkout, Node setup, dependency installation, and test steps are properly configured. The use of
npminstallfor faster installs in CI is appropriate.helloworld/config/config.default.ts (1)
1-5: Remove hardcoded secret and use environment variables with development fallback.The TypeScript configuration and
defineConfigapproach aligns well with Egg v4. However, the hardcodedkeysvalue presents a security risk: Egg useskeysfor signing cookies, and production deployments must never hardcode secrets.For this example, use an environment variable with a fallback for local development:
import { defineConfig } from 'egg'; export default defineConfig({ - keys: 'my secret keys', + keys: process.env.EGG_KEYS || 'my secret keys', // dev only });In production, secrets should be stored in environment variables rather than hardcoded in files, making the app configurable.
hello-tegg/app/middleware/trace_method.ts (1)
1-1: LGTM! Type-only import is appropriate.The change to
import typeis correct sinceEggContextandNextare only used for type annotations, not at runtime. This improves tree-shaking and makes the intent explicit.hello-tegg/config/plugin.ts (1)
1-1: LGTM! Type-only import is appropriate.Since
EggPluginis only used for type annotation, converting toimport typeis correct and follows TypeScript best practices.hello-tegg/test/biz/HelloService.test.ts (1)
1-16: LGTM! Test modernization aligns with Egg v4 patterns.The updates to use
@eggjs/mock/bootstrap,node:assert/strict, and a singlebeforehook follow the new testing conventions. The explicit.tsextension in the import is consistent with ES module resolution in TypeScript.hello-tegg/test/controller/HelloController.test.ts (1)
1-16: LGTM! Test refactoring improves clarity.The migration to
@eggjs/mock/bootstrapandnode:assert/strictis consistent with Egg v4 patterns. Separating the HTTP request from the assertion makes the test more readable.hello-tegg/config/config.default.ts (1)
1-8: LGTM! Correctly adopts Egg v4 config factory pattern.The migration from a static config object to
defineConfigFactoryaligns with Egg v4's factory-based configuration approach, which provides better type inference and access toappInfo.body-parser-example/package.json (1)
15-15: Based on my web search verification, @eggjs/bin is the current package name (version 7.3.1) that replaced the older egg-bin. Importantly, the @eggjs/bin repository's own documentation shows scripts using "egg-bin cov", indicating that npm packages supply a bin field in package.json which maps command names to local files, and these are symlinked into node_modules/.bin.The review comment's concern appears to be unfounded. The evidence indicates that @eggjs/bin maintains backward compatibility by exporting the "egg-bin" binary name, as demonstrated by the official eggjs/bin GitHub repository still using "egg-bin cov" in its documentation. The npm script "egg-bin cov" should resolve correctly after npm install when @eggjs/bin is installed.
Verify the bin command resolves correctly after package rename.
No action required. The package dependency is now "@eggjs/bin" instead of "egg-bin", and the @eggjs/bin repository's documentation confirms the binary command remains "egg-bin", maintaining backward compatibility. The npm script using "egg-bin cov" will resolve correctly.
| { | ||
| "name": "bodyParser", | ||
| "name": "body-parser-example", | ||
| "version": "1.0.0", | ||
| "dependencies": { | ||
| "egg": "^2", | ||
| "egg": "beta", | ||
| "xml-js": "^1.6.9" | ||
| }, | ||
| "devDependencies": { | ||
| "egg-bin": "^4.3.5", | ||
| "egg-mock": "^3.13.1" | ||
| "@eggjs/bin": "beta", | ||
| "@eggjs/mock": "beta" | ||
| }, | ||
| "scripts": { | ||
| "dev": "egg-bin dev", | ||
| "test": "egg-bin test", | ||
| "cov": "egg-bin cov" | ||
| "ci": "egg-bin cov" | ||
| }, | ||
| "private": true | ||
| } |
There was a problem hiding this comment.
Add Node.js engine constraint to enforce minimum version.
Per the PR objectives, egg v4 requires Node.js >= 18.19.0. Add an "engines" field to package.json to enforce this constraint for developers and CI environments.
Apply this diff to add the engines field:
"scripts": {
"dev": "egg-bin dev",
"test": "egg-bin test",
"ci": "egg-bin cov"
},
+ "engines": {
+ "node": ">=18.19.0"
+ },
"private": trueBased on learnings about egg v4 migration requirements.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| "name": "bodyParser", | |
| "name": "body-parser-example", | |
| "version": "1.0.0", | |
| "dependencies": { | |
| "egg": "^2", | |
| "egg": "beta", | |
| "xml-js": "^1.6.9" | |
| }, | |
| "devDependencies": { | |
| "egg-bin": "^4.3.5", | |
| "egg-mock": "^3.13.1" | |
| "@eggjs/bin": "beta", | |
| "@eggjs/mock": "beta" | |
| }, | |
| "scripts": { | |
| "dev": "egg-bin dev", | |
| "test": "egg-bin test", | |
| "cov": "egg-bin cov" | |
| "ci": "egg-bin cov" | |
| }, | |
| "private": true | |
| } | |
| { | |
| "name": "body-parser-example", | |
| "version": "1.0.0", | |
| "dependencies": { | |
| "egg": "beta", | |
| "xml-js": "^1.6.9" | |
| }, | |
| "devDependencies": { | |
| "@eggjs/bin": "beta", | |
| "@eggjs/mock": "beta" | |
| }, | |
| "scripts": { | |
| "dev": "egg-bin dev", | |
| "test": "egg-bin test", | |
| "ci": "egg-bin cov" | |
| }, | |
| "engines": { | |
| "node": ">=18.19.0" | |
| }, | |
| "private": true | |
| } |
🤖 Prompt for AI Agents
In body-parser-example/package.json around lines 1 to 18, add an "engines" field
to enforce Node.js >=18.19.0 required by egg v4; update the top-level JSON to
include "engines": { "node": ">=18.19.0" } so CI and developers are constrained
to the minimum Node version (ensure valid JSON comma placement after the
"version" or before "dependencies").
| "version": "1.0.0", | ||
| "dependencies": { | ||
| "egg": "^2", | ||
| "egg": "beta", |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid loose "beta" version tags; pin to specific beta releases.
Using "beta" as a version spec is too permissive and may introduce non-deterministic installs. Different CI runs or developer machines could resolve to different beta patch versions, breaking reproducibility. Since this is an example repo migrating to egg v4 (beta), pin to specific versions or use a narrower semver range.
Apply this diff to pin specific beta versions:
"dependencies": {
- "egg": "beta",
+ "egg": "^4.0.0-beta",
"xml-js": "^1.6.9"
},
"devDependencies": {
- "@eggjs/bin": "beta",
- "@eggjs/mock": "beta"
+ "@eggjs/bin": "^4.0.0-beta",
+ "@eggjs/mock": "^4.0.0-beta"
},Alternatively, inspect the eggjs org releases to find the latest stable beta tag (e.g., 4.0.0-beta.5) and pin those specific versions.
Also applies to: 9-10
I'll verify the review comment by examining the package.json file and checking the available beta versions from the eggjs organization.
import json
# Try to parse package.json if accessible
try:
with open("body-parser-example/package.json", "r") as f:
data = json.load(f)
print("Dependencies:")
if "dependencies" in data:
for dep, version in data.get("dependencies", {}).items():
if "egg" in dep.lower():
print(f" {dep}: {version}")
print("\nDevDependencies:")
if "devDependencies" in data:
for dep, version in data.get("devDependencies", {}).items():
if "egg" in dep.lower() or "eggjs" in dep.lower():
print(f" {dep}: {version}")
except Exception as e:
print(f"Error: {e}")
🏁 Script executed:
#!/bin/bash
# Inspect the package.json file to verify current state
if [ -f "body-parser-example/package.json" ]; then
cat body-parser-example/package.json
else
echo "File not found"
fiLength of output: 321
🏁 Script executed:
#!/bin/bash
# Search for egg, @eggjs/bin, and @eggjs/mock version references
rg -n '"egg"|"@eggjs/bin"|"@eggjs/mock"' body-parser-example/package.json -A 0 -B 0Length of output: 77
🏁 Script executed:
#!/bin/bash
# Check if there are any other package.json files that might reference these packages
fd 'package.json' --exec grep -l "egg\|@eggjs" {} \;Length of output: 1277
🌐 Web query:
egg v4 beta versions npm registry eggjs
💡 Result:
You can find Egg v4 beta releases on the npm registry and install them either by the beta version tag or the next dist-tag.
Quick options:
- Install the latest beta via dist-tag: npm i egg@next. [1][2]
- Install a specific beta (example): npm i egg@4.1.0-beta.13. [3][2]
References:
- egg package (npm) — versions and dist-tags. [https://www.npmjs.com/package/egg]. [1]
- egg v4 release notes / dist-tag mentions (v4.0.x / @next). [newreleases / eggjs/egg]. [2]
- Example prerelease listing showing v4.1.0-beta.* and published beta packages. [3]
🌐 Web query:
@eggjs/bin @eggjs/mock latest beta versions
💡 Result:
- @eggjs/bin — latest beta: 7.3.1-beta.6. [1][2]
- @eggjs/mock — latest beta: 6.1.0-beta.8. [1][3]
Sources:
[1] Release listing for egg v4.1.0 prereleases (shows published prerelease package versions).
[2] @eggjs/bin npm page (stable/latest info).
[3] @eggjs/mock package/version listing.
Pin specific beta versions instead of loose "beta" dist-tag for reproducibility.
Using "beta" as a version specifier is a floating dist-tag that will resolve to different versions as new beta releases are published. This breaks reproducibility—installations at different times will pull different beta patch versions, complicating debugging and testing. Since the eggjs org has published specific beta versions (e.g., egg 4.1.0-beta.13, @eggjs/bin 7.3.1-beta.6, @eggjs/mock 6.1.0-beta.8), pin to a specific beta release instead.
Apply this diff:
"dependencies": {
- "egg": "beta",
+ "egg": "^4.0.0-beta",
"xml-js": "^1.6.9"
},
"devDependencies": {
- "@eggjs/bin": "beta",
- "@eggjs/mock": "beta"
+ "@eggjs/bin": "^4.0.0-beta",
+ "@eggjs/mock": "^4.0.0-beta"
},Alternatively, pin exact versions (e.g., "egg": "4.0.0-beta.13") if you need strict reproducibility across all installs.
Also applies to: lines 9–10
🤖 Prompt for AI Agents
In body-parser-example/package.json around line 5 (and also update lines 9–10),
the dependency version "beta" is a floating dist-tag causing non-reproducible
installs; replace the loose "beta" specifier with a concrete beta release (for
example "4.1.0-beta.13" for egg or the exact beta versions published by the
eggjs org) so installs resolve to a fixed package version—update the other beta
entries on lines 9–10 to their corresponding specific beta versions as well to
ensure reproducibility.
| "@eggjs/scripts": "beta", | ||
| "@eggjs/tegg": "beta", | ||
| "@eggjs/tegg-config": "beta", | ||
| "@eggjs/tegg-controller-plugin": "beta", | ||
| "@eggjs/tegg-plugin": "beta", | ||
| "egg": "beta" |
There was a problem hiding this comment.
🧩 Analysis chain
Pin specific beta versions to avoid breaking changes.
Using the "beta" tag for dependencies means the package will resolve to the latest beta version at install time, which can introduce breaking changes unexpectedly between installs. This can lead to inconsistent behavior across different environments and makes builds non-reproducible.
Consider pinning to specific beta versions. Run this script to check the current beta versions available:
Also applies to: 35-37
🏁 Script executed:
#!/bin/bash
# Description: Check latest beta versions for @eggjs packages
echo "Checking beta versions for @eggjs packages..."
for pkg in "@eggjs/scripts" "@eggjs/tegg" "@eggjs/tegg-config" "@eggjs/tegg-controller-plugin" "@eggjs/tegg-plugin" "egg" "@eggjs/tsconfig" "@eggjs/bin" "@eggjs/mock"; do
echo "=== $pkg ==="
npm view "$pkg" dist-tags.beta versions --json | jq -r 'if type == "array" then .[-5:] else . end'
doneLength of output: 16364
Pin specific beta versions to avoid non-reproducible builds.
Using "beta" as a version specifier resolves to the latest beta version at install time. This creates the reproducibility issues you've identified. The npm registry data confirms multiple beta versions are available for each package—installing at different times would pull different versions without a lock file.
Recommended pinned versions based on current latest betas:
"@eggjs/scripts": "5.0.0-beta.28""@eggjs/tegg": "4.0.0-beta.11""@eggjs/tegg-config": "4.0.0-beta.11""@eggjs/tegg-controller-plugin": "4.0.0-beta.11""@eggjs/tegg-plugin": "4.0.0-beta.11""egg": "4.1.0-beta.28"
Apply the same approach to the dependencies at lines 35-37.
🤖 Prompt for AI Agents
In hello-tegg/package.json around lines 25 to 30 (and apply same change to the
related dependencies at lines 35-37), the packages use the loose "beta" tag
which yields non-reproducible installs; replace each "beta" specifier with the
pinned beta versions provided by the reviewer: set "@eggjs/scripts" to
"5.0.0-beta.28", "@eggjs/tegg" to "4.0.0-beta.11", "@eggjs/tegg-config" to
"4.0.0-beta.11", "@eggjs/tegg-controller-plugin" to "4.0.0-beta.11",
"@eggjs/tegg-plugin" to "4.0.0-beta.11", and "egg" to "4.1.0-beta.28", and make
the same pinning for the matching entries at lines 35-37 to ensure reproducible
builds.
| "engines": { | ||
| "node": ">=20.0.0" | ||
| } |
There was a problem hiding this comment.
Node.js version requirement mismatch with PR description.
The PR description states "drop Node.js < 18.19.0 support," but this package requires >=20.0.0. This inconsistency may cause confusion. Additionally, the helloworld package doesn't specify an engines field at all.
Please clarify:
- Is the minimum Node.js version 18.19.0 or 20.0.0?
- Should all example packages have consistent engine requirements?
🤖 Prompt for AI Agents
In hello-tegg/package.json around lines 43-45 the "engines" field requires
"node": ">=20.0.0" which conflicts with the PR description that drops Node.js
<18.19.0; decide the project-wide minimum (either 18.19.0 or 20.0.0), update
this package.json to use "node": ">=<chosen_version>", add the same "engines"
entry to the helloworld package.json to keep examples consistent, and update the
PR description to state the chosen minimum Node.js version so they match.
| { | ||
| "name": "helloworld", | ||
| "private": true, | ||
| "type": "module", | ||
| "dependencies": { | ||
| "egg": "^1.10.1" | ||
| "@eggjs/scripts": "beta", | ||
| "egg": "beta" | ||
| }, | ||
| "devDependencies": { | ||
| "egg-bin": "^4.3.5", | ||
| "egg-mock": "^3.13.1" | ||
| "@eggjs/bin": "beta", | ||
| "@eggjs/mock": "beta", | ||
| "@eggjs/tsconfig": "beta", | ||
| "@types/mocha": "10", | ||
| "@types/node": "22", | ||
| "eslint": "8", | ||
| "eslint-config-egg": "14", | ||
| "typescript": "5" | ||
| }, | ||
| "scripts": { | ||
| "start": "eggctl start --daemon", | ||
| "stop": "eggctl stop", | ||
| "lint": "eslint . --ext .ts", | ||
| "predev": "npm run clean && npm run lint -- --fix", | ||
| "dev": "egg-bin dev", | ||
| "pretest": "npm run clean && npm run lint -- --fix", | ||
| "test": "egg-bin test", | ||
| "cov": "egg-bin cov" | ||
| "preci": "npm run clean && npm run lint", | ||
| "ci": "egg-bin cov", | ||
| "postci": "npm run prepublishOnly && npm run clean", | ||
| "clean": "tsc -b --clean", | ||
| "prepublishOnly": "npm run clean && tsc" | ||
| }, | ||
| "private": true | ||
| "repository": "git@github.com:eggjs/examples.git" | ||
| } |
There was a problem hiding this comment.
Missing Node.js engine requirement.
Unlike hello-tegg/package.json, this package doesn't specify an engines field. Given that the PR drops support for Node.js < 18.19.0, all example packages should explicitly declare their minimum Node.js version to prevent users from running them on unsupported versions.
Add an engines field to enforce the minimum Node.js version:
"repository": "git@github.com:eggjs/examples.git"
+ "engines": {
+ "node": ">=18.19.0"
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| "name": "helloworld", | |
| "private": true, | |
| "type": "module", | |
| "dependencies": { | |
| "egg": "^1.10.1" | |
| "@eggjs/scripts": "beta", | |
| "egg": "beta" | |
| }, | |
| "devDependencies": { | |
| "egg-bin": "^4.3.5", | |
| "egg-mock": "^3.13.1" | |
| "@eggjs/bin": "beta", | |
| "@eggjs/mock": "beta", | |
| "@eggjs/tsconfig": "beta", | |
| "@types/mocha": "10", | |
| "@types/node": "22", | |
| "eslint": "8", | |
| "eslint-config-egg": "14", | |
| "typescript": "5" | |
| }, | |
| "scripts": { | |
| "start": "eggctl start --daemon", | |
| "stop": "eggctl stop", | |
| "lint": "eslint . --ext .ts", | |
| "predev": "npm run clean && npm run lint -- --fix", | |
| "dev": "egg-bin dev", | |
| "pretest": "npm run clean && npm run lint -- --fix", | |
| "test": "egg-bin test", | |
| "cov": "egg-bin cov" | |
| "preci": "npm run clean && npm run lint", | |
| "ci": "egg-bin cov", | |
| "postci": "npm run prepublishOnly && npm run clean", | |
| "clean": "tsc -b --clean", | |
| "prepublishOnly": "npm run clean && tsc" | |
| }, | |
| "private": true | |
| "repository": "git@github.com:eggjs/examples.git" | |
| } | |
| { | |
| "name": "helloworld", | |
| "private": true, | |
| "type": "module", | |
| "dependencies": { | |
| "@eggjs/scripts": "beta", | |
| "egg": "beta" | |
| }, | |
| "devDependencies": { | |
| "@eggjs/bin": "beta", | |
| "@eggjs/mock": "beta", | |
| "@eggjs/tsconfig": "beta", | |
| "@types/mocha": "10", | |
| "@types/node": "22", | |
| "eslint": "8", | |
| "eslint-config-egg": "14", | |
| "typescript": "5" | |
| }, | |
| "scripts": { | |
| "start": "eggctl start --daemon", | |
| "stop": "eggctl stop", | |
| "lint": "eslint . --ext .ts", | |
| "predev": "npm run clean && npm run lint -- --fix", | |
| "dev": "egg-bin dev", | |
| "pretest": "npm run clean && npm run lint -- --fix", | |
| "test": "egg-bin test", | |
| "preci": "npm run clean && npm run lint", | |
| "ci": "egg-bin cov", | |
| "postci": "npm run prepublishOnly && npm run clean", | |
| "clean": "tsc -b --clean", | |
| "prepublishOnly": "npm run clean && tsc" | |
| }, | |
| "repository": "git@github.com:eggjs/examples.git", | |
| "engines": { | |
| "node": ">=18.19.0" | |
| } | |
| } |
🤖 Prompt for AI Agents
In helloworld/package.json around lines 1 to 34, the package is missing an
"engines" field to enforce the minimum Node.js version; add an "engines" object
with "node": ">=18.19.0" (matching the repo minimum) so users are prevented from
running the example on unsupported Node versions, and commit the updated
package.json.
| "@eggjs/scripts": "beta", | ||
| "egg": "beta" |
There was a problem hiding this comment.
Pin specific beta versions to avoid breaking changes.
Using the "beta" tag without version pinning can lead to non-reproducible builds and unexpected breaking changes. This is particularly risky during active beta development.
Consider pinning to specific beta versions as suggested for hello-tegg/package.json.
Also applies to: 10-12
🤖 Prompt for AI Agents
In helloworld/package.json around lines 6 to 7 (and also check lines 10-12), the
dependencies use the unpinned "beta" tag which risks non-reproducible builds and
breaking changes; update the "egg" and "@eggjs/scripts" entries to specific beta
release versions (e.g., "beta.X.Y" or the exact semver found in
hello-tegg/package.json) instead of the bare "beta", commit the updated
package.json, and run npm install/npm ci to update lockfile so CI and local
builds use the pinned beta versions.
| ``` | ||
|
|
||
| **Recommend to use Node >= 8** | ||
| **Recommend to use Node >= 20** |
There was a problem hiding this comment.
Convert emphasis to proper heading to resolve markdown lint warning.
The line uses bold emphasis instead of a proper heading structure, which triggers MD036 from markdownlint-cli2.
Apply this diff:
-**Recommend to use Node >= 20**
+## Recommend to use Node >= 20📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Recommend to use Node >= 20** | |
| ## Recommend to use Node >= 20 |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
22-22: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In README.md around line 22, the line currently uses bold emphasis ("**Recommend
to use Node >= 20**") which triggers markdownlint MD036; replace the bold text
with a proper markdown heading (e.g., prepend one or two # characters like "#
Recommend to use Node >= 20" or "## Recommend to use Node >= 20" to match
surrounding heading levels), remove the surrounding ** markers, and ensure a
single blank line before and after the new heading for proper formatting.
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
eggjs/egg#5257
Summary by CodeRabbit