feat: support cjs and esm both by tshy #120
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 8 minutes and 56 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)
WalkthroughThis pull request encompasses a significant overhaul of the project's configuration and testing framework. Key changes include the restructuring of the ESLint configuration to utilize external presets, updates to GitHub Actions workflows for Node.js compatibility, the introduction of new workflows for package publishing, and enhancements to the Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant CI as GitHub Actions
participant Node as Node.js
participant Package as Package Registry
Dev->>CI: Push code changes
CI->>Node: Setup Node.js 20
CI->>CI: Install dependencies
CI->>CI: Run build
CI->>Package: Publish package
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: 15
🧹 Nitpick comments (5)
test/cfork.test.ts (1)
184-238: Refactor repetitive test code to reduce duplicationBetween lines 184 and 238, the
await assert.rejectsblock is repeated multiple times with identical logic. Consider refactoring this code to eliminate duplication, which will improve maintainability and readability.You can refactor by looping the assertions:
const attempts = 7; for (let i = 0; i < attempts; i++) { await assert.rejects(async () => { await urllib.request('http://127.0.0.1:1984/error', { dataType: 'text', timeout: 5000, }); }, (err: any) => { assert.match(err.message, /(timeout|ECONNREFUSED|other side closed)/); return true; }); }package.json (1)
41-48: Review build process completenessThe new build process includes TypeScript compilation and type checking. However, there might be a need for additional scripts:
Consider adding:
- A
build:watchscript for development- A
cleanscript to remove dist directory- A
typecheckscript to run TypeScript checks without building"scripts": { "lint": "eslint --cache src test --ext .ts", "pretest": "npm run lint -- --fix && npm run prepublishOnly", "test": "egg-bin test", "preci": "npm run lint && npm run prepublishOnly", "ci": "egg-bin cov", "prepublishOnly": "tshy && tshy-after && attw --pack", + "build:watch": "tshy --watch", + "clean": "rm -rf dist", + "typecheck": "tsc --noEmit" },README.md (3)
Line range hint
32-72: Fix typos and enhance TypeScript examples.
There are spelling inconsistencies in the example:
- "wroker" should be "worker" in the disconnect and exit event handlers
Consider adding TypeScript type annotations for better documentation:
- .on('fork', worker => { + .on('fork', (worker: import('cluster').Worker) => {
74-77: Consider providing equivalent examples for both ESM and CJS.The CommonJS example is significantly shorter than the ESM example. Consider either:
- Providing a full CommonJS example with event handlers, or
- Making the ESM example shorter and moving the detailed event handling to a separate section
89-89: Improve autoCoverage option documentation.Consider adding a hyphen and more context about Istanbul coverage:
- * **autoCoverage**: auto fork with istanbul when `running_under_istanbul` env set, default is `false` + * **autoCoverage**: auto-fork with Istanbul code coverage tool when `running_under_istanbul` environment variable is set, default is `false`🧰 Tools
🪛 LanguageTool
[uncategorized] ~89-~89: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...limit / duration) * autoCoverage: auto fork with istanbul when `running_under_istan...(AUTO_HYPHEN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.eslintrc(1 hunks).github/workflows/nodejs.yml(1 hunks).github/workflows/pkg.pr.new.yml(1 hunks).gitignore(1 hunks)README.md(3 hunks)fixtures/master.js(0 hunks)index.js(0 hunks)package.json(2 hunks)src/index.ts(1 hunks)test/cfork.test.js(0 hunks)test/cfork.test.ts(1 hunks)test/fixtures/kill_worker/master.cjs(1 hunks)test/fixtures/master.cjs(1 hunks)test/fixtures/one_worker_cluster/master.cjs(1 hunks)test/fixtures/slave.cjs(2 hunks)test/fixtures/worker.cjs(2 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (3)
- fixtures/master.js
- index.js
- test/cfork.test.js
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- test/fixtures/one_worker_cluster/master.cjs
- .eslintrc
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~89-~89: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...limit / duration) * autoCoverage: auto fork with istanbul when `running_under_istan...
(AUTO_HYPHEN)
🪛 Biome (1.9.4)
test/fixtures/master.cjs
[error] 29-29: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 35-35: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
src/index.ts
[error] 122-122: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 123-123: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 164-164: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 165-165: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 283-283: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🪛 eslint
test/fixtures/master.cjs
[error] 50-50: 'master' is not defined.
(no-undef)
🔇 Additional comments (11)
tsconfig.json (1)
1-10: TypeScript configuration looks good
The tsconfig.json file is well-configured, enabling strict type checking and aligning with modern JavaScript standards.
.github/workflows/nodejs.yml (1)
15-15: LGTM! Version changes align with the PR objectives.
The updated Node.js version matrix correctly reflects:
- Dropping support for older versions (14, 16)
- Adding specific minimum version (18.19.0)
- Maintaining support for newer versions (18, 20, 22)
Let's verify the Node.js version requirements are consistent across the project:
✅ Verification successful
Node.js version requirements are consistent across the project
The verification confirms that:
- The
package.jsoncorrectly specifies"node": ">= 18.19.0"in its engines field - This aligns perfectly with the GitHub workflow's version matrix starting from
18.19.0 - All Node.js imports use the new
node:protocol prefix, indicating modern Node.js practices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Node.js version specifications across the project
echo "Checking Node.js version requirements..."
rg -g '!node_modules' -g '!package-lock.json' "18\.19\.0|engines|node:"
Length of output: 1029
test/fixtures/slave.cjs (1)
29-29: Verify the updated cfork import path
The import path has been updated to use the project root. Let's ensure this works with both CJS and ESM.
✅ Verification successful
Import path is correctly configured
The import path require('../..') in test/fixtures/slave.cjs is correct and consistent with the project structure. The package.json confirms this project supports both CJS and ESM formats, with proper exports configuration. The same import pattern is consistently used in both test files (slave.cjs and worker.cjs), and it correctly resolves to the package root.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check cfork import usage across test files
echo "Checking cfork import patterns..."
rg "require\(['\"]\.\.(/\.\.)?['\]\)\.cfork" test/
rg "import.*cfork.*from" test/
Length of output: 292
Script:
#!/bin/bash
# Check cfork import usage with corrected regex
echo "Checking cfork import patterns..."
rg "require\\(['\"]\\.\\./\\.\\.['\"]\\.cfork" test/
rg "require\\(['\"]\\.\\.['\"]\\.cfork" test/
Length of output: 177
Script:
#!/bin/bash
# Let's check the content of the test files to understand the import patterns
echo "Content of test/fixtures/slave.cjs:"
cat test/fixtures/slave.cjs
echo -e "\nChecking all require statements in test directory:"
rg "require\\(.*cfork" test/
Length of output: 1080
Script:
#!/bin/bash
# Let's check the content of worker.cjs and package.json to understand the module structure
echo "Content of test/fixtures/worker.cjs:"
cat test/fixtures/worker.cjs
echo -e "\nContent of package.json:"
cat package.json
Length of output: 2964
test/fixtures/worker.cjs (1)
1-2: LGTM: Modern Node.js import practices
Good use of the node: protocol for importing built-in modules, which is recommended for explicit internal module resolution.
test/fixtures/kill_worker/master.cjs (1)
8-10: Verify worker and slave path resolution
The paths have been updated to use .cjs extension, which is correct for CommonJS modules. Let's verify these files exist.
✅ Verification successful
Path resolution verified successfully
The referenced worker and slave files exist at the correct locations with the .cjs extension, confirming proper path resolution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the referenced .cjs files exist
fd -e cjs "worker|slave" test/fixtures/
Length of output: 90
package.json (3)
56-68: LGTM: Proper dual module system configuration
The exports field is correctly configured to support both ESM and CommonJS:
- ESM: ./dist/esm/index.js with types
- CommonJS: ./dist/commonjs/index.js with types
69-72: LGTM: Appropriate files included in package
The files array correctly includes the necessary directories:
- dist: For compiled output
- src: For source maps and TypeScript source reference
27-40: Verify TypeScript dependencies compatibility
The development dependencies have been significantly updated to support TypeScript and modern tooling.
✅ Verification successful
TypeScript configuration appears compatible and well-structured
The TypeScript setup shows a proper configuration with:
- Appropriate core dependencies: TypeScript 5.x, @types/node 22.x
- Build tooling: tshy 3.x for hybrid package builds
- Type checking: @arethetypeswrong/cli for validating type definitions
- Proper source structure: TypeScript files in src/ and test/
- Valid tsconfig.json present
- TypeScript-aware scripts configured in package.json
The dependencies are at compatible major versions and the project structure follows TypeScript best practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential dependency conflicts
npm ls @types/node typescript @eggjs/tsconfig 2>/dev/null || true
# Verify TypeScript configuration
fd -e json "tsconfig" . -x cat {} \; | jq .
Length of output: 65679
Script:
#!/bin/bash
# Check for TypeScript configuration files
fd -e json "tsconfig" .
# Check for TypeScript source files
fd -e ts -e tsx .
# Check package.json for TypeScript-related configurations
cat package.json | jq '.dependencies, .devDependencies, .scripts, .tshy'
Length of output: 835
README.md (3)
7-7: LGTM: Node.js version badge addition is appropriate.
The new badge clearly communicates the Node.js version requirement, which is important given the breaking change dropping support for versions < 18.19.0.
25-25: LGTM: Installation command simplified appropriately.
Removing the --save flag aligns with modern npm behavior where saving dependencies is the default.
100-102: LGTM: Modern approach to contributor recognition.
The switch to contrib.rocks provides a more visual and maintainable way to recognize contributors.
| }); | ||
|
|
||
| setTimeout(function() { | ||
| master.mock.uncaughtException; |
There was a problem hiding this comment.
Fix undefined variable master
The variable master is not defined, which will cause a ReferenceError at runtime. If the intention is to simulate an uncaught exception, you should throw an error explicitly.
Apply this diff:
- master.mock.uncaughtException;
+ // Simulate an uncaught exception
+ throw new Error('Mock uncaught exception');📝 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.
| master.mock.uncaughtException; | |
| // Simulate an uncaught exception | |
| throw new Error('Mock uncaught exception'); |
🧰 Tools
🪛 eslint
[error] 50-50: 'master' is not defined.
(no-undef)
| }) | ||
| .on('exit', function(worker, code, signal) { | ||
| const exitCode = worker.process.exitCode; | ||
| const propertyName = worker.hasOwnProperty('exitedAfterDisconnect') ? 'exitedAfterDisconnect' : 'suicide'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Object.hasOwn() instead of hasOwnProperty
Same issue as above. Replace hasOwnProperty with Object.hasOwn() for safe property existence checks.
Apply this diff:
- const propertyName = worker.hasOwnProperty('exitedAfterDisconnect') ? 'exitedAfterDisconnect' : 'suicide';
+ const propertyName = Object.hasOwn(worker, 'exitedAfterDisconnect') ? 'exitedAfterDisconnect' : 'suicide';📝 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.
| const propertyName = worker.hasOwnProperty('exitedAfterDisconnect') ? 'exitedAfterDisconnect' : 'suicide'; | |
| const propertyName = Object.hasOwn(worker, 'exitedAfterDisconnect') ? 'exitedAfterDisconnect' : 'suicide'; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
test/fixtures/one_worker_cluster/master.cjs (2)
25-25: Use 'Object.hasOwn' instead of 'hasOwnProperty'To avoid issues with the prototype chain and possible false positives, it's recommended to use
Object.hasOwn()when checking for an object's own properties.Apply this diff:
- const propertyName = worker.hasOwnProperty('exitedAfterDisconnect') ? 'exitedAfterDisconnect' : 'suicide'; + const propertyName = Object.hasOwn(worker, 'exitedAfterDisconnect') ? 'exitedAfterDisconnect' : 'suicide';🧰 Tools
🪛 Biome (1.9.4)
[error] 25-25: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
31-31: Use 'Object.hasOwn' instead of 'hasOwnProperty'Consistently use
Object.hasOwn()for property checks to ensure reliability and avoid potential issues with inherited properties.Apply this diff:
- const propertyName = worker.hasOwnProperty('exitedAfterDisconnect') ? 'exitedAfterDisconnect' : 'suicide'; + const propertyName = Object.hasOwn(worker, 'exitedAfterDisconnect') ? 'exitedAfterDisconnect' : 'suicide';🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
test/kill_worker.test.ts (1)
1-1: Import 'strict' assertions for better accuracyTo use strict assertion methods like
assert.strictEqual, import{ strict as assert }from'node:assert'.Apply this diff:
-import assert from 'node:assert'; +import { strict as assert } from 'node:assert';Then update your assertion in line 42:
-assert.equal(response.data, 'kill 3 workers'); +assert.strictEqual(response.data, 'kill 3 workers');test/one_worker_cluster.test.ts (1)
44-44: Remove duplicate 'ECONNRESET' in the regular expressionsThe error message
ECONNRESETis repeated in the regex patterns. Removing the duplicate improves readability.Apply this diff:
- assert.match(err.message, /(ECONNRESET|socket hang up|timeout|ECONNRESET)/); + assert.match(err.message, /(ECONNRESET|socket hang up|timeout)/);Also applies to: 52-52
test/fixtures/kill_worker/master.cjs (2)
27-27: Use 'Object.hasOwn' instead of 'hasOwnProperty'For reliable property checks and to avoid issues with the object's prototype chain, replace
hasOwnPropertywithObject.hasOwn().Apply this diff:
- const propertyName = worker.hasOwnProperty('exitedAfterDisconnect') ? 'exitedAfterDisconnect' : 'suicide'; + const propertyName = Object.hasOwn(worker, 'exitedAfterDisconnect') ? 'exitedAfterDisconnect' : 'suicide';🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
33-33: Use 'Object.hasOwn' instead of 'hasOwnProperty'Consistently apply
Object.hasOwn()for property existence checks to prevent potential errors.Apply this diff:
- const propertyName = worker.hasOwnProperty('exitedAfterDisconnect') ? 'exitedAfterDisconnect' : 'suicide'; + const propertyName = Object.hasOwn(worker, 'exitedAfterDisconnect') ? 'exitedAfterDisconnect' : 'suicide';🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
test/cfork.test.ts(1 hunks)test/fixtures/kill_worker/master.cjs(1 hunks)test/fixtures/one_worker_cluster/master.cjs(1 hunks)test/kill_worker.test.js(0 hunks)test/kill_worker.test.ts(1 hunks)test/one_worker_cluster.test.js(0 hunks)test/one_worker_cluster.test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- test/one_worker_cluster.test.js
- test/kill_worker.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/cfork.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
test/fixtures/kill_worker/master.cjs
[error] 27-27: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 33-33: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
test/fixtures/one_worker_cluster/master.cjs
[error] 25-25: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 31-31: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🪛 eslint
test/fixtures/one_worker_cluster/master.cjs
[error] 46-46: 'mock' is not defined.
(no-undef)
🔇 Additional comments (1)
test/fixtures/one_worker_cluster/master.cjs (1)
46-46: Undefined variable 'mock' may cause unintended errors
The variable mock is not defined, which will result in a ReferenceError. If the intention is to simulate an uncaught exception for testing purposes, please add a comment to clarify this. Otherwise, define mock or adjust the code accordingly.
🧰 Tools
🪛 eslint
[error] 46-46: 'mock' is not defined.
(no-undef)
[skip ci] ## [2.0.0](v1.11.0...v2.0.0) (2024-12-15) ### ⚠ 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** - Introduced a new GitHub Actions workflow for automated package publishing. - Added TypeScript configuration to enhance type safety and modern JavaScript compatibility. - New tests for child process management functionality. - **Updates** - Updated ESLint configuration to utilize external presets. - Modified GitHub Actions CI to specify a more precise Node.js version. - Enhanced README with installation and usage examples for ESM and TypeScript. - Updated package.json to reflect new module structure and dependencies. - Improved .gitignore to exclude additional temporary files and directories. - **Bug Fixes** - Improved .gitignore to exclude additional temporary files and directories. - **Removals** - Removed outdated JavaScript files and configurations in favor of CommonJS and TypeScript standards. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#120](#120)) ([b2e1b53](b2e1b53))
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
eggjs/egg#5257
Summary by CodeRabbit
Release Notes
New Features
Updates
Bug Fixes
Removals