feat: support cjs and esm both by tshy #12
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257
WalkthroughThis pull request represents a comprehensive modernization and refactoring of the Changes
Sequence DiagramsequenceDiagram
participant Client
participant EggApp as Egg Application
participant JSONPMiddleware as JSONP Middleware
participant SecurityCheck as Security Validator
Client->>EggApp: JSONP Request
EggApp->>JSONPMiddleware: Process Request
JSONPMiddleware->>SecurityCheck: Validate Referrer
SecurityCheck-->>JSONPMiddleware: Validation Result
JSONPMiddleware->>SecurityCheck: Validate CSRF
SecurityCheck-->>JSONPMiddleware: Validation Result
JSONPMiddleware->>EggApp: Generate JSONP Response
EggApp-->>Client: JSONP Response
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (1)
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 (
|
commit: |
|
New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected], npm/[email protected], npm/[email protected], 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: 1
🧹 Nitpick comments (8)
src/app/extend/application.ts (3)
27-29: Simplify conditional checks using optional chainingConsider using optional chaining to simplify the
csrfEnableassignment and improve readability.Apply this diff to refactor:
-const csrfEnable = this.plugins.security && this.plugins.security.enable // security enable - && this.config.security.csrf && this.config.security.csrf.enable !== false // csrf enable - && options.csrf; // jsonp csrf enabled +const csrfEnable = this.plugins.security?.enable // security enable + && this.config.security.csrf?.enable !== false // csrf enable + && options.csrf; // jsonp csrf enabled🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
51-51: Use optional chaining for cleaner function invocationUtilize optional chaining when calling
validateReferrerto simplify the condition and handle cases wherevalidateReferrermight be undefined.Apply this diff:
-if (validateReferrer && validateReferrer(referrer)) return; +if (validateReferrer?.(referrer)) return;🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
34-34: Enhance security warning message for clarityThe current warning message could be more informative. Consider providing additional context or guidance on the potential risks when both CSRF and referrer checks are disabled.
Apply this diff to adjust the warning message:
-this.coreLogger.warn('[@eggjs/jsonp] SECURITY WARNING!! csrf check and referrer check are both closed!'); +this.coreLogger.warn('[@eggjs/jsonp] SECURITY WARNING: Both CSRF and referrer checks are disabled. This may expose the application to security vulnerabilities.');src/error/JSONPForbiddenReferrerError.ts (1)
1-12: LGTM! Consider enhancing error messages.The error class implementation follows TypeScript best practices. Consider enhancing the error message to include the referrer value for better debugging:
constructor(message: string, referrer: string, status: number) { - super(message); + super(`${message} (referrer: ${referrer})`); this.name = this.constructor.name; this.referrer = referrer; this.status = status; Error.captureStackTrace(this, this.constructor); }test/fixtures/jsonp-test/app/router.js (1)
12-17: Enhance error handling with type safety and logging.While the async/await migration is good, the error handling could be improved:
- app.get('/error', async (ctx, next) => { + app.get('/error', async (ctx, next) => { try { await next(); } catch (error) { + // Log the error for debugging + ctx.logger.error(error); + // Ensure error is an Error instance + const err = error instanceof Error ? error : new Error(String(error)); - ctx.createJsonpBody({ msg: error.message }); + ctx.createJsonpBody({ msg: err.message }); } }, app.jsonp(), 'jsonp.error');src/app/extend/context.ts (1)
Line range hint
21-33: Consider improving type safety and documentation.The
createJsonpBodymethod could benefit from:
- A more specific type for the
bodyparameter- Better JSDoc documentation including examples
- createJsonpBody(body: any) { + /** + * Wraps the response body in a JSONP format + * @param {unknown} body - The response body to wrap + * @example + * ```ts + * ctx.createJsonpBody({ data: 'example' }); + * // => callback({ "data": "example" }) + * ``` + */ + createJsonpBody(body: unknown) {src/types.ts (1)
7-24: Consider enhancing type safety for JSONPConfig.The interface is well-defined, but could benefit from:
- More specific types for the whiteList
- Better validation of callback names
export interface JSONPConfig { + /** + * @example + * ```ts + * callback: ['_callback', 'callback'] + * // or + * callback: 'callback' + * ``` + */ callback: string[] | string; + /** + * @minimum 1 + * @maximum 100 + */ limit: number; csrf: boolean; + /** + * List of allowed referrers + * @example + * ```ts + * whiteList: '.example.com' + * // or + * whiteList: /^https:\/\/example\.com/ + * // or + * whiteList: ['.example.com', /^https:\/\/example\.com/] + * ``` + */ whiteList?: string | RegExp | (string | RegExp)[]; }README.md (1)
22-24: Add dual-format module support to Requirements section.The Requirements section should mention the dual-format (ESM/CommonJS) support.
Add this information after the egg version requirement:
## Requirements - egg >= 4.x +- Supports both ESM and CommonJS formats
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.autod.conf.js(0 hunks).eslintrc(1 hunks).github/PULL_REQUEST_TEMPLATE.md(0 hunks).github/workflows/nodejs.yml(1 hunks).github/workflows/pkg.pr.new.yml(1 hunks).github/workflows/release.yml(1 hunks).gitignore(1 hunks).travis.yml(0 hunks)README.md(3 hunks)app/extend/application.js(0 hunks)appveyor.yml(0 hunks)config/config.default.js(0 hunks)lib/private_key.js(0 hunks)package.json(1 hunks)src/app/extend/application.ts(1 hunks)src/app/extend/context.ts(2 hunks)src/config/config.default.ts(1 hunks)src/error/JSONPForbiddenReferrerError.ts(1 hunks)src/index.ts(1 hunks)src/lib/private_key.ts(1 hunks)src/types.ts(1 hunks)src/typings/index.d.ts(1 hunks)test/fixtures/jsonp-test/app/controller/jsonp.js(1 hunks)test/fixtures/jsonp-test/app/router.js(1 hunks)test/fixtures/jsonp-test/config/config.default.js(0 hunks)test/jsonp.test.ts(2 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (8)
- .github/PULL_REQUEST_TEMPLATE.md
- lib/private_key.js
- .travis.yml
- config/config.default.js
- appveyor.yml
- app/extend/application.js
- test/fixtures/jsonp-test/config/config.default.js
- .autod.conf.js
✅ Files skipped from review due to trivial changes (4)
- src/index.ts
- src/lib/private_key.ts
- src/typings/index.d.ts
- tsconfig.json
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 5 exclamation marks for a text that’s 1479 characters long)
Context: ...p [![NPM version][npm-image]][npm-url] 
[![Test coverage][codecov-image]][codecov-url] [![Known Vulnerabilities][snyk-image]][snyk-url] [![npm download][download-image]][download-url] [
🪛 Biome (1.9.4)
src/app/extend/application.ts
[error] 27-27: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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 (ubuntu-latest, 20)
🔇 Additional comments (16)
test/fixtures/jsonp-test/app/controller/jsonp.js (1)
5-5: Confirm function behavior after changing from generator to regular functionThe
emptyfunction has been changed from a generator function to a regular function. Ensure that this modification doesn't affect any middleware or framework expectations, especially if the function is expected to be asynchronous.If the function needs to handle asynchronous operations, consider updating it to an async function:
-exports.empty = function() {}; +exports.empty = async function() {};Alternatively, if no asynchronous behavior is required, the change is acceptable.
✅ Verification successful
Change from generator to regular function is safe
The empty function is used to test JSONP handling of empty responses, and the framework's JSONP middleware correctly handles the response regardless of the function type. The test "should support jsonp if response is empty" passes with the regular function implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find the original implementation and changes git log -p test/fixtures/jsonp-test/app/controller/jsonp.js # Find files referencing this controller rg -l "jsonp-test.*controller.*jsonp" test/ # Find similar controller patterns in test fixtures fd "controller" test/fixtures/ --type f --exec rg -l "exports\..*function"Length of output: 120
Script:
#!/bin/bash # Check the current implementation cat test/fixtures/jsonp-test/app/controller/jsonp.js # Find references to jsonp test rg -l "jsonp.*test" test/ # Find similar controller patterns ast-grep --pattern 'exports.$_ = function() { }'Length of output: 556
Script:
#!/bin/bash # Check the test file using this controller cat test/jsonp.test.tsLength of output: 5952
src/app/extend/context.ts (1)
1-3: LGTM! Clean ES module imports.The migration to ES modules is well-implemented.
src/types.ts (1)
26-55: Verify module augmentation compatibility.The module augmentation looks good, but ensure compatibility with the latest @eggjs/core version.
✅ Verification successful
Module augmentation is correctly implemented ✅
The module augmentation for
@eggjs/corefollows TypeScript best practices and properly extends the core interfaces with JSONP-specific functionality. The implementation is compatible with the current version (^6.2.13) of @eggjs/core.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check @eggjs/core version and types if [ -f "package.json" ]; then echo "Checking @eggjs/core version..." jq '.dependencies["@eggjs/core"] // .devDependencies["@eggjs/core"]' package.json fi # Verify if the core types exist rg -l "interface (EggAppConfig|Context|EggCore)" --type tsLength of output: 266
Script:
#!/bin/bash # Check if these interfaces are declared in the codebase ast-grep --pattern 'declare interface EggAppConfig' ast-grep --pattern 'declare interface Context' ast-grep --pattern 'declare interface EggCore' # Look for any other module augmentations rg "declare module '@eggjs/core'" --type ts -A 2Length of output: 343
test/jsonp.test.ts (2)
1-1: LGTM! Clean TypeScript migration.The migration to TypeScript is well-executed with proper type imports and annotations. The use of
MockApplicationtype from@eggjs/mockensures type safety.Also applies to: 4-4
15-19: LGTM! Modern async/await syntax adoption.The conversion from generator functions to async/await improves code readability while maintaining the same test coverage and assertions.
Also applies to: 22-26, 29-33, 36-40, 43-47, 50-54, 57-60, 63-69, 72-78, 81-103, 107-129, 133-155, 159-165, 168-173, 176-180, 183-189, 191-195, 198-202, 205-210
.eslintrc (1)
2-5: LGTM! Enhanced ESLint configuration.The ESLint configuration properly extends TypeScript-specific rules and enforces Node.js prefix conventions, which aligns with the project's modernization efforts.
.gitignore (1)
5-11: LGTM! Updated ignore patterns for TypeScript and modern tooling.The .gitignore patterns have been appropriately updated to handle:
- TypeScript build artifacts (dist, .tshy*)
- Modern tooling artifacts (.eslintcache)
- Package management files (package-lock.json)
.github/workflows/release.yml (2)
1-6: LGTM! Release workflow trigger configuration.The workflow is correctly configured to trigger on pushes to the master branch.
7-13: Verify secret availability.The workflow requires NPM_TOKEN and GIT_TOKEN secrets for publishing. Please ensure these secrets are properly configured in the repository settings.
Run this script to check the required secrets:
.github/workflows/nodejs.yml (1)
1-16: LGTM! Node.js versions align with the minimum version requirement.The workflow configuration correctly tests against Node.js 18.19.0 (minimum supported version), 20, and 22, which aligns with dropping support for older Node.js versions.
.github/workflows/pkg.pr.new.yml (1)
1-23: Verify the publishing workflow configuration.The workflow looks good but let's ensure it handles dual-format (CJS/ESM) publishing correctly.
Run this script to verify the publishing configuration:
✅ Verification successful
Dual-format publishing configuration verified successfully
The package.json correctly configures both ESM and CJS formats with their respective type definitions, and the workflow is properly set up to handle the publishing process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the publishing workflow handles both CJS and ESM formats # Check if tshy is configured in package.json jq -r '.tshy' package.json # Check if both formats are included in the build output echo "Checking build output paths in package.json exports:" jq -r '.exports["."]' package.jsonLength of output: 469
package.json (3)
32-34: LGTM! Node.js engine requirement aligns with PR objectives.The minimum Node.js version requirement of 18.19.0 aligns with the PR's breaking change.
10-14:⚠️ Potential issueFix incorrect exports configuration in eggPlugin.
The
exportsfield is incorrectly placed insideeggPlugin. It should be at the root level only.Apply this diff to fix the configuration:
"eggPlugin": { "name": "jsonp", "optionalDependencies": [ "security" - ], - "exports": { - "import": "./dist/esm", - "require": "./dist/commonjs", - "typescript": "./src" - } + ] },Likely invalid or redundant comment.
71-90: LGTM! Dual-format module configuration looks correct.The exports field correctly configures both ESM and CommonJS formats with their respective type definitions.
Let's verify the type definitions are correctly generated:
✅ Verification successful
Type definition configuration is correctly set up ✅
The package uses
tshyfor building hybrid modules, which will generate the correct type definitions for both ESM and CommonJS formats. The build process is properly configured with type checking viaattw.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify TypeScript declarations for both formats # Check if type definitions exist for both formats echo "Checking TypeScript declarations:" fd -e d.ts -p "index.d.ts$" dist/Length of output: 205
Script:
#!/bin/bash # Check build configuration echo "=== Package.json build scripts ===" cat package.json | jq '.scripts' echo -e "\n=== TypeScript configuration ===" fd -e json -p "tsconfig.*json$"Length of output: 577
README.md (2)
34-42: LGTM! Configuration example updated to TypeScript.The plugin configuration example has been correctly updated to use TypeScript syntax.
116-126: LGTM! Router configuration example updated to TypeScript.The router configuration example has been correctly updated to use TypeScript syntax.
[skip ci] ## [3.0.0](v2.0.0...v3.0.0) (2025-01-11) ### ⚠ 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 - **New Features** - Added TypeScript support for the JSONP plugin - Modernized project structure with ES module syntax - Enhanced type definitions and configuration - Introduced new GitHub Actions workflows for CI/CD - Added a new class for JSONP error handling - **Breaking Changes** - Renamed package from `egg-jsonp` to `@eggjs/jsonp` - Dropped support for Node.js versions below 18.19.0 - Refactored configuration and middleware approach - **Improvements** - Updated GitHub Actions workflows for CI/CD - Improved security checks for JSONP requests - Added more robust error handling - Enhanced logging configuration - **Dependency Updates** - Updated core dependencies - Migrated to modern TypeScript tooling <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#12](#12)) ([9136768](9136768))
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
eggjs/egg#5257
Summary by CodeRabbit
Release Notes
New Features
Breaking Changes
egg-jsonpto@eggjs/jsonpImprovements
Dependency Updates