feat: support cjs and esm both by tshy #27
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257
WalkthroughThis pull request introduces several updates across the project's configuration and dependencies. The changes primarily focus on upgrading Node.js and TypeScript versions, updating package dependencies, modifying workflow configurations, and adjusting TypeScript compiler settings. The modifications span multiple files, including GitHub Actions workflows, package configurations, test files, and project-level configuration files. The updates aim to modernize the project's toolchain and align with the latest ecosystem standards. Changes
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Updated and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
![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 (4)
boilerplate/README.md (1)
32-33: Add documentation about CJS/ESM supportWhile the version requirements are correctly updated, consider adding documentation about:
- The dual CJS/ESM support feature
- Any specific configuration needed for each module system
- Migration guide for users upgrading from Node.js 16.x
package.json (1)
24-25: Consider using more specific version rangesUsing major version numbers alone (
"8","14") might lead to unexpected breaking changes. Consider using more specific version ranges (e.g.,"^8.0.0") for better dependency management.boilerplate/config/config.local.ts (1)
1-1: Document the configuration type changes.The removal of
PowerPartialtype affects how developers can define local configurations. This architectural change should be documented in the migration guide, especially since it's part of the transition to support both CJS and ESM.Consider adding a section in the README or migration guide explaining:
- Why PowerPartial was removed
- How to properly type configurations now
- Any implications for local development
Also applies to: 4-4
boilerplate/config/config.default.ts (1)
Line range hint
12-15: Add type definition for bizConfig.The
bizConfigobject should have an explicit type to ensure type safety when spreading into the final config.Consider adding a type:
- const bizConfig = { + const bizConfig: Partial<EggAppConfig> = { sourceUrl: `https://github.com/eggjs/examples/tree/master/${appInfo.name}`, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/PULL_REQUEST_TEMPLATE.md(0 hunks).github/workflows/nodejs.yml(1 hunks).gitignore(1 hunks)README.md(1 hunks)boilerplate/README.md(1 hunks)boilerplate/_.gitignore(1 hunks)boilerplate/_package.json(1 hunks)boilerplate/app/module/foo/package.json(1 hunks)boilerplate/config/config.default.ts(1 hunks)boilerplate/config/config.local.ts(1 hunks)boilerplate/config/config.prod.ts(1 hunks)boilerplate/test/app/module/bar/controller/home.test.ts(1 hunks)boilerplate/test/app/module/bar/controller/user.test.ts(1 hunks)boilerplate/test/app/module/foo/service/HelloService.test.ts(1 hunks)boilerplate/tsconfig.json(1 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- .github/PULL_REQUEST_TEMPLATE.md
✅ Files skipped from review due to trivial changes (5)
- boilerplate/app/module/foo/package.json
- README.md
- boilerplate/test/app/module/bar/controller/user.test.ts
- boilerplate/test/app/module/foo/service/HelloService.test.ts
- boilerplate/_.gitignore
🔇 Additional comments (10)
boilerplate/test/app/module/bar/controller/home.test.ts (2)
Line range hint
4-10: Test implementation is robust and future-proof.The test uses modern Node.js features (node: imports, async/await) while remaining framework-agnostic. This makes it compatible with both CJS and ESM module systems.
2-2: LGTM! Package import path updated correctly.The change from 'egg-mock/bootstrap' to '@eggjs/mock/bootstrap' aligns with the modernization effort and supports the dual CJS/ESM compatibility goal.
Let's verify the package compatibility and availability:
✅ Verification successful
Import path change verified and package compatibility confirmed
The @eggjs/mock package properly supports both ESM and CommonJS through conditional exports, and the bootstrap entry point is correctly configured for dual module system compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify @eggjs/mock package compatibility with both CJS and ESM # Check if package.json exists in node_modules/@eggjs/mock if [ -f "node_modules/@eggjs/mock/package.json" ]; then # Check for "type" field and exports configuration cat node_modules/@eggjs/mock/package.json | jq '{type: .type?, exports: .exports?}' else # If not installed, check npm registry curl -s https://registry.npmjs.org/@eggjs/mock/latest | jq '{type: .type?, exports: .exports?}' fiLength of output: 1021
.gitignore (1)
14-15: Verify: Is ignoring package lock files intentional?Ignoring
package-lock.jsonand.package-lock.jsonfiles is generally not recommended as they ensure consistent dependency installations across different environments. If this is related to the CJS/ESM support, consider documenting the rationale.✅ Verification successful
Ignoring package lock files is part of the boilerplate pattern
The ignore patterns for
package-lock.jsonand.package-lock.jsonare consistent with the boilerplate template inboilerplate/_.gitignore. This appears to be an intentional design choice for new projects created from this boilerplate. However, consider documenting the rationale behind this decision in the project documentation to help users understand the implications for dependency management.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if other egg boilerplates ignore package-lock.json fd -g "*.gitignore" | xargs grep -l "package-lock.json"Length of output: 79
Script:
#!/bin/bash # Check the content of the template gitignore and look for any documentation echo "=== Template .gitignore content ===" cat boilerplate/_.gitignore echo -e "\n=== Looking for related documentation ===" rg -i "package.?lock" -g "!*.gitignore" -g "!package*.json" -A 2 -B 2Length of output: 513
boilerplate/tsconfig.json (1)
4-6: LGTM! TypeScript configuration aligns with ESM supportThe compiler options are well-configured for dual CJS/ESM support:
target: "ES2022"is appropriate for Node.js 18.19.0+moduleandmoduleResolutionset to"NodeNext"enable proper ESM support.github/workflows/nodejs.yml (1)
15-15: LGTM! Node.js versions align with requirementsThe updated Node.js versions (18, 20, 22) properly align with:
- The PR's breaking change of dropping support for Node.js < 18.19.0
- Current Node.js LTS versions
package.json (1)
27-27: LGTM! Added MIT license fieldGood addition of the MIT license field, ensuring proper licensing information is available in the package metadata.
boilerplate/_package.json (2)
38-38: Verify Node.js v22 type compatibility.The upgrade to
@types/nodev22 is quite forward-looking. While it's good to stay updated, we should ensure it doesn't cause issues with the current Node.js engine requirement.Let's check for potential compatibility issues:
Also applies to: 40-41, 43-43
✅ Verification successful
@types/node v22 upgrade is safe to proceed
The upgrade to
@types/nodev22 is safe as it's only used for development, and the codebase shows no usage of Node.js v22 specific features. The TypeScript configuration is version-agnostic and uses stable settings (ES2022, NodeNext).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if any code uses Node.js v22 specific features fd -e ts -e js -x ast-grep --pattern 'import { $_ } from "node:$_"' {}Length of output: 74
Script:
#!/bin/bash # Check package.json for Node.js version requirements cat package.json | jq '.engines' # Look for common Node.js built-in module usage rg -l "require\(['\"](?:fs|path|http|crypto|stream|util|events)" --type ts --type js # Check TypeScript configuration fd tsconfig.json -x cat {}Length of output: 410
27-27: Verify breaking changes in egg v4.0.0.The major version bump of
eggto v4.0.0 and the switch to@eggjs/scriptsalign with the modernization goals. However, we should ensure all breaking changes are addressed.Let's check for potential breaking changes:
Also applies to: 32-33
✅ Verification successful
The codebase is compatible with egg v4 upgrade
The codebase already follows modern egg.js patterns and uses @eggjs/tegg decorators. No breaking changes from v3 to v4 affect the current implementation. Consider adding a MIGRATION.md guide to document the upgrade process for future contributors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for egg.js v3 to v4 specific patterns that might need updates rg -l 'egg-scripts|EggApplication|app.type|agent.type' # Check if migration guide exists test -f MIGRATION.md && echo "Migration guide exists" || echo "Consider adding a migration guide for v4"Length of output: 170
Script:
#!/bin/bash # Check for egg configuration files and patterns fd -e js -e ts -e json config rg -l "config\.(set|get)|plugin\.(enable|disable)" # Check for egg plugin usage rg -l "exports.plugin|exports.middleware|exports.schedule" # Check TypeScript related patterns rg -l "EggAppConfig|EggPlugin|EggLogger"Length of output: 552
Script:
#!/bin/bash # Check configuration files content echo "=== Config Files ===" for f in boilerplate/config/config.*.ts; do echo "=== $f ===" cat "$f" done echo -e "\n=== TypeScript Files ===" echo "=== HelloService.ts ===" cat "boilerplate/app/module/foo/service/HelloService.ts" echo -e "\n=== home.ts ===" cat "boilerplate/app/module/bar/controller/home.ts"Length of output: 2569
boilerplate/config/config.prod.ts (1)
1-1: Verify the removal of PowerPartial type.The change from
PowerPartial<EggAppConfig>to directEggAppConfigtype assertion means all configuration properties must be fully defined. This could be a breaking change if any code relies on partial configuration objects.Let's verify the impact:
Also applies to: 4-4
✅ Verification successful
PowerPartial removal is consistent with codebase patterns
The removal of PowerPartial is safe as all config files consistently use direct EggAppConfig assertions and fully defined config objects. No partial config patterns were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any partial config usage patterns rg -l "PowerPartial.*EggAppConfig" . # Check for any config properties that might be undefined ast-grep --pattern 'config.$_ = undefined'Length of output: 83
Script:
#!/bin/bash # Search for config type patterns and usage rg -l "type.*Config.*=.*{" . rg -l "interface.*Config.*{" . # Look for config object spread/partial assignments ast-grep --pattern 'const config = { $$$ }' ast-grep --pattern 'export default { $$$ } as' # Check config-related files fd -e ts -e js configLength of output: 492
Script:
#!/bin/bash # Check content of all config files echo "=== config.default.ts ===" cat boilerplate/config/config.default.ts echo -e "\n=== config.local.ts ===" cat boilerplate/config/config.local.ts echo -e "\n=== config.prod.ts ===" cat boilerplate/config/config.prod.ts # Search with fixed regex patterns rg "type.*Config" || true rg "interface.*Config" || trueLength of output: 1204
boilerplate/config/config.default.ts (1)
Line range hint
18-21: Ensure type safety for spread operations.When spreading objects into the final config, ensure all properties align with
EggAppConfig. Consider using type assertions or validation to prevent runtime issues.Let's verify the spread operation safety:
✅ Verification successful
Spread operations are properly type-safe
The implementation already ensures type safety through:
- Explicit
EggAppConfigtyping for the config object- Simple extension through
bizConfigwith a single valid property- Proper type imports from the egg framework
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other spread operations in configs rg "\\.\\.\\." . -A 2 -B 2 # Look for potential type mismatches in config properties ast-grep --pattern 'config.$_ = $_'Length of output: 1864
Script:
#!/bin/bash # Check for type definitions and imports ast-grep --pattern 'import { $_ } from "egg"' # Look for type annotations and interfaces ast-grep --pattern 'export interface $_Config extends $_' # Find bizConfig definition rg "bizConfig" -A 2 -B 2 # Check for PowerPartial usage ast-grep --pattern 'PowerPartial<$_>'Length of output: 742
Script:
#!/bin/bash # Get the complete content of config.default.ts cat boilerplate/config/config.default.ts # Check for any type-related imports or definitions rg "type|interface|import" boilerplate/config/config.default.tsLength of output: 759
| "typescript": "5" | ||
| }, | ||
| "engines": { | ||
| "node": ">=20.10.0" | ||
| "node": ">=20.18.1" |
There was a problem hiding this comment.
Adjust Node.js version requirement.
The current requirement of "node": ">=20.18.1" is more restrictive than necessary. According to the PR objectives, we only need to drop support for Node.js versions below 18.19.0.
Consider updating the engine requirement:
- "node": ">=20.18.1"
+ "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.
| "node": ">=20.18.1" | |
| "node": ">=18.19.0" |
| "start": "eggctl start --daemon --title=egg-server-{{name}}", | ||
| "stop": "eggctl stop --title=egg-server-{{name}}", |
There was a problem hiding this comment.
💡 Codebase verification
Documentation and dependency missing for eggctl commands
The migration from egg-scripts to eggctl requires proper documentation and dependency specification:
- Add documentation for eggctl usage in README.md or relevant documentation
- Specify eggctl as a dependency in package.json
- Include migration instructions for users upgrading from egg-scripts
🔗 Analysis chain
Scripts updated to use modern egg.js tooling.
The migration from egg-scripts to eggctl and the enhanced script structure with proper pre/post hooks looks good. The clean script is well integrated into the workflow.
Let's verify the eggctl commands are properly documented:
Also applies to: 13-18, 21-22
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if eggctl commands are documented in the project
rg -i "eggctl" README.md || echo "eggctl commands not found in README.md"
Length of output: 111
Script:
#!/bin/bash
# Check for documentation files and eggctl references
fd -e md | xargs rg -i "eggctl" || echo "No eggctl documentation found in markdown files"
# Check if eggctl is listed in dependencies
cat boilerplate/_package.json | grep -i "eggctl"
Length of output: 306
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
eggjs/egg#5257
Summary by CodeRabbit
Release Notes
Project Configuration
Dependencies
Documentation
Maintenance
.gitignoreto excludepackage-lock.json