feat: support cjs and esm both by tshy #23
Conversation
BREAKING CHANGE: drop Node.js < 14.0.0 support part of eggjs/egg#3644
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 43 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 changes in this pull request involve multiple modifications across various files, primarily focusing on updates to ESLint configuration, GitHub Actions workflows, TypeScript definitions, and the project structure within 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 (
|
|
New 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: 3
🧹 Outside diff range and nitpick comments (11)
src/index.ts (6)
84-87: Ensureoptions.cwdis a valid stringWhile converting
options.cwdto a string if it's an object (e.g., a URL object) is helpful, consider adding validation to ensure it results in a valid directory path. This can prevent unexpected errors if an invalidcwdis provided.
Line range hint
120-128: Avoid multiple consumption ofproc.stdoutto prevent data lossCurrently,
proc.stdoutis both being read using a'data'event listener and piped tooptions.stdout, if provided. Consuming a stream in multiple ways can lead to data loss or unexpected behavior.Refactor the code to consume
proc.stdoutonly once:if (proc.stdout) { - proc.stdout.on('data', (buf: Buffer) => { - debug('stdout %d bytes', buf.length); - stdout.push(buf); - }); if (options.stdout) { proc.stdout.pipe(options.stdout); + } else { + proc.stdout.on('data', (buf: Buffer) => { + debug('stdout %d bytes', buf.length); + stdout.push(buf); + }); } }
Line range hint
129-137: Avoid multiple consumption ofproc.stderrto prevent data lossSimilar to
proc.stdout,proc.stderrshould not be consumed in multiple ways. This can prevent data loss and ensure consistent error output handling.Refactor the code for
proc.stderr:if (proc.stderr) { - proc.stderr.on('data', (buf: Buffer) => { - debug('stderr %d bytes', buf.length); - stderr.push(buf); - }); if (options.stderr) { proc.stderr.pipe(options.stderr); + } else { + proc.stderr.on('data', (buf: Buffer) => { + debug('stderr %d bytes', buf.length); + stderr.push(buf); + }); } }
Line range hint
145-156: Check for process termination before clearing timeoutIn the
'error'and'exit'event handlers,clearTimeout(timeoutTimer);is called without checking if the timer exists. Ensure that the timer is active before attempting to clear it.Modify the
clearTimeoutcalls:isEnd = true; - clearTimeout(timeoutTimer); + if (timeoutTimer) { + clearTimeout(timeoutTimer); + }
168-170: Error message clarity inRunScriptErrorThe error message includes the entire script and flags, which can be lengthy. Consider simplifying the error message or ensuring that overly long commands are truncated for readability.
Example:
const err = new RunScriptError( - `Run "${sh} ${shFlag} ${script}" error, exit code ${code}`, stdio, code); + `Running the script failed with exit code ${code}`, stdio, code);
198-200: Error message clarity inRunScriptTimeoutErrorSimilar to
RunScriptError, ensure the timeout error message is clear and concise.Example:
const err = new RunScriptTimeoutError( - `Run "${sh} ${shFlag} ${script}" timeout in ${extraOptions.timeout}ms`, stdio, timeout); + `Script execution timed out after ${timeout}ms`, stdio, timeout);.github/workflows/nodejs.yml (1)
15-15: Consider including Node.js 14 in the CI test matrixThe CI configuration has been updated to test against Node.js versions
16, 18, 20, 22, but Node.js 14 support has been omitted. Since the project only drops support for Node.js versions earlier than 14, and Node.js 14 might still be in use, consider adding it back to ensure compatibility.Update the
versionfield:os: 'ubuntu-latest, macos-latest, windows-latest' - version: '16, 18, 20, 22' + version: '14, 16, 18, 20, 22'.github/workflows/nodejs-14.yml (1)
22-22: Improve reliability by separating NVM operationsThe current approach chains multiple commands in a single line, making it harder to debug failures. Consider breaking this into separate steps.
- - run: . /home/runner/mynvm/nvm.sh && nvm install 14 && nvm use 14 && node -v && npm run test:node14 + - name: Setup Node.js 14 environment + run: | + . ~/.nvm/nvm.sh + nvm install 14 + nvm use 14 + - name: Verify Node.js version + run: node -v + - name: Run Node.js 14 tests + run: npm run test:node14README.md (2)
1-4: Consider adding Node.js version requirementsSince this PR drops support for Node.js versions <14, it would be helpful to add a clear note about the minimum Node.js version requirement.
# runscript 📢📢📢📢📢 You should use [execa](https://github.com/sindresorhus/execa) instead. 📢📢📢📢 +> **Note:** Requires Node.js 14 or later + ---
Line range hint
30-38: Consider adding ESM import exampleSince the PR adds ESM support, it would be helpful to include an ESM import example alongside the CommonJS example.
Add an ESM example:
// ESM import { runScript } from 'runscript'; await runScript('node -v', { stdio: 'pipe' }) .then(stdio => { console.log(stdio); }) .catch(err => { console.error(err); });test/runscript.test.ts (1)
Line range hint
174-200: Test coverage for CJS interop scenariosThe test includes a specific test for CJS module (child-process-with-unclosed-stdio.cjs) but we should verify complete interop coverage.
Consider adding explicit tests for:
- ESM importing CJS modules
- CJS requiring ESM modules (with dynamic import)
- Mixed format child process spawning
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
.eslintcache(1 hunks).eslintrc(1 hunks).github/workflows/codeql.yml(0 hunks).github/workflows/nodejs-14.yml(1 hunks).github/workflows/nodejs.yml(1 hunks).gitignore(1 hunks)README.md(4 hunks)index.d.ts(0 hunks)package.json(2 hunks)src/index.ts(9 hunks)test/fixtures/child-process-with-unclosed-stdio.cjs(1 hunks)test/fixtures/ts/check.ts(0 hunks)test/fixtures/ts/checkExtraOptions.ts(0 hunks)test/fixtures/ts/tsconfig.json(0 hunks)test/helper.ts(1 hunks)test/index.d.ts.test.js(0 hunks)test/runscript.test.ts(6 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (6)
- test/fixtures/ts/tsconfig.json
- test/fixtures/ts/checkExtraOptions.ts
- .github/workflows/codeql.yml
- test/fixtures/ts/check.ts
- test/index.d.ts.test.js
- index.d.ts
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- tsconfig.json
- .eslintcache
🧰 Additional context used
🪛 eslint
test/fixtures/child-process-with-unclosed-stdio.cjs
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🔇 Additional comments (13)
src/index.ts (3)
1-6: Import statements updated to ES module syntax
The import statements have been correctly updated to use ES module syntax, ensuring compatibility with modern Node.js versions and TypeScript.
44-55: Custom Error classes are properly defined
The RunScriptError and RunScriptTimeoutError classes correctly extend the base Error class and include additional properties for detailed error handling. The use of Error.captureStackTrace ensures that the stack trace starts at the point where the error is instantiated.
89-94: Validation of options.stdout and options.stderr streams
The assertions correctly ensure that options.stdout and options.stderr are writable streams, preventing errors during stream operations.
.eslintrc (1)
2-5: ESLint configuration updated for TypeScript support
Extending "eslint-config-egg/typescript" and "eslint-config-egg/lib/rules/enforce-node-prefix" ensures that linting rules are properly applied for TypeScript and Node.js specific conventions.
test/helper.ts (1)
1-9: Helper function getFixtures is correctly implemented
The getFixtures function correctly resolves the path to fixture files using ES module syntax and ensures compatibility across different environments.
test/fixtures/child-process-with-unclosed-stdio.cjs (2)
2-2: LGTM: Import style updated for dual CJS/ESM support
The change to named imports (const { runScript } = require('../../')) aligns with the PR objective of supporting both CJS and ESM formats. The ESLint warning about require statements can be ignored as this is explicitly a CJS file.
🧰 Tools
🪛 eslint
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
7-7: Verify path resolution works in both CJS and ESM contexts
The script path has been updated to use the .cjs extension. Let's verify that this path resolution works consistently.
✅ Verification successful
Path resolution is correctly implemented for both CJS and ESM contexts
The path resolution is working correctly because:
- The file is explicitly using
.cjsextension in both the test suite (test/runscript.test.ts) and the fixture file itself - The package is configured with
"type": "module", making explicit extensions mandatory - The file is properly referenced using both
path.join()and the test helpergetFixtures() - No mismatched extensions were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any mismatched extensions in script paths
rg -g '*.{js,cjs,mjs,ts}' "child-process-with-unclosed-stdio\.[^c]js"
Length of output: 71
Script:
#!/bin/bash
# Let's check how this file is being used across the codebase
fd "child-process-with-unclosed-stdio" --type f
# Check the content of the test file to understand the context
cat test/fixtures/child-process-with-unclosed-stdio.cjs
# Check if there are any imports/requires of this file
rg -g '*.{js,cjs,mjs,ts}' -l "require.*child-process-with-unclosed-stdio"
rg -g '*.{js,cjs,mjs,ts}' -l "import.*child-process-with-unclosed-stdio"
Length of output: 935
Script:
#!/bin/bash
# Let's check if this test file is being executed in any test suites
rg -g '*.{js,cjs,mjs,ts}' "child-process-with-unclosed-stdio" --type-add 'test:*.{test,spec}.{js,cjs,mjs,ts}' -g 'test:*'
# Let's also check the package.json to see if there are any relevant test configurations
cat package.json | jq '.scripts, .type'
Length of output: 736
package.json (3)
48-74: Dual format configuration looks good but verify package.json exports
The configuration for dual CJS/ESM support is well structured with:
type: "module"for ESM default- Proper export maps for both formats
- Correct main/module/types fields
Let's verify the exports configuration:
✅ Verification successful
Package exports configuration is correctly aligned with source files
The verification shows that:
- There is only one source file
src/index.tswhich is the main entry point - Package exports correctly map to this entry point with both ESM and CJS formats
- The exports configuration in package.json properly covers all source files since there is only one entry point
- The entry point exports all necessary types and functions for the package's functionality
The dual format configuration is valid and complete with no missing exports.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if all source files are properly exported
fd -e ts -e js src/ | while read -r file; do
echo "Checking $file..."
# Extract the relative path without extension
base_path=$(echo "$file" | sed 's/^src\///' | sed 's/\.[^.]*$//')
# Check if it's exported in package.json
rg "\"\./$base_path\"" package.json || echo "Warning: $file might not be exported"
done
Length of output: 358
Script:
#!/bin/bash
# List all source files
fd -e ts -e js . src/
# Check package.json exports
echo -e "\nChecking package.json exports:"
cat package.json | jq -r '.exports | keys[]'
# Show the entry point file content
echo -e "\nEntry point content:"
cat src/index.ts || cat src/index.js
Length of output: 6102
6-12: Build setup looks good but verify test coverage for both formats
The build setup using tshy for dual CJS/ESM support is well configured. However, the test scripts only run in one format.
Let's verify if tests run for both formats:
✅ Verification successful
Tests are properly configured for dual format compatibility
The test setup is correctly handling both CJS and ESM formats:
- The test files are written in TypeScript and use ESM imports
- The test helper properly handles ESM paths using
import.meta.url - The test fixtures include both
.jsand.cjsfiles - The build step (
prepublishOnly) runs before tests, ensuring both formats are built - The test runner (
egg-bin) is compatible with both CJS and ESM
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are format-specific tests
rg -l "import.*from.*\.(js|cjs|mjs)" test/
rg -l "require.*\.(js|cjs)" test/
Length of output: 103
Script:
#!/bin/bash
# Let's check the test files and their imports more thoroughly
echo "=== Test Files ==="
fd . test/ -e ts -e js
echo -e "\n=== Test File Contents ==="
for f in $(fd . test/ -e ts -e js); do
echo -e "\n=== $f ==="
cat "$f"
done
echo -e "\n=== Package.json Test Scripts ==="
cat package.json | jq '.scripts | with_entries(select(.key | contains("test")))'
Length of output: 9123
15-28: Dependencies alignment needs verification
The dependencies have been updated significantly, particularly:
- is-type-of upgraded to ^2.2.0
- New TypeScript-related devDependencies added
Let's verify compatibility:
✅ Verification successful
The previous results show that is-type-of@2.2.0 has proper dual format support (both ESM and CommonJS) with TypeScript types. Let's verify how it's being used in the codebase and check the TypeScript configuration to ensure compatibility.
Dependencies are properly aligned and compatible
The verification shows:
is-type-of@2.2.0provides dual format support (ESM/CommonJS) with TypeScript types- The package is correctly imported using ESM syntax in
src/index.ts - The project is configured as ESM (
"type": "module") and uses TypeScript - The dependency upgrade is compatible with the project's module system and TypeScript setup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if is-type-of v2 has dual format support
curl -s https://registry.npmjs.org/is-type-of/2.2.0 | jq '.exports, .type'
# Verify if any dependencies have known dual format issues
npx attw --pack
Length of output: 466
Script:
#!/bin/bash
# Check for any direct usage of is-type-of in the codebase
rg "require\(['\"]is-type-of['\"]|from ['\"]is-type-of['\"]" -g '!package*.json' -g '!*.d.ts'
# Check package.json for any TypeScript configuration
cat package.json | jq '.tshy, .type'
# Look for any TypeScript configuration files
fd -e tsconfig.json
Length of output: 316
test/runscript.test.ts (3)
1-4: Import statements properly updated for ESM
The conversion to ESM imports is correct, including the use of node: protocol for built-ins.
118-126: File path handling needs cross-platform verification
The test is using getFixtures helper for paths, but we should verify cross-platform compatibility.
✅ Verification successful
Let me check the specific path handling implementations in these files to better understand the cross-platform handling.
Let me check the getFixtures implementation to complete the verification.
Let me check the helper.js implementation to see how getFixtures handles paths.
Let me try to find the helper file with the correct extension.
Cross-platform path handling is properly implemented
The codebase demonstrates robust cross-platform path handling:
getFixturesuses Node'spath.join()which automatically handles platform-specific path separators- The main code in
src/index.tshas specific Windows path handling logic - Test cases include platform-specific tests for Windows paths
- Path normalization is properly handled for both Unix and Windows style paths
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Windows-specific path handling
rg -l "path\.win32|\\\\|\\.exe" src/ test/
Length of output: 77
Script:
#!/bin/bash
# Check the implementation details of path handling in both files
rg -C 5 "path\.win32|\\\\|\\.exe" src/index.ts test/runscript.test.ts
Length of output: 6017
Script:
#!/bin/bash
# Check getFixtures implementation
rg -C 5 "getFixtures" test/runscript.test.ts
Length of output: 3752
Script:
#!/bin/bash
# Check helper.js implementation
rg -C 5 "getFixtures" test/helper.js
Length of output: 91
Script:
#!/bin/bash
# Find helper file and its implementation
fd helper.ts test/ && cat test/helper.ts
Length of output: 328
50-58: Timeout error handling improved but needs verification
The timeout error handling has been improved with proper TypeScript types and assertions. However, we should verify the timeout behavior is consistent across formats.
✅ Verification successful
Timeout error handling is properly implemented and verified
The timeout behavior is consistent across different scenarios:
- Handles stdout timeouts (1200ms) with proper error type and message verification
- Handles stderr timeouts (1500ms) with proper error type
- Correctly handles normal exit before timeout (2000ms)
- RunScriptTimeoutError properly captures stdio and timeout information
The implementation is well-tested with different test fixtures that cover:
- Continuous stdout output (timeout.js)
- Continuous stderr output (timeout-stderr.js)
- Normal exit before timeout (timeout-and-exit.js)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent timeout handling across the codebase
rg -A 3 "timeout.*RunScriptTimeoutError" src/
Length of output: 47
Script:
#!/bin/bash
# Let's first check for timeout-related error handling in the source code
rg -A 5 "timeout.*Error" src/
# Also check for timeout-related test cases
rg -A 5 "timeout.*Error" test/
# Let's also look for the RunScriptTimeoutError class definition
ast-grep --pattern 'class RunScriptTimeoutError {
$$$
}'
Length of output: 464
Script:
#!/bin/bash
# Let's check the implementation of the timeout.js fixture
cat "$(fd 'timeout.js' fixtures)"
# Let's also check for all timeout-related test cases
rg -B 3 -A 3 "timeout.*test" test/
Length of output: 202
Script:
#!/bin/bash
# Let's check the test directory structure first
fd . test/
# Then look for timeout-related files and tests
rg -B 3 -A 3 "timeout" test/
# Check the RunScriptTimeoutError implementation
rg -A 10 "class RunScriptTimeoutError" src/
Length of output: 2646
Script:
#!/bin/bash
# Let's check the content of the timeout test fixtures
cat test/fixtures/timeout.js
cat test/fixtures/timeout-stderr.js
cat test/fixtures/timeout-and-exit.js
Length of output: 511
[skip ci] ## [2.0.0](v1.6.0...v2.0.0) (2024-12-09) ### ⚠ BREAKING CHANGES * drop Node.js < 14.0.0 support part of eggjs/egg#3644 ### Features * support cjs and esm both by tshy ([#23](#23)) ([eb81077](eb81077))
BREAKING CHANGE: drop Node.js < 14.0.0 support
part of eggjs/egg#3644
Summary by CodeRabbit
New Features
Bug Fixes
runScriptfunction.Documentation
Chores
.gitignorefile to exclude additional files and directories.Refactor