feat: support cjs and esm both by tshy #44
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (11)
WalkthroughThis pull request introduces a comprehensive update to the Egg.js Redis plugin, transitioning it from Changes
Sequence DiagramsequenceDiagram
participant App as Egg.js Application
participant RedisBoot as Redis Boot
participant RedisClient as Redis Client
App->>RedisBoot: Initialize Redis
RedisBoot->>RedisClient: Create Client
RedisClient-->>RedisBoot: Client Created
RedisBoot-->>App: Redis Singleton Ready
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (9)
test/fixtures/apps/ts/redisapp-ts/app/controller/home.ts (2)
16-20: Add error handling for Redis operations.The Redis operations should include proper error handling to demonstrate robust integration testing. Also, consider using constants for Redis keys.
Consider this improvement:
async index() { const { ctx, app } = this; const redis = app.redis; - await redis.set('foo', 'bar'); - const cacheValue = await redis.get('foo'); - ctx.body = cacheValue; + const TEST_KEY = 'foo'; + try { + await redis.set(TEST_KEY, 'bar'); + const cacheValue = await redis.get(TEST_KEY); + ctx.body = cacheValue; + } catch (error) { + ctx.status = 500; + ctx.body = { error: 'Redis operation failed' }; + } }
13-21: Enhance test coverage for ESM/CJS compatibility.Since this PR aims to support both CJS and ESM, consider expanding the test to cover more Redis operations and demonstrate module interoperability.
Would you like me to suggest additional test cases that specifically validate ESM/CJS compatibility?
example/hello/run/agent_config.json (1)
158-158: Review multipart temporary directory configuration.The multipart configuration uses a hardcoded temporary directory path and includes a cleanup schedule. Consider:
- Using environment variables for the temporary directory path
- Documenting the cleanup schedule in comments
Apply this diff to improve the configuration:
- "tmpdir": "/var/folders/g8/c087s6rn3n16q32vnxbs47lh0000gn/T/egg-multipart-tmp/hello-redis", + "tmpdir": "${env.TEMP_DIR}/egg-multipart-tmp/hello-redis", "cleanSchedule": { + // Runs daily at 4:30 AM "cron": "0 30 4 * * *", "disable": false }Also applies to: 159-159, 160-160, 161-161, 162-162
example/hello/app/router.ts (1)
6-11: Consider moving Redis operations to a service layer.Direct Redis operations in route handlers can lead to:
- Reduced code reusability
- Harder testing
- Mixed responsibilities
Consider creating a dedicated service to handle Redis operations.
export default (app: Application) => { const { router } = app; - router.get('/', async ctx => { - const redis = app.redis; - await redis.set('foo', 'bar'); - const cacheValue = await redis.get('foo'); - ctx.body = cacheValue; - }); + router.get('/', app.controller.home.index); };And in a new service file:
// app/service/cache.ts import { Service } from 'egg'; export default class CacheService extends Service { async setValue(key: string, value: string) { await this.app.redis.set(key, value); } async getValue(key: string) { return await this.app.redis.get(key); } }example/hello/start.ts (1)
12-12: Enhance the startup log message.Consider adding more information to the startup log such as environment, Node.js version, and application name.
-console.log(`Server started at http://localhost:${app.config.cluster.listen.port}`); +console.log(` + App: ${app.name} + Environment: ${app.config.env} + Node.js: ${process.version} + URL: http://localhost:${app.config.cluster.listen.port} +`);test/fixtures/apps/ts-multi/redisapp-ts/app/controller/home.ts (1)
18-21: Consider error handling for Redis operations.The Redis operations lack error handling which could lead to unhandled exceptions.
Consider wrapping Redis operations in try-catch:
+ try { await redis.set('foo', 'bar'); const redis2 = app.redis.getSingletonInstance('cache'); await redis2.set('foo2', 'bar2'); + } catch (error) { + ctx.logger.error('Redis operation failed:', error); + ctx.status = 500; + ctx.body = 'Internal Server Error'; + return; + }src/config/config.default.ts (1)
67-73: Improve example configuration types.The example configurations use string literals for numeric values like 'port', which could be misleading.
Consider updating the examples to use proper types:
- port: 'port', + port: 6379,Also applies to: 76-91, 94-109
test/redis.test.ts (1)
89-92: Consider using a helper function for TypeScript compilation.The TypeScript compilation logic is duplicated across test suites and could be extracted into a helper function.
Consider creating a helper:
async function compileTypeScript(destPath: string) { const compilerPath = path.resolve('./node_modules/typescript/bin/tsc'); compile.execSync(`node ${compilerPath} -p ${destPath}`, { cwd: destPath, stdio: 'inherit', }); } async function cleanupTypeScript() { const compilerPath = path.resolve('./node_modules/typescript/bin/tsc'); compile.execSync(`node ${compilerPath} --build --clean`); }Also applies to: 99-101
README.md (1)
122-123: Fix grammatical issues in the documentation.The sentence has grammatical issues:
- "other version" should be "another version" or "other versions"
- Redundant mention of both "iovalkey" and "ioredis"
-`@eggjs/redis` using `ioredis@5` now, if you want to use other version of iovalkey or ioredis, +`@eggjs/redis` uses `ioredis@5`. If you want to use another version of ioredis,🧰 Tools
🪛 LanguageTool
[style] ~122-~122: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...js/redisusingioredis@5` now, if you want to use other version of iovalkey or ioredi...(REP_WANT_TO_VB)
[grammar] ~122-~122: Use the plural noun, or add a word such as ‘the’ or ‘some’ in front of ‘other’.
Context: ...ingioredis@5now, if you want to use other version of iovalkey or ioredis, you can pass th...(OTHER_NN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
.eslintignore(1 hunks).eslintrc(1 hunks).github/workflows/nodejs.yml(1 hunks).gitignore(1 hunks)README.md(6 hunks)agent.js(0 hunks)app.js(0 hunks)config/config.default.js(0 hunks)example/hello/app/router.ts(1 hunks)example/hello/config/config.default.ts(1 hunks)example/hello/config/plugin.ts(1 hunks)example/hello/package.json(1 hunks)example/hello/run/agent_config.json(1 hunks)example/hello/run/agent_config_meta.json(1 hunks)example/hello/run/agent_timing_95228.json(1 hunks)example/hello/run/application_config.json(1 hunks)example/hello/run/application_config_meta.json(1 hunks)example/hello/run/application_timing_95243.json(1 hunks)example/hello/run/router.json(1 hunks)example/hello/start.ts(1 hunks)example/hello/tsconfig.json(1 hunks)example/hello/typings/index.d.ts(1 hunks)index.d.ts(0 hunks)lib/redis.js(0 hunks)package.json(2 hunks)src/agent.ts(1 hunks)src/app.ts(1 hunks)src/config/config.default.ts(1 hunks)src/index.ts(1 hunks)src/lib/redis.ts(1 hunks)src/types.ts(1 hunks)src/typings/index.d.ts(1 hunks)test/fixtures/apps/redisapp-customize/app/controller/home.js(1 hunks)test/fixtures/apps/redisapp-disable-offline-queue/app/controller/home.js(1 hunks)test/fixtures/apps/redisapp-supportTimeCommand-false/app/controller/home.js(1 hunks)test/fixtures/apps/redisapp-weakdependent/app/controller/home.js(1 hunks)test/fixtures/apps/redisapp-weakdependent/config/config.js(0 hunks)test/fixtures/apps/redisapp/app/controller/home.js(1 hunks)test/fixtures/apps/redisclusterapp/app/controller/home.js(1 hunks)test/fixtures/apps/redispathapp/app/controller/home.js(1 hunks)test/fixtures/apps/redissentinelapp/app/controller/home.js(1 hunks)test/fixtures/apps/ts-multi/redisapp-ts/app/controller/home.ts(1 hunks)test/fixtures/apps/ts-multi/tsconfig.json(1 hunks)test/fixtures/apps/ts/redisapp-ts/app/controller/home.ts(1 hunks)test/fixtures/apps/ts/redisapp-ts/tsconfig.json(1 hunks)test/fixtures/apps/ts/tsconfig.json(0 hunks)test/redis.test.ts(10 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (7)
- test/fixtures/apps/redisapp-weakdependent/config/config.js
- test/fixtures/apps/ts/tsconfig.json
- config/config.default.js
- app.js
- agent.js
- index.d.ts
- lib/redis.js
✅ Files skipped from review due to trivial changes (11)
- src/index.ts
- example/hello/config/config.default.ts
- example/hello/package.json
- example/hello/typings/index.d.ts
- src/app.ts
- example/hello/tsconfig.json
- src/typings/index.d.ts
- example/hello/run/application_timing_95243.json
- src/agent.ts
- example/hello/config/plugin.ts
- .eslintignore
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 2764 characters long)
Context: ...s [![NPM version][npm-image]][npm-url] 
[![Test coverage][codecov-image]][codecov-url] [![Known Vulnerabilities][snyk-image]][snyk-url] [![npm download][download-image]][download-url] 
(EN_EXCESSIVE_EXCLAMATION)
[style] ~122-~122: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...js/redisusingioredis@5` now, if you want to use other version of iovalkey or ioredi...
(REP_WANT_TO_VB)
[grammar] ~122-~122: Use the plural noun, or add a word such as ‘the’ or ‘some’ in front of ‘other’.
Context: ...ing ioredis@5 now, if you want to use other version of iovalkey or ioredis, you can pass th...
(OTHER_NN)
🔇 Additional comments (34)
test/fixtures/apps/ts/redisapp-ts/app/controller/home.ts (2)
9-11: LGTM! Well-typed Redis integration.The interface extension correctly combines Redis with Singleton type, providing proper typing for the redis property.
1-2: Consider adding type version constraints.Since this is a TypeScript file in a test fixture demonstrating the new ESM/CJS compatibility, ensure that the
@types/ioredispackage is properly specified in the project's dependencies with a version that supports both module systems.Run this script to check the types package version:
example/hello/run/agent_config.json (2)
207-208: Verify package type configuration aligns with dual CJS/ESM support.The package is configured as ESM-only (
"type": "module"), but according to the PR objectives, it should support both CJS and ESM.Let's verify the package configuration in other files:
✅ Verification successful
Package type configuration is correct for dual format support
The "type": "module" in example/hello/run/agent_config.json is correct as:
- The main package (@eggjs/redis) properly supports both CJS and ESM formats through its exports configuration
- The example app is intentionally ESM-only as a modern demo application
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check package.json and tsconfig.json for dual module support echo "Checking package.json configurations..." fd -e json -E "**/node_modules/**" -x sh -c 'echo "=== {} ==="; cat {}'Length of output: 90192
448-456: Review Redis plugin configuration.The Redis plugin is enabled but uses a relative path (
../../../../..) which could be problematic:
- Path traversal might break in different environments
- No version specified for tracking updates
Let's verify the plugin configuration:
test/fixtures/apps/redisapp/app/controller/home.js (1)
3-6: LGTM: Updated to async/await successfullyThe transition from generator functions to
async/awaitis correctly implemented, improving readability and aligning with modern JavaScript practices.test/fixtures/apps/redisapp-disable-offline-queue/app/controller/home.js (1)
3-6: LGTM: Updated to async/await successfullyThe update from generator functions to
async/awaitis properly done, enhancing code clarity and conforming to current standards.test/fixtures/apps/redisapp-customize/app/controller/home.js (1)
3-6: LGTM! Clean transition to modern async/await syntax.The conversion from generator function to async/await improves code readability while maintaining the same functionality. This change aligns well with the PR's objective of modernizing the codebase and dropping support for older Node.js versions.
test/fixtures/apps/redispathapp/app/controller/home.js (1)
3-6: LGTM! Consistent with the modernization pattern.The changes mirror the same clean transition to async/await syntax seen in other test fixtures, maintaining consistency across the codebase.
test/fixtures/apps/redissentinelapp/app/controller/home.js (1)
3-6: LGTM! Redis Sentinel configuration works with async/await.The transition to async/await is correctly implemented for Redis Sentinel configuration, demonstrating that the modernization works across different Redis setups.
test/fixtures/apps/redisclusterapp/app/controller/home.js (1)
3-6: LGTM! Redis Cluster configuration works with async/await.The transition to async/await is correctly implemented for Redis Cluster configuration, further validating that the syntax modernization works across various Redis deployment models.
test/fixtures/apps/redisapp-weakdependent/app/controller/home.js (1)
3-7: LGTM! Good modernization of async code.The transition from generator functions to async/await improves code readability and aligns with modern JavaScript practices. This change is consistent with dropping support for older Node.js versions.
test/fixtures/apps/redisapp-supportTimeCommand-false/app/controller/home.js (1)
3-7: Identical changes as in other test fixtures.The async/await modernization is consistent with the changes in other test fixture files.
src/types.ts (2)
1-3: LGTM! Clean type imports.The imports are well-organized and properly typed, importing only the necessary types from each package.
5-14: LGTM! Well-structured module augmentation.The module augmentation for
@eggjs/coreis clean and properly typed:
- Extends
EggAppConfigwith Redis configuration- Extends
EggCorewith Redis singleton supporttest/redis.test.ts (1)
139-139: Consider implementing skipped tests.There are two skipped test suites with TODOs that should be addressed:
- Redis sentinel tests
- Redis path tests
Let's check if these tests can be implemented:
Also applies to: 178-178
.eslintrc (1)
2-5: LGTM! ESLint configuration properly set up for TypeScript.The modular ESLint configuration correctly extends TypeScript-specific rules and enforces node prefix conventions.
Let's verify the TypeScript setup is complete:
✅ Verification successful
Node prefix exceptions are justified, TypeScript setup is correct
The ESLint configuration is properly set up, and the apparent node prefix violations are acceptable:
- Internal module-scoped counter and helper function don't require the prefix
- All exported entities (RedisBoot) follow the convention
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for essential TypeScript dependencies grep -E "typescript|@types|ts-node" package.json || echo "Missing TypeScript dependencies" # Verify all TypeScript files follow the node prefix convention rg -t ts "^(class|function|const|let|var|interface|type)" -g "!test/" | grep -v "^node"Length of output: 572
Script:
#!/bin/bash # Check if createClient is exported and if count is module-scoped ast-grep --pattern 'export function createClient' src/lib/redis.ts ast-grep --pattern 'export let count' src/lib/redis.tsLength of output: 125
Script:
#!/bin/bash # View complete redis.ts file to understand module structure cat src/lib/redis.tsLength of output: 3735
.gitignore (1)
10-11: Reconsider ignoring package lock files.Package lock files (
package-lock.jsonand.package-lock.json) should typically be committed to ensure consistent installations across environments. Consider removing these entries unless there's a specific reason to ignore them.Let's check the project's package manager configuration:
test/fixtures/apps/ts-multi/tsconfig.json (1)
2-8: LGTM! TypeScript configuration properly set up for dual CJS/ESM support.The configuration correctly uses
NodeNextfor module resolution and targets ES2022, which aligns with the Node.js version requirement (>= 18.19.0).Let's verify the Node.js version compatibility:
✅ Verification successful
Node.js version compatibility verified ✓
The TypeScript configuration is fully compatible with the specified Node.js version requirement (>= 18.19.0). ES2022 and NodeNext module resolution are well-supported features in Node.js 18.19.0+.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if package.json specifies the correct Node.js version grep -A 5 '"engines"' package.json || echo "No engines field found in package.json"Length of output: 154
tsconfig.json (1)
1-10: LGTM! Root TypeScript configuration is consistent.The configuration matches the test fixtures and correctly supports both CJS and ESM modules.
Let's verify configuration consistency across all tsconfig files:
✅ Verification successful
TypeScript configuration is perfectly consistent across all project files
All 4 tsconfig.json files in the repository are identical, ensuring uniform TypeScript behavior across the main project, examples, and test fixtures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find and compare all tsconfig.json files fd tsconfig.json | xargs md5sumLength of output: 301
test/fixtures/apps/ts/redisapp-ts/tsconfig.json (1)
1-10: LGTM! TypeScript configuration maintains consistency.The configuration correctly matches the project's TypeScript standards and module resolution strategy.
example/hello/run/agent_timing_95228.json (1)
1-362: LGTM!The timing data is well-structured and provides valuable insights into the application's startup process.
example/hello/run/application_config.json (3)
15-28: Session configuration looks secure!The session configuration follows security best practices:
- HTTP-only cookies
- Signed cookies
- Appropriate session lifetime
- Secure cookie settings
279-297: Logging configuration follows best practices!The logging configuration is well-structured with:
- Appropriate log levels
- Separate log files for different components
- Production-safe defaults
- Disabled debug logging in production
309-315: Review cluster listening configuration.The empty hostname in the cluster configuration will bind the server to all network interfaces (0.0.0.0), which might not be desired in some environments.
Consider restricting the hostname for better security:
"cluster": { "listen": { "path": "", "port": 7001, - "hostname": "" + "hostname": "127.0.0.1" } }example/hello/run/application_config_meta.json (1)
1-259: Generated configuration mapping file looks good.This is an auto-generated file that correctly maps configuration keys to their source files.
example/hello/run/agent_config_meta.json (1)
1-259: Generated agent configuration mapping file looks good.This is an auto-generated file that correctly maps agent configuration keys to their source files.
example/hello/run/router.json (1)
1-14: LGTM! Route configuration follows RESTful conventions.The route configuration is well-structured with:
- Proper HTTP method declarations (HEAD, GET)
- Clear path definition
- Valid regular expression for path matching
- Middleware configuration
.github/workflows/nodejs.yml (1)
14-14: LGTM! Node.js version matrix aligns with minimum version requirements.The updated version matrix correctly:
- Removes Node.js 16 support
- Adds specific version 18.19.0 to ensure minimum version compatibility
- Maintains support for newer versions (18, 20, 22)
package.json (6)
2-7: LGTM! Package identity and visibility properly configured.The package has been correctly scoped under
@eggjsorganization with public access, which is essential for npm publishing.
8-14: LGTM! Plugin exports configuration supports both ESM and CJS.The
eggPluginconfiguration properly defines export paths for:
- ESM imports (
import)- CommonJS requires (
require)- TypeScript development (
typescript)
34-36: LGTM! Engine requirement aligns with PR objectives.The minimum Node.js version requirement of
>= 18.19.0aligns with the PR's breaking change to drop support for older versions.
56-65: LGTM! Build scripts support dual module format.The scripts configuration properly handles:
- Linting with TypeScript support
- Cleaning build artifacts
- Testing with coverage
- Type checking during publishing
66-92: LGTM! Dual module configuration follows best practices.The configuration properly:
- Sets
"type": "module"for ESM support- Uses
tshyfor dual-format builds- Configures proper entry points for both formats
- Maintains TypeScript declarations
37-55: Verify compatibility of dependency versions.Dependencies have been updated to support dual module formats:
@eggjs/core: ^6.3.0ioredis: ^5.4.2Development dependencies now include modern tooling for dual-format builds.
Run this script to check for any compatibility issues:
✅ Verification successful
Dependencies are correctly configured for dual module format support
The package is properly set up with dual ESM/CJS support:
- Package exports both ESM and CJS formats via conditional exports
- Dependencies are correctly used in both formats throughout the codebase
- Build tooling (tshy) is configured for dual-format compilation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential compatibility issues in dependencies # Check if dependencies support both ESM and CJS npx @arethetypeswrong/cli check @eggjs/[email protected] [email protected] # Check for any known vulnerabilities npm audit --json @eggjs/[email protected] [email protected]Length of output: 799
Script:
#!/bin/bash # Check package.json for module type and examine import patterns cat package.json | jq '.type' # Look for dual format indicators fd -e mjs -e cjs # Check import patterns rg "require\(" -A 1 rg "import .* from" -A 1 # Check for conditional exports in package.json cat package.json | jq '.exports'Length of output: 2799
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44 +/- ##
==========================================
+ Coverage 83.44% 87.08% +3.63%
==========================================
Files 4 4
Lines 145 209 +64
Branches 19 17 -2
==========================================
+ Hits 121 182 +61
- Misses 24 27 +3 ☔ View full report in Codecov by Sentry. |
[skip ci] ## [3.0.0](v2.6.1...v3.0.0) (2025-01-21) ### ⚠ BREAKING CHANGES * drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit Here are the release notes for this update: - **New Features** - Updated Redis plugin to support Valkey and Redis - Added support for Node.js 18.19.0 and newer versions - Enhanced TypeScript configuration and type definitions - **Breaking Changes** - Renamed package from `egg-redis` to `@eggjs/redis` - Migrated from generator functions to async/await syntax - Updated minimum Node.js version requirement to 18.19.0 - **Improvements** - Improved Redis client configuration options - Enhanced module compatibility with ES modules - Updated dependencies and plugin configuration - **Bug Fixes** - Refined Redis connection and initialization process - Improved error handling and logging for Redis connections <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#44](#44)) ([ceadd9d](ceadd9d))
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
eggjs/egg#5257
Summary by CodeRabbit
Here are the release notes for this update:
New Features
Breaking Changes
egg-redisto@eggjs/redisImprovements
Bug Fixes