feat: upgrade to egg v4 #126
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257
WalkthroughThe pull request comprises multiple updates across configuration, testing, and application code. Several legacy files have been removed while new files—including TypeScript configuration, ESLint settings, and updated controllers—have been introduced. The CI workflow and documentation now reflect updated Node.js and framework version requirements. Test files and scripts have been refactored and augmented, and dependency as well as export syntax updates have been applied across package files and source code. Overall, the changes modernize configurations and testing approaches and migrate legacy modules to ES6 module syntax. Changes
Sequence Diagram(s)sequenceDiagram
participant Shell
participant Dir
participant npm
Shell->>Dir: cd into target directory (helloworld or hello-tegg)
Dir-->>Shell: current working directory
Shell->>Dir: Remove node_modules & package-lock.json
Shell->>npm: Run "npm ci" from registry
npm-->>Shell: Install dependencies
Shell->>Dir: Return to previous directory
sequenceDiagram
participant Client
participant Router
participant HomeController
participant FooController
Client->>Router: GET "/"
Router->>HomeController: Call render()
HomeController-->>Router: Return "Hello World"
Router->>Client: Deliver response
Client->>Router: GET "/foo"
Router->>FooController: Call render()
FooController-->>Router: Return status 400 with {foo:"bar"}
Router->>Client: Deliver response
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (
|
|
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:✅ 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:❓ 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.
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
eggjs/egg#5257
Summary by CodeRabbit
New Features
Documentation
Chores