feat: support cjs and esm both by tshy #1
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257
WalkthroughThe pull request represents a comprehensive refactoring of the Changes
Sequence DiagramsequenceDiagram
participant User
participant SuperTest as @eggjs/supertest
participant Server
User->>SuperTest: Import library
User->>SuperTest: Create request
SuperTest->>Server: Send HTTP request
Server-->>SuperTest: Receive response
SuperTest-->>User: Return testable response
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 ↗︎
|
commit: |
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: 10
🔭 Outside diff range comments (2)
test/supertest.test.ts (1)
Line range hint
264-283: Nested requests
Nested requests can sometimes cause race conditions if servers aren’t closed properly or if concurrency is introduced. Keep an eye on possible concurrency pitfalls when scaling these tests.README.md (1)
Line range hint
49-67: Add ESM import example alongside CommonJSSince the library now supports both CJS and ESM, provide examples for both import styles.
Add ESM example:
const { request } = require('@eggjs/supertest'); +// Or using ESM: +// import { request } from '@eggjs/supertest';
🧹 Nitpick comments (16)
.github/workflows/pkg.pr.new.yml (1)
12-16: Enhance Node.js setup configurationThe setup can be improved for better reliability and performance:
- Specify the minimum Node.js version as per PR requirements (≥18.19.0)
- Pin the package manager version
- Add dependency caching
Apply these improvements:
- - run: corepack enable + - run: corepack enable && corepack prepare pnpm@8.9.0 --activate - uses: actions/setup-node@v4 with: - node-version: 20 + node-version: '>=18.19.0' + cache: 'pnpm'src/request.ts (2)
7-9: Consider documenting http2 usage in RequestOptions
While the optional http2 property is clear, adding extra JSDoc or TSDoc comments about the default and implications could help other contributors.
28-34: Test coverage suggestion for _testRequest
It’s beneficial to confirm that all code paths are tested—specifically, verifying that the .http2() call within the Test class is covered. This ensures reliable behavior in both HTTP/1 and HTTP/2 modes.src/agent.ts (2)
42-61: Validate cookie and header handling
The _testRequest method attaches cookies and sets headers. Consider adding tests that validate these side effects on subsequent requests, ensuring correct behavior when multiple consecutive requests are made.
88-93: Consider clarifying proxyAgent usage
proxyAgent is a Proxy for TestAgent construction. For maintainability, including minimal inline commentary or docstrings would help future contributors understand the delegation logic.src/test.ts (2)
Line range hint
187-197: Careful with internal redirection
While this redirect logic seems straightforward, note that frequent server redirects can complicate testing flows. Keep track of redirect usage and confirm that different status codes are effectively tested (e.g., 3xx).
Line range hint
351-355: Check ephemeral server concurrency
Line 351 closes the server immediately, which might inadvertently affect concurrent tests if they rely on the same instance. If concurrency is planned, ensure each test suite or test file spawns its own server.🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
test/supertest.test.ts (2)
128-148: Validate POST body transformations
The lines at 128–148 show testing with bodyParser and JSON payloads. Consider adding negative test cases (e.g., invalid JSON) to confirm robust error handling.
383-392: Handle connection refused for ephemeral port
We've tested an ephemeral port that’s not open (127.0.0.1:1234). This is valuable coverage—just ensure tests that rely on ephemeral ports are stable in CI environments to avoid transient failures.test/throwError.ts (2)
4-8: Add return type annotation for better type safetyWhile the TypeScript migration is good, consider adding an explicit return type annotation for better type safety and documentation.
-export function throwError(message: string) { +export function throwError(message: string): () => never {
1-3: Enhance documentation for better maintainabilityConsider expanding the documentation to include:
/** * This method needs to reside in its own module in order to properly test stack trace handling. + * + * @param message - The error message to throw + * @returns A function that throws an Error with the provided message + * @throws {Error} Always throws with the provided message */tsconfig.json (1)
3-9: Consider additional compiler options for enhanced type safetyWhile the current configuration is good, consider adding these compiler options for better type safety and maintainability:
"compilerOptions": { "strict": true, "noImplicitAny": true, "target": "ES2022", "module": "NodeNext", - "moduleResolution": "NodeNext" + "moduleResolution": "NodeNext", + "esModuleInterop": true, + "skipLibCheck": true, + "forceConsistentCasingInFileNames": true }src/error/AssertError.ts (1)
2-3: Consider using generic type parameters instead of 'any'.The
expectedandactualproperties could benefit from stronger typing using generics.-export class AssertError extends Error { - expected: any; - actual: any; +export class AssertError<T = unknown> extends Error { + expected: T; + actual: T;src/types.ts (1)
15-17: Consider documenting HTTP/2 option behavior.The
http2flag inAgentOptionscould benefit from JSDoc comments explaining its behavior and default value.export interface AgentOptions extends SAgentOptions { + /** Enable HTTP/2 support. Defaults to false. */ http2?: boolean; }src/index.ts (2)
9-11: Consider adding more detailed JSDoc documentationWhile the basic description is present, consider adding:
@param appdescription@param optionsdescription@returnsdescription- Example usage
25-39: Review the Proxy implementation for edge casesThe Proxy implementation looks solid but consider:
- The
applytrap might need type checking forargumentsList- The type assertion at the end is quite complex
Consider this safer implementation:
- apply(target, _, argumentsList) { + apply(target, _, argumentsList: unknown[]) { + if (argumentsList.length < 1) { + throw new TypeError('App parameter is required'); + } return target(argumentsList[0], argumentsList[1]); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.editorconfig(0 hunks).eslintrc(1 hunks).github/workflows/node.js.yml(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).npmignore(0 hunks)LICENSE(1 hunks)README.md(7 hunks)ci/remove-deps-4-old-node.js(0 hunks)index.js(0 hunks)lib/agent.js(0 hunks)package.json(2 hunks)src/agent.ts(1 hunks)src/error/AssertError.ts(1 hunks)src/index.ts(1 hunks)src/request.ts(1 hunks)src/test.ts(9 hunks)src/types.ts(1 hunks)test/.eslintrc(0 hunks)test/supertest.test.ts(29 hunks)test/throwError.ts(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (7)
- .editorconfig
- .npmignore
- test/.eslintrc
- ci/remove-deps-4-old-node.js
- .github/workflows/node.js.yml
- index.js
- lib/agent.js
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- LICENSE
🧰 Additional context used
🪛 Biome (1.9.4)
src/test.ts
[error] 11-11: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (16)
.eslintrc (1)
2-5: LGTM! Verify TypeScript linting coverage.
The migration to eslint-config-egg/typescript aligns well with the TypeScript migration. The addition of Node.js prefix enforcement is also appropriate given the Node.js version requirements.
Let's verify the TypeScript linting rules coverage:
✅ Verification successful
TypeScript ESLint configuration is properly set up
The verification shows:
- TypeScript configuration is properly set up with
@eggjs/tsconfig - ESLint is configured with
eslint-config-egg/typescript(v14) which is the official egg.js TypeScript ruleset - No conflicting ESLint configurations were found
- The configuration covers all TypeScript files in the project (8 .ts files identified)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new ESLint config covers all necessary TypeScript rules
# Check if tsconfig.json exists and its contents
echo "Checking TypeScript configuration..."
if [ -f "tsconfig.json" ]; then
cat tsconfig.json
fi
# List all TypeScript files to ensure they're covered by linting
echo "Listing TypeScript files..."
fd -e ts -e tsx
# Check for any existing ESLint overrides in package.json
echo "Checking for ESLint configurations in package.json..."
if [ -f "package.json" ]; then
cat package.json | jq '.eslintConfig'
fi
Length of output: 726
Script:
#!/bin/bash
# Let's verify the eslint-config-egg/typescript rules coverage
# Check the contents of the ESLint config file
echo "Current ESLint configuration:"
cat .eslintrc
# Check if eslint-config-egg is installed and its version
echo -e "\nInstalled eslint-config-egg version:"
if [ -f "package.json" ]; then
cat package.json | jq '.dependencies["eslint-config-egg"], .devDependencies["eslint-config-egg"]'
fi
# Look for any other ESLint configuration files
echo -e "\nSearching for other ESLint config files:"
fd -e js -e json -e yaml -e yml -e cjs | rg -i 'eslint'
# Check for TypeScript-specific ESLint rules in the codebase
echo -e "\nChecking for TypeScript-specific ESLint rules:"
rg "@typescript-eslint" -A 2
Length of output: 713
.github/workflows/pkg.pr.new.yml (1)
17-23: 🛠️ Refactor suggestion
Add verification steps and verify pkg-pr-new usage
The build and publish process needs additional safeguards:
- Missing crucial verification steps before publishing
- No explicit error handling
- Need to verify pkg-pr-new configuration
Add verification steps:
- name: Install dependencies
run: npm install
+ - name: Lint
+ run: npm run lint --if-present
+
+ - name: Test
+ run: npm test
+
- name: Build
run: npm run prepublishOnly --if-present
+ - name: Verify package contents
+ run: npm pack --dry-run
+
- run: npx pkg-pr-new publish
+ env:
+ NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}src/agent.ts (1)
20-32: Ensure server closure for ephemeral servers
Like in src/request.ts, ephemeral servers are started under certain conditions. Confirm these servers are closed after usage or testing to avoid resource leaks.
src/test.ts (1)
158-158: Confirm unbuffered scenario coverage
The block at line 158 tests an unbuffered response. Make sure future changes also keep unbuffered requests well-tested, as buffering can mask subtle stream issues.
test/supertest.test.ts (4)
342-355: Edge case for closed server
This test shows a server is closed before a request is made. Great coverage for connection refusal. Keep verifying that no stale references remain, preventing partial re-use of the closed server.
834-847: Proxy usage
Line 839 uses a custom Proxy object in the test. This is a unique edge case ensuring that leftover or non-Error objects are not treated as errors. Good coverage!
1128-1142: Ordering of body & status checks
The test at lines 1128–1142 ensures that a status mismatch is reported before comparing body content. This is a strong pattern preventing misleading error messages.
205-214: Ensure proper server destroys
For lines capturing a socket error (where res.destroy() is called), confirm that the server can handle this error scenario gracefully. Extended logs or specific error handling may be beneficial.
✅ Verification successful
Let me gather more context about the error handling in this test file.
Let me check how the server cleanup is handled in the test suite.
Let me check the complete implementation of the socket error test case to see how it's handling cleanup.
Socket error test case is properly implemented
The test case correctly verifies socket error handling by checking for the existence of an error when the response is destroyed. The server cleanup is handled automatically by the request(app) helper, which creates an ephemeral server instance. No additional error handling or logging is necessary as this is a specific test for socket error scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check references to app.get and res.destroy usage across tests
rg -A 5 "res.destroy"
Length of output: 249
Script:
#!/bin/bash
# Check the complete test case implementation and surrounding context
rg -B 5 -A 10 "should handle socket errors"
Length of output: 726
Script:
#!/bin/bash
# Check for server cleanup patterns in the test file
rg -B 5 -A 10 "server\.close|app\.close|\.listen\(" test/supertest.test.ts
Length of output: 2652
Script:
#!/bin/bash
# Get the complete implementation of the socket error test case
rg -B 5 -A 15 "should handle socket errors" test/supertest.test.ts
Length of output: 450
tsconfig.json (1)
6-8: Verify Node.js version compatibility with ES2022
The TypeScript target and module settings align with the PR's Node.js 18.19.0+ requirement. However, consider adding "lib": ["ES2022"] to explicitly specify the available APIs.
✅ Verification successful
ES2022 target is compatible with Node.js engine requirement
The package.json correctly specifies Node.js >= 18.19.0, which fully supports ES2022 features. The TypeScript configuration is properly aligned with the runtime environment, and explicit "lib" configuration is optional since the "target" setting already includes the appropriate library declarations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if package.json specifies the correct Node.js engine constraint
jq '.engines.node' package.json
Length of output: 44
.github/workflows/nodejs.yml (2)
14-14: LGTM! Node.js version range aligns with the breaking change.
The specified Node.js versions (18.19.0+) correctly align with the PR objective of dropping support for older versions.
12-12: Verify the reusable workflow compatibility.
The workflow uses a shared configuration from node-modules/github-actions.
✅ Verification successful
Let me decode the base64 content and analyze the reusable workflow. I'll need to check our repository's Node.js configuration to ensure compatibility.
The reusable workflow is compatible with your Node.js requirements
The reusable workflow supports Node.js versions 16, 18, 20, and 22 by default, while your package.json requires Node.js >= 18.19.0. The workflow includes all necessary steps (checkout, setup Node.js, install dependencies, lint, test, and coverage) that match your package.json scripts configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the reusable workflow exists and supports our Node.js versions
gh api repos/node-modules/github-actions/contents/.github/workflows/node-test.yml
Length of output: 5597
Script:
#!/bin/bash
# Check package.json for Node.js version requirements
fd package.json --type f --exec cat {} \;
# Check if we have any specific Node.js version configurations
fd .nvmrc --type f --exec cat {} \;
fd .node-version --type f --exec cat {} \;
Length of output: 2135
src/error/AssertError.ts (1)
5-11: LGTM! Proper error handling implementation.
The error class follows best practices:
- Properly extends Error
- Captures stack trace
- Preserves error name
src/types.ts (2)
6-7: LGTM! Clear type definitions for both HTTP versions.
The separate type definitions for HTTP/1 and HTTP/2 request listeners provide good clarity and type safety.
9-13: LGTM! Flexible App type definition.
The union type App provides good flexibility for different server implementations while maintaining type safety.
src/index.ts (1)
1-4: LGTM: Clean imports with explicit types
The imports are well-organized and use explicit type imports, which is good practice for TypeScript.
package.json (1)
60-85: Validate the dual module format configuration
The tshy and exports configuration looks correct for supporting both CJS and ESM. However, let's verify the type definitions are generated correctly.
✅ Verification successful
The dual module format configuration appears valid
The configuration is correctly set up for dual CJS/ESM support:
- TypeScript is properly configured with
NodeNextmodule system - Build uses
tshywhich is a specialized tool for dual CJS/ESM packages - The
exportsfield correctly defines both formats with their type definitions - The absence of
.d.tsfiles is expected as they will be generated during build viatshy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify type definitions will be generated correctly
attw --pack
Length of output: 108
Script:
#!/bin/bash
# Check if TypeScript configuration exists and its content
fd tsconfig.json --exec cat {}
# Check the build script in package.json
cat package.json | jq '.scripts'
# Check if there are any type definition files in the dist directory
fd '\.d\.ts$' dist/ || echo "No .d.ts files found (dist directory might not exist yet)"
Length of output: 775
| "extends": [ | ||
| "eslint-config-egg/typescript", | ||
| "eslint-config-egg/lib/rules/enforce-node-prefix" | ||
| ] |
There was a problem hiding this comment.
💡 Codebase verification
Documentation needed for ESLint configuration and TypeScript coding standards
The repository lacks documentation about linting rules and TypeScript coding standards. The evidence shows:
- Only a brief mention of TypeScript in README.md
- No CONTRIBUTING.md or other development guide files
- Active ESLint configuration and TypeScript tooling in package.json
- Complex ESLint setup with multiple rule sets being used
Consider adding:
- A CONTRIBUTING.md file explaining the linting setup
- Documentation about the
eslint-config-egg/typescriptandenforce-node-prefixrule sets - Instructions for running the lint command (
npm run lint) - TypeScript coding conventions specific to this project
🔗 Analysis chain
Consider documenting the linting rules for contributors.
Since we're transitioning to a new ESLint configuration, it would be helpful to document the key linting rules and expectations in the project's contributing guidelines.
Let's check if documentation exists:
Let me check if there are any other potential documentation files or development guides in the repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing documentation about linting rules
# Check common documentation locations
echo "Checking for linting documentation..."
for file in README.md CONTRIBUTING.md docs/CONTRIBUTING.md docs/development.md; do
if [ -f "$file" ]; then
echo "Checking $file..."
rg -i "lint|eslint|typescript|coding.?style" "$file"
fi
done
Length of output: 670
Script:
#!/bin/bash
# Search for any documentation files in the repository
echo "Searching for documentation files..."
fd -e md -e txt --exec echo "Found: {}"
# Check package.json for any lint-related scripts or dependencies
echo -e "\nChecking package.json for lint configurations..."
if [ -f "package.json" ]; then
jq '.scripts, .devDependencies' package.json
fi
Length of output: 1116
| constructor(appOrListener: App, options: RequestOptions = {}) { | ||
| if (typeof appOrListener === 'function') { | ||
| if (options.http2) { | ||
| this.#http2 = true; | ||
| this.app = http2.createServer(appOrListener as H2RequestListener); // eslint-disable-line no-param-reassign | ||
| } else { | ||
| this.app = http.createServer(appOrListener as H1RequestListener); // eslint-disable-line no-param-reassign | ||
| } | ||
| } else { | ||
| this.app = appOrListener; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Server cleanup handling needs implementation
The codebase shows no built-in cleanup mechanism for the HTTP/1 and HTTP/2 servers created in the Request constructor. While there's evidence of server closure in tests (test/supertest.test.ts), the Request class itself doesn't implement a cleanup method. To prevent resource leaks:
- Add a
close()method to the Request class to properly clean up server instances - Document the responsibility of server cleanup in the class documentation
🔗 Analysis chain
Confirm server closure logic for ephemeral ports
When creating temporary servers (HTTP/1 or HTTP/2) inside the constructor, ensure that these servers are properly closed after tests or usage. Otherwise, they may remain open if not handled externally, potentially causing resource leaks.
Let me gather more information about server closure patterns in tests and the cleanup methods.
Let me check for any server cleanup methods in the main implementation classes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Identify test or usage patterns that ensure server closure
rg -A 5 "createServer"
Length of output: 1233
Script:
#!/bin/bash
# Search for server closure patterns in tests
rg -A 5 "\.close\(\)" test/
# Search for cleanup or teardown patterns in tests
rg -A 5 "afterEach|after\(" test/
# Look for any server-related test utilities
fd "test-utils|test.setup" --type f
Length of output: 435
Script:
#!/bin/bash
# Search for cleanup or close methods in Request class
ast-grep --pattern 'class Request {
$$$
close() {
$$$
}
$$$
}'
# Search for any server cleanup methods in the codebase
rg -B 2 -A 5 "\.close\(\)" src/
Length of output: 110
| > HTTP assertions made easy via [superagent](http://github.com/ladjs/superagent). Maintained for [Forward Email](https://github.com/forwardemail) and [Lad](https://github.com/ladjs). | ||
| > Forked for TypeScript friendly |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document TypeScript and dual module format support
While the "Forked for TypeScript friendly" note is present, consider adding more detailed information about:
- TypeScript support
- Dual module format (CJS/ESM) support
- Breaking changes regarding Node.js version
Add a new section about TypeScript and module format support:
> Forked for TypeScript friendly
+
+## TypeScript & Module Format Support
+
+This fork provides:
+- Full TypeScript support with type definitions
+- Dual module format support (CommonJS and ESM)
+- Requires Node.js >= 18.19.0📝 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.
| > HTTP assertions made easy via [superagent](http://github.com/ladjs/superagent). Maintained for [Forward Email](https://github.com/forwardemail) and [Lad](https://github.com/ladjs). | |
| > Forked for TypeScript friendly | |
| > HTTP assertions made easy via [superagent](http://github.com/ladjs/superagent). Maintained for [Forward Email](https://github.com/forwardemail) and [Lad](https://github.com/ladjs). | |
| > Forked for TypeScript friendly | |
| ## TypeScript & Module Format Support | |
| This fork provides: | |
| - Full TypeScript support with type definitions | |
| - Dual module format support (CommonJS and ESM) | |
| - Requires Node.js >= 18.19.0 |
[skip ci] ## [8.0.0](v7.0.0...v8.0.0) (2024-12-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 - **New Features** - Introduced a new `TestAgent` class for enhanced HTTP and HTTP/2 requests. - Added a new `Request` class to facilitate server requests. - Implemented a new `AssertError` class for improved error handling. - Created new GitHub Actions workflows for CI and package publishing. - **Documentation** - Updated the `README.md` to reflect rebranding to `@eggjs/supertest` and revised installation instructions. - **Bug Fixes** - Enhanced error handling and type safety in tests. - **Chores** - Removed outdated configuration files and unnecessary dependencies. - Updated TypeScript configuration for stricter type checking. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#1](#1)) ([09fb755](09fb755))
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
eggjs/egg#5257
Summary by CodeRabbit
New Features
TestAgentclass for enhanced HTTP and HTTP/2 requests.Requestclass to facilitate server requests.AssertErrorclass for improved error handling.Documentation
README.mdto reflect rebranding to@eggjs/supertestand revised installation instructions.Bug Fixes
Chores