feat: support cjs and esm both by tshy #20
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 22 minutes and 51 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 (1)
WalkthroughThe pull request introduces a comprehensive overhaul of the Changes
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: 4
🧹 Nitpick comments (9)
src/app/extend/application.ts (1)
34-34: Typo in log message: 'overwrite' should be 'overwritten'The log message on line 34 should read 'will be overwritten' instead of 'will be overwrite'.
Apply this diff to fix the typo:
- this.coreLogger.warn('[@eggjs/session] sessionStore already exists and will be overwrite'); + this.coreLogger.warn('[@eggjs/session] sessionStore already exists and will be overwritten');test/fixtures/samesite-none-session/app/controller/home.js (1)
1-3: Consider migrating test fixtures to TypeScript.While the async/await changes are good, consider migrating test fixtures to TypeScript to maintain consistency with the project's TypeScript migration. This would provide better type safety and IDE support.
The async function changes look good and align with modern JavaScript practices.
Also applies to: 5-8
src/config/config.default.ts (1)
10-17: Consider documenting security implications of configuration options.While the default values are reasonable, it would be helpful to add JSDoc comments explaining:
- The security implications of
httpOnlyandencrypt- The impact of
maxAgeon session lifetime- The purpose of
logValueand when to disable ittest/fixtures/memory-session-generator/app/controller/home.js (1)
1-23: Consider removing unnecessary async keywords.Since none of these functions perform any asynchronous operations, the
asynckeyword is unnecessary and adds overhead. Consider removing it from all the functions unless there are plans to add async operations in the future.-exports.get = async function(ctx) { +exports.get = function(ctx) { ctx.body = ctx.session; }; -exports.set = async function(ctx) { +exports.set = function(ctx) { ctx.session = ctx.query; ctx.body = ctx.session; }; -exports.setKey = async function(ctx) { +exports.setKey = function(ctx) { ctx.session.key = ctx.query.key; ctx.body = ctx.session; }; -exports.remove = async function(ctx) { +exports.remove = function(ctx) { ctx.session = null; ctx.body = ctx.session; }; -exports.maxAge = async function(ctx) { +exports.maxAge = function(ctx) { ctx.session.maxAge = Number(ctx.query.maxAge); ctx.body = ctx.session; };src/app.ts (2)
15-17: Enhance security warning message for httpOnly.The warning message could be more specific about the security implications. Consider adding details about potential XSS attacks and session hijacking risks.
- app.coreLogger.warn('[@eggjs/session]: please set `config.session.httpOnly` to true. It is very dangerous if session can read by client JavaScript.'); + app.coreLogger.warn('[@eggjs/session]: please set `config.session.httpOnly` to true. Setting this to false exposes sessions to XSS attacks and potential session hijacking through client-side JavaScript.');
20-28: Improve type safety and structure of event logging.The event handlers could benefit from:
- Type-safe event names using string literals
- Structured logging format for better parsing
+ type SessionEventName = 'session:missed' | 'session:expired' | 'session:invalid'; + interface SessionEventPayload { + ctx: Context; + key: string; + value?: unknown; + } + // listen on session's events - app.on('session:missed', ({ ctx, key }) => { - ctx.coreLogger.warn('[session][missed] key(%s)', key); + app.on<SessionEventName>('session:missed', ({ ctx, key }: SessionEventPayload) => { + ctx.coreLogger.warn({ event: 'session:missed', key }); });test/app/middleware/session.test.ts (1)
144-144: Improve test reliability by using constants for wait durations.The hardcoded wait durations (200ms, 1000ms) could make tests flaky. Consider:
- Using named constants for better maintainability
- Implementing a more reliable way to detect session expiration
+const TEST_DELAYS = { + SHORT_EXPIRY: 200, + LONG_EXPIRY: 1000, +} as const; -await scheduler.wait(200); +await scheduler.wait(TEST_DELAYS.SHORT_EXPIRY);Also applies to: 166-166, 174-174, 192-192
README.md (1)
26-26: Add peer dependency information to installation instructionsSince the package now requires Node.js ≥ 18.19.0, this should be clearly documented in the installation section.
Add peer dependency information:
npm i @eggjs/session + +## Prerequisites + +- Node.js >= 18.19.0test/fixtures/cookie-session/app/controller/home.js (1)
1-22: Consider implementing a shared test utility.Given the identical structure across multiple test fixtures, consider implementing a shared test utility that these controllers can extend. This would:
- Reduce code duplication
- Ensure consistent behavior across test scenarios
- Make maintenance easier
Would you like me to propose a design for this shared test utility?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.eslintignore(1 hunks).eslintrc(1 hunks).github/workflows/nodejs.yml(1 hunks).github/workflows/pkg.pr.new.yml(1 hunks).gitignore(1 hunks)README.md(2 hunks)__snapshots__/session.test.ts.js(1 hunks)app.js(0 hunks)app/extend/application.js(0 hunks)app/middleware/session.js(0 hunks)config/config.default.js(0 hunks)package.json(1 hunks)src/app.ts(1 hunks)src/app/extend/application.ts(1 hunks)src/app/middleware/session.ts(1 hunks)src/config/config.default.ts(1 hunks)src/index.ts(1 hunks)src/types.ts(1 hunks)src/typings/index.d.ts(1 hunks)test/app/middleware/session.test.ts(11 hunks)test/fixtures/chips/app/controller/home.js(0 hunks)test/fixtures/chips/app/router.js(0 hunks)test/fixtures/cookie-session/app/controller/home.js(1 hunks)test/fixtures/httponly-false-session/app/controller/home.js(1 hunks)test/fixtures/logValue-false-session/app/controller/home.js(1 hunks)test/fixtures/memory-session-generator/app.js(1 hunks)test/fixtures/memory-session-generator/app/controller/home.js(1 hunks)test/fixtures/memory-session/app.js(1 hunks)test/fixtures/memory-session/app/router.js(0 hunks)test/fixtures/redis-session/app.js(1 hunks)test/fixtures/redis-session/app/controller/home.js(1 hunks)test/fixtures/redis-session/app/router.js(0 hunks)test/fixtures/samesite-none-session/app/controller/home.js(1 hunks)test/fixtures/session-maxage-session/app/controller/home.js(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (8)
- test/fixtures/chips/app/router.js
- test/fixtures/memory-session/app/router.js
- test/fixtures/redis-session/app/router.js
- test/fixtures/chips/app/controller/home.js
- app/extend/application.js
- app/middleware/session.js
- config/config.default.js
- app.js
✅ Files skipped from review due to trivial changes (5)
- tsconfig.json
- .eslintignore
- src/index.ts
- .gitignore
- src/typings/index.d.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18.19.0)
- GitHub Check: Node.js / Test (macos-latest, 20)
🔇 Additional comments (19)
test/fixtures/memory-session/app.js (1)
3-23: Correct implementation ofAppBootHookfor session store setupThe
AppBootHookclass properly initializes thesessionStorein thewillReadylifecycle method. This ensures that the session store is set up before the application is ready, following the appropriate lifecycle hook usage.src/app/extend/application.ts (1)
32-51: Verify modifyingthis.config.session.storeat runtimeThe setter for
sessionStoremodifiesthis.config.session.storeat runtime. Typically, configuration objects in Egg.js are immutable during runtime. Please verify that changingthis.config.session.storedynamically will not cause unexpected behavior in the application.src/app/middleware/session.ts (1)
1-3: Correctly exporting session middlewareThe
createSessionfunction fromkoa-sessionis correctly imported and exported as the default export. This allows the middleware to be integrated seamlessly into the application.src/types.ts (1)
1-8: Verify package exports configuration for dual CJS/ESM support.The relative import with
.jsextension might cause issues:
- TypeScript typically doesn't require
.jsextensions in imports- For dual CJS/ESM support, package exports need careful configuration
Let's verify the package.json exports configuration:
✅ Verification successful
The
.jsextension in imports is correct and follows best practicesThe current implementation follows the recommended TypeScript practices:
- Using
NodeNextmodule resolution which requires.jsextensions- Consistent usage across the codebase
- Proper package exports configuration for dual CJS/ESM support
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check package.json for proper exports configuration cat package.json | jq '.exports' # Verify if config.default.js exists in both CJS and ESM outputs fd -e js 'config.default.js$'Length of output: 850
Script:
#!/bin/bash # Find config.default.ts source file fd -e ts 'config.default.ts$' # Check tsconfig for module settings cat tsconfig.json | jq '.compilerOptions.module,.compilerOptions.moduleResolution' # Verify if there are any other imports using .js extension rg "from.*\.js[\"']" --type tsLength of output: 505
__snapshots__/session.test.ts.js (1)
1-12: Document session configuration and consider snapshot format.The session configuration looks good with secure defaults:
httpOnly: truefor XSS protectionsigned: truefor tampering protectionmaxAge: 86400000(24 hours) is reasonableHowever:
- The empty
store: {}might need documentation- The snapshot uses CJS exports despite having
.tsin the filenameLet's check if this is a Jest snapshot convention:
src/config/config.default.ts (1)
4-8: LGTM! Good use of zod for runtime type safety.The schema extends
SessionOptionsand adds type-safe configuration with runtime validation. Exporting both the schema and inferred type is a good practice for TypeScript integration.test/fixtures/redis-session/app/controller/home.js (1)
1-23: Same feedback as memory-session-generator controller.The async keywords are unnecessary here as well since no asynchronous operations are performed.
test/fixtures/logValue-false-session/app/controller/home.js (1)
1-23: Same feedback as previous controllers.The async keywords are unnecessary here as well since no asynchronous operations are performed.
test/fixtures/redis-session/app.js (1)
7-20: LGTM! Proper async/await implementation with Redis operations.The conversion from generator functions to async/await is well implemented:
- All Redis operations are properly awaited
- Error cases are handled (null check in get)
- JSON serialization is done at the right time (before Redis operations)
.eslintrc (1)
2-5: LGTM! Good ESLint configuration for TypeScript.The ESLint configuration properly extends TypeScript-specific rules and enforces node prefix conventions.
.github/workflows/nodejs.yml (1)
12-14: LGTM! Node.js version requirements aligned with PR objectives.The CI configuration correctly enforces the new minimum Node.js version requirement of 18.19.0.
Let's verify that all package.json files in the repository are consistent with this Node.js version requirement:
✅ Verification successful
Node.js version requirements are consistently defined across the repository ✅
The main package.json correctly specifies
>= 18.19.0as the minimum Node.js version requirement, which aligns perfectly with the CI workflow testing against versions 18.19.0, 20, and 22. Test fixture package.json files don't need explicit version requirements as they inherit from the root configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all package.json files and check their Node.js version requirements fd package.json --type f --exec sh -c 'echo "Checking {}" && cat {} | jq -r ".engines.node // empty"'Length of output: 646
package.json (4)
72-84: LGTM: Proper dual module configurationThe exports field is correctly configured for dual CJS/ESM support:
- Provides separate entry points for import/require
- Includes proper type definitions
- Maintains backward compatibility
66-71: LGTM: Proper tshy configurationThe tshy configuration correctly specifies the entry points for building both CJS and ESM formats.
32-34: Verify compatibility with updated dependenciesMajor version updates that need verification:
- koa-session: ^6.0.0 → ^7.0.2
- Added new dependency: zod ^3.24.1
Let's check for breaking changes:
✅ Verification successful
Dependencies update is compatible ✅
- Node.js version requirement (>= 18.19.0) matches koa-session v7
- zod is correctly integrated for session configuration validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check changelogs for breaking changes gh api graphql -f query=' { repository(owner: "koajs", name: "session") { releases(first: 1, orderBy: {field: CREATED_AT, direction: DESC}) { nodes { tagName description } } } }'Length of output: 734
Script:
#!/bin/bash # Check koa-session v7.0.0 release notes gh api graphql -f query=' { repository(owner: "koajs", name: "session") { release(tagName: "v7.0.0") { description } } } ' # Search for zod usage rg -l "zod" --type ts --type jsLength of output: 736
Script:
#!/bin/bash # Check Node.js version in package.json cat package.json | jq .engines # Check zod usage cat src/config/config.default.tsLength of output: 483
28-30: Verify Node.js version compatibilityThe engine requirement aligns with the PR objective to drop support for Node.js versions earlier than 18.19.0.
Let's verify this doesn't conflict with dependencies:
✅ Verification successful
Node.js version requirement is appropriate ✅
The Node.js engine requirement of
>= 18.19.0is well-aligned with:
- The modern dependency versions being used
- The development tooling requirements
- The @eggjs ecosystem's latest major versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if any dependencies have conflicting Node.js version requirements npx ls-enginesLength of output: 110
Script:
#!/bin/bash # Get all dependencies and their engine requirements echo "Main package.json dependencies:" jq -r '.dependencies, .devDependencies | to_entries[] | .key + "@" + .value' package.json echo -e "\nSearching for Node.js engine requirements in the codebase:" rg -g "package.json" '"engines":\s*{[^}]*"node":[^}]*}' -A 1Length of output: 794
Script:
#!/bin/bash # Search for Node.js engine requirements with fixed regex echo "Searching for Node.js engine requirements:" rg -g "package.json" '"engines"' -A 3Length of output: 249
README.md (1)
8-10: LGTM: Improved repository status visibilityAdded helpful badges:
- Node.js version compatibility
- PR welcome status
- CodeRabbit PR reviews
test/fixtures/session-maxage-session/app/controller/home.js (1)
1-3: Consider the necessity of async functions.While the conversion from generator functions to async functions is syntactically correct, none of these methods contain any
awaitoperations. Consider whether these methods truly need to be asynchronous.Run this script to check if there are any async operations in the session middleware that these controllers interact with:
Also applies to: 5-7, 10-12, 15-17, 20-22
test/fixtures/httponly-false-session/app/controller/home.js (1)
1-22: LGTM! Well-structured implementation.The code correctly implements session handling with proper type conversion for maxAge values.
test/fixtures/cookie-session/app/controller/home.js (1)
1-22: LGTM! Verify dual module system compatibility.The implementation is correct and consistent with other fixtures.
Given the PR's objective to support both CJS and ESM, let's verify the module system compatibility:
✅ Verification successful
Module system compatibility verified successfully
The implementation maintains proper separation between ESM (source code) and CJS (test fixtures) with correct dual module system support configured in package.json.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential module system compatibility issues # Look for mixed usage of require/import or module.exports/export rg -l "require|import|exports|module\.exports" | grep -v "node_modules" | xargs rg "require|import|exports|module\.exports"Length of output: 7795
[skip ci] ## [4.0.0](v3.3.0...v4.0.0) (2025-01-19) ### ⚠ 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 ## Release Notes - **Package Upgrade** - Renamed package from `egg-session` to `@eggjs/session` - Updated Node.js compatibility to version 18.19.0+ - **New Features** - Enhanced session configuration with improved type safety - Added support for more granular session management - Improved logging and security configurations - **Breaking Changes** - Dropped support for Node.js versions below 18.19.0 - Migrated from generator functions to async/await syntax - Updated session middleware and configuration structure - **Performance** - Updated dependencies, including `koa-session` to version 7.0.2 - Optimized session store handling - **Security** - Strengthened default session configurations - Added warnings for potential security risks in session settings <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#20](#20)) ([b1a96e5](b1a96e5))
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #20 +/- ##
============================================
+ Coverage 89.65% 100.00% +10.34%
============================================
Files 4 4
Lines 87 109 +22
Branches 14 16 +2
============================================
+ Hits 78 109 +31
+ Misses 9 0 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
eggjs/egg#5257
Summary by CodeRabbit
Release Notes
Package Upgrade
egg-sessionto@eggjs/sessionNew Features
Breaking Changes
Performance
koa-sessionto version 7.0.2Security