feat: support egg v4 #114
feat: support egg v4 #114
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 15 minutes and 28 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 ignored due to path filters (1)
📒 Files selected for processing (38)
WalkthroughThis pull request removes the CodeQL workflow and adjusts several GitHub Actions workflows by removing triggers and updating configurations. The Node.js version support in CI and various dependencies in the package manifest have been updated. In addition, a new "Contributors" section is added to the documentation. Numerous source and test files now standardize all imports to use the Node.js built‐in modules with the Changes
Sequence Diagram(s)sequenceDiagram
participant Script as eggInfo Script
participant Loader as Loader Module
participant FS as File System
Script->>Loader: loadPlugin()
Note right of Script: Awaits plugin loading
Script->>Loader: loadConfig()
Note right of Script: Awaits config loading
Script->>FS: Write eggInfo to file
Script-->>Script: Complete execution
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/[email protected], npm/[email protected], npm/[email protected] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
test/generators/plugin.test.ts (1)
15-15: Remove commented debugging code.The commented console.log statement should be removed as it appears to be debugging code.
- // console.log(result);src/scripts/eggInfo.ts (4)
26-26: Use optional chaining for safer property access.Replace the nested object access with optional chaining for better safety.
-const framework = (utils.getPkgInfo(cwd).egg || {}).framework || process.env.ETS_SCRIPT_FRAMEWORK || 'egg'; +const framework = utils.getPkgInfo(cwd)?.egg?.framework || process.env.ETS_SCRIPT_FRAMEWORK || 'egg';🧰 Tools
🪛 Biome (1.9.4)
[error] 26-26: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
32-34: Consider structured error logging.The error logging could be improved by using structured logging format for better error tracking and debugging.
-console.warn('[egg-ts-helper] WARN loader.loadPlugin() error: %s, cwd: %s, framework: %s', - e, cwd, framework); +console.warn('[egg-ts-helper]', { + level: 'WARN', + error: e instanceof Error ? e.message : String(e), + context: { cwd, framework }, + operation: 'loader.loadPlugin()' +});Also applies to: 49-51
39-39: Use optional chaining in filepath check.The filepath check could be simplified using optional chaining.
-if (filepath && filepath.substring(filepath.lastIndexOf(path.sep) + 1).startsWith('config.')) { +if (filepath?.substring(filepath.lastIndexOf(path.sep) + 1)?.startsWith('config.')) {🧰 Tools
🪛 Biome (1.9.4)
[error] 39-39: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
86-86: Enhance warning message with recovery steps.The warning message could be more helpful by including recovery steps or troubleshooting information.
-console.warn('[egg-ts-helper] WARN cannot find @eggjs/core from frameworkPath: %s', frameworkPath); +console.warn('[egg-ts-helper] WARN cannot find @eggjs/core from frameworkPath: %s. Please ensure @eggjs/core is installed and the framework path is correct.', frameworkPath);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/fixtures/real/node_modules/egg-test/index.d.tsis excluded by!**/node_modules/**
📒 Files selected for processing (40)
.github/workflows/codeql.yml(0 hunks).github/workflows/nodejs.yml(1 hunks).github/workflows/release.yml(1 hunks)README.md(1 hunks)README.zh-CN.md(1 hunks)package.json(3 hunks)src/cmd/init.ts(1 hunks)src/command.ts(1 hunks)src/config.ts(1 hunks)src/core.ts(1 hunks)src/generator.ts(1 hunks)src/generators/class.ts(1 hunks)src/generators/config.ts(1 hunks)src/generators/custom.ts(1 hunks)src/generators/egg.ts(1 hunks)src/generators/extend.ts(1 hunks)src/generators/plugin.ts(1 hunks)src/register.ts(1 hunks)src/scripts/eggInfo.ts(4 hunks)src/utils.ts(1 hunks)src/watcher.ts(1 hunks)test/bin.test.ts(1 hunks)test/cmd/clean.test.ts(1 hunks)test/cmd/init.test.ts(1 hunks)test/fixtures/real-unittest/test/index.test.ts(1 hunks)test/fixtures/real/app.ts(1 hunks)test/fixtures/real/config/config.default.ts(1 hunks)test/fixtures/real/test/index.test.ts(1 hunks)test/fixtures/real/tsconfig-base.json(1 hunks)test/generator.test.ts(1 hunks)test/generators/class.test.ts(1 hunks)test/generators/config.test.ts(1 hunks)test/generators/extend.test.ts(1 hunks)test/generators/plugin.test.ts(1 hunks)test/generators/utils.ts(1 hunks)test/index.test.ts(2 hunks)test/register.test.ts(1 hunks)test/utils.test.ts(1 hunks)test/utils.ts(1 hunks)test/watcher.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/codeql.yml
✅ Files skipped from review due to trivial changes (26)
- src/generators/plugin.ts
- src/config.ts
- src/utils.ts
- src/generators/class.ts
- src/generators/custom.ts
- src/generators/config.ts
- src/command.ts
- src/generator.ts
- README.zh-CN.md
- src/generators/egg.ts
- test/register.test.ts
- test/utils.test.ts
- test/watcher.test.ts
- src/core.ts
- test/generators/extend.test.ts
- test/bin.test.ts
- src/generators/extend.ts
- test/fixtures/real/test/index.test.ts
- test/generators/class.test.ts
- test/utils.ts
- test/cmd/init.test.ts
- src/cmd/init.ts
- test/fixtures/real-unittest/test/index.test.ts
- README.md
- test/cmd/clean.test.ts
- src/watcher.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/scripts/eggInfo.ts
[error] 26-26: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 39-39: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
🔇 Additional comments (22)
test/generators/plugin.test.ts (3)
1-2: LGTM! Modern Node.js import practices applied.The changes align with modern Node.js practices by using the
node:prefix for built-in modules and updating to the new@eggjs/mockpackage.Also applies to: 6-6
8-8: LGTM! Improved test suite description.The updated description provides better clarity about the test file location.
17-18: LGTM! Updated package imports for egg 4.0.0+.The assertions correctly reflect the new
@eggjs/*package imports, aligning with the PR objectives of supporting egg 4.0.0+.test/generators/utils.ts (1)
1-1: LGTM! Usingnode:prefix for built-in modules.The change to use
node:prefix for built-in modules is a good practice that ensures consistent module resolution in both CJS and ESM environments.Please verify that the Node.js version requirement is properly documented in the package.json:
Also applies to: 3-3
✅ Verification successful
LGTM! The package.json file correctly enforces the Node.js version requirement (>= 18.19.0), and the use of the
node:prefix aligns with the best practices for both CJS and ESM environments.
- Verified the
enginesfield is correctly set in package.json.- The import changes in
test/generators/utils.tsare appropriate.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Node.js version requirement in package.json # Test: Search for engines field in package.json rg '"engines"' -A 5Length of output: 119
src/scripts/eggInfo.ts (2)
5-6: LGTM! Good use ofnode:prefix.The update to use the
node:prefix for built-in modules follows Node.js best practices.
105-105: LGTM! Good addition of configurable egg core package name.The addition of the
eggCorePkgNameparameter with a default value improves flexibility while maintaining backward compatibility.src/register.ts (1)
1-2: LGTM! Good modernization of Node.js imports.The update to use the
node:prefix for built-in modules follows Node.js best practices, improving module resolution reliability and ESM compatibility.Since my knowledge of Node.js best practices might be outdated, please verify the current recommendations:
✅ Verification successful
LGTM – the use of the
node:prefix aligns with modern Node.js best practices.The recent Node.js recommendations clearly endorse using the
node:prefix for built-in modules to improve clarity and prevent conflicts with userland modules. This update insrc/register.tsadheres to these best practices without introducing any functional changes.
- File
src/register.ts: Updated import statements now explicitly import built-in modules using thenode:prefix.🏁 Scripts executed
The following scripts were executed for the analysis:
Web query:
What are the current Node.js best practices for importing built-in modules? Should the node: prefix be used?Length of output: 4309
test/fixtures/real/app.ts (1)
1-2: LGTM! Changes align with modern Node.js practices.The updates follow best practices by:
- Using the
node:prefix for built-in modules- Importing types directly from 'egg' package
Since this is a breaking change, let's verify the Node.js version requirement is documented:
Also applies to: 4-4
✅ Verification successful
Node.js engine requirement documentation verified.
- The Node.js requirement is properly documented in
package.jsonwith"node": ">= 18.19.0".- This confirms that the breaking change has the necessary version requirement specified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Node.js version requirement is documented rg -l "node.*18\.19\.0|nodejs.*18\.19\.0" "*.md" "docs/*.md"Length of output: 157
Script:
#!/bin/bash # Check for documented engine requirements in package.json echo "--- Checking for an 'engines' field in package.json ---" rg -n '"engines":' package.json echo "--- Checking for a Node.js version requirement in package.json ---" rg -n '"node":' package.json echo "--- Searching the entire repository for the version '18.19.0' ---" rg -n "18\.19\.0"Length of output: 588
test/fixtures/real/config/config.default.ts (1)
1-2: LGTM! Type imports improve code clarity.The changes improve the code by:
- Using explicit type imports instead of namespace access
- Using
typeimport modifier for better tree-shakingAlso applies to: 5-5
test/generator.test.ts (1)
1-1: LGTM! Import statement modernized.The change from CommonJS to ESM syntax with
node:prefix aligns with modern Node.js practices.test/generators/config.test.ts (1)
1-2: LGTM! Dependencies and imports modernized.The changes improve the code by:
- Using
node:prefix for built-in modules- Updating to
@eggjs/mockfromegg-mockLet's verify the package dependency is updated in package.json:
Also applies to: 6-6
✅ Verification successful
Verified: Package Dependency Updated Correctly
The package.json includes the "@eggjs/mock" dependency with version "^6.0.5", confirming that the update from egg-mock has been applied as intended. The changes, including the use of the
node:prefix for built-in modules, align with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if @eggjs/mock is listed in package.json jq '.dependencies["@eggjs/mock"] // .devDependencies["@eggjs/mock"]' package.jsonLength of output: 92
test/index.test.ts (2)
1-1: LGTM! Import statements updated to follow Node.js best practices.The change to use the
node:prefix for built-in modules improves clarity and follows Node.js recommended practices.Also applies to: 3-5, 7-7
351-351: Verify the reason for skipping the test.The test "should works without error in real app" has been skipped and the
--tsflag was removed from the egg dev command. This could indicate potential compatibility issues.Could you please clarify why this test was skipped and if there are plans to re-enable it?
Also applies to: 359-359
test/fixtures/real/tsconfig-base.json (1)
7-8: LGTM! Enhanced module interoperability.The addition of
"esModuleInterop": truealigns with the PR objective to support both CommonJS and ESM modules..github/workflows/release.yml (1)
3-6: Verify workflow trigger changes and organization update.The changes include:
- Removal of manual trigger capability (workflow_dispatch)
- Update to use eggjs organization's actions
Please confirm if removing the manual trigger capability is intentional.
Also applies to: 10-10
.github/workflows/nodejs.yml (1)
15-15: LGTM! Node.js version requirements updated.The update to test against Node.js versions 18, 20, and 22 aligns with the PR's requirement of Node.js 18.19.0 or higher.
package.json (6)
26-26: Simplified "test-local" script command.
The removal of the--tsflag in"egg-bin test"streamlines the test command. Verify that TypeScript checking is handled separately and that tests run correctly without the flag.
28-28: Simplified "cov" script command.
Changing"egg-bin cov --ts"to"egg-bin cov"removes redundant flags, which aligns with the overall refactoring for Node.js 18.19.0+ and egg.js v4 compatibility. Ensure that the coverage reports remain correct under the new command.
51-52: Added new devDependencies for enhanced tooling.
The introduction of@eggjs/bin(^7.0.4) and@eggjs/mock(^6.0.5) reflects the project’s move toward updated modules and improved support for both CommonJS and ESM. Please verify that these versions work well with the rest of the project and that any related documentation is updated accordingly.
61-61: Updatedeggdependency to "^4.0.7".
This dependency update ensures compatibility with egg.js versions 4.0.0 and higher as required by the PR. Confirm that all usage of egg-specific APIs in the project align with this version upgrade.
64-64: Updated ESLint configuration dependency to "14".
Raising the version ofeslint-config-egghelps enforce updated linting rules and coding standards. Make sure that the eslint rules still behave as expected and that there are no breaking changes in the linting configuration.
68-68: Updated Node.js engine requirement to ">= 18.19.0".
This change clearly indicates the dropping of support for Node.js versions earlier than 18.19.0. It’s critical to ensure that both the documentation and CI/CD workflows reflect this breaking change so that consumers are aware of the new minimum version requirement.
fengmk2
force-pushed
the
support-egg-v4
branch
from
f9ab4e4 to
5684d18
Compare
BREAKING CHANGE: drop Node.js < 18.19.0 support Only support egg >= 4.0.0 part of eggjs/egg#3644 eggjs/egg#5257
fengmk2
force-pushed
the
support-egg-v4
branch
from
5684d18 to
37e2e6e
Compare
[skip ci] ## [3.0.0](v2.1.1...v3.0.0) (2025-02-04) ### ⚠ BREAKING CHANGES * drop Node.js < 18.19.0 support Only support egg >= 4.0.0 part of eggjs/egg#3644 eggjs/egg#5257 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Introduced a new “Contributors” section in the project documentation to highlight community involvement. - **Chores** - Streamlined CI workflows by removing legacy configurations and updating triggers for improved efficiency. - Upgraded key dependencies and increased the minimum required Node.js version for enhanced performance and security. - **Refactor** - Standardized module import practices across the codebase to align with current Node.js conventions. - Improved asynchronous handling in core operations. - **Tests** - Updated test setups and assertions to reflect the new module import standards and dependency changes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support egg v4 ([#114](#114)) ([90491cf](90491cf))
BREAKING CHANGE: drop Node.js < 18.19.0 support
Only support egg >= 4.0.0
part of eggjs/egg#3644
eggjs/egg#5257
Summary by CodeRabbit