test: run test on Node.js 23 #543
Conversation
WalkthroughThe changes in this pull request include updates to the GitHub Actions CI pipeline configuration to support an additional Node.js version (23) in the Changes
Possibly related PRs
Suggested labels
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/nodejs.yml (1 hunks)
- README.md (0 hunks)
💤 Files with no reviewable changes (1)
- README.md
🧰 Additional context used
| @@ -12,6 +12,6 @@ jobs: | |||
| uses: node-modules/github-actions/.github/workflows/node-test.yml@master | |||
| with: | |||
| os: 'ubuntu-latest, macos-latest, windows-latest' | |||
| version: '18.19.0, 18, 20, 22' | |||
| version: '18.19.0, 18, 20, 22, 23' | |||
There was a problem hiding this comment.
💡 Codebase verification
Update README to Document Node.js 23 Support
The README.md currently does not mention the supported Node.js versions. Please update the documentation to include Node.js 23 to ensure clarity for developers and users.
🔗 Analysis chain
LGTM! Consider documenting Node.js 23 support.
The addition of Node.js 23 to the test matrix is a proactive step in ensuring compatibility with the latest Node.js version. This change will help catch any potential issues early on.
Consider the following follow-up actions:
- Update the project's README or documentation to reflect support for Node.js 23.
- Verify that all project dependencies are compatible with Node.js 23.
- Monitor the CI runs to ensure Node.js 23 tests pass consistently, as it might be a very recent or pre-release version.
Let's check if the README mentions Node.js version support:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if README mentions Node.js version support
# Expected: README should mention supported Node.js versions
rg -i "node\.?js.*(version|support)" README.md
Length of output: 48
Script:
#!/bin/bash
# Description: Broadly search README.md for any mention of Node.js versions or compatibility
# Expected: Any line mentioning Node.js versions, compatibility, or support
rg -i "node\.?js\s*(version|versions|support|compatible|compatibility)"
Length of output: 130
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #543 +/- ##
==========================================
+ Coverage 94.70% 94.75% +0.05%
==========================================
Files 9 9
Lines 1208 1164 -44
Branches 281 288 +7
==========================================
- Hits 1144 1103 -41
+ Misses 60 57 -3
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
fengmk2
force-pushed
the
node-23-test
branch
from
c1943e4 to
78f5c5d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
test/options.writeStream.test.ts (1)
120-126: LGTM! Well-handled error code compatibility.The error handling gracefully supports both the legacy
ENOENTand the newERR_STREAM_UNABLE_TO_PIPEerror codes for Node.js 23+. The conditional error message assertions are appropriate for each case.Consider enhancing the comment to be more specific about the Node.js version requirements:
- // change to ERR_STREAM_UNABLE_TO_PIPE on Node.js >= 23 + // Node.js < 23 throws ENOENT, >= 23 throws ERR_STREAM_UNABLE_TO_PIPEtest/HttpClient.test.ts (1)
Line range hint
52-114: Consider cleanup and documentation improvements in the test.A few suggestions to improve the test maintainability:
- Remove or comment out the
console.logstatements- Either remove or uncomment the
sensitiveHeadersassertions- Add a comment explaining the expected connection pooling behavior with HTTP/2
- console.log(response.res.socket, response.res.timing); + // Uncomment for debugging + // console.log(response.res.socket, response.res.timing); - // assert.equal(sensitiveHeaders in response.headers, true); + assert.equal(sensitiveHeaders in response.headers, true); + // HTTP/2 allows multiplexing multiple requests over a single connection + // Expect 4 connections due to 4 parallel requests made via Promise.all assert.equal(httpClient.getDispatcherPoolStats()['https://registry.npmmirror.com'].connected, 4);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/nodejs.yml(1 hunks)test/HttpClient.test.ts(1 hunks)test/options.writeStream.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/nodejs.yml
🔇 Additional comments (2)
test/options.writeStream.test.ts (1)
120-126: Verify error handling implementation in the main code.
Let's ensure the main implementation properly handles these error codes and that other test files are updated consistently.
✅ Verification successful
Let me gather more specific information about the error handling implementation.
Based on the comprehensive results, I can now provide the final response:
Error handling implementation is correctly implemented and tested
The error handling for stream-related errors is properly implemented and tested:
- The test file correctly handles both
ENOENTandERR_STREAM_UNABLE_TO_PIPEerror codes - The implementation in
src/Request.tsincludes proper stream error handling configurations - The test suite covers various stream error scenarios including timeouts, server errors, and write stream failures
- The conditional error message check accounts for Node.js version differences appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for error handling patterns in implementation and tests
# Check main implementation for error handling
echo "Checking main implementation for error handling..."
rg -l "ERR_STREAM_UNABLE_TO_PIPE|ENOENT" src/
# Check other test files for similar patterns
echo "Checking other test files for consistency..."
rg -l "writeStream.*error" test/
Length of output: 325
Script:
#!/bin/bash
# Search for error handling patterns in the codebase
echo "Checking for error handling in implementation..."
rg -A 5 "catch.*err" src/
# Look for stream-related error handling
echo "Checking stream-related error handling..."
rg -A 5 "stream.*error|error.*stream" src/
# Check the specific test file content
echo "Examining the test file content..."
cat test/options.writeStream.test.ts
Length of output: 6456
test/HttpClient.test.ts (1)
113-113: LGTM! The updated assertion correctly reflects HTTP/2 connection pooling behavior.
The change from 1 to 4 connected clients aligns with the parallel request pattern in the test, where 4 concurrent requests are made using Promise.all. This better validates the connection pooling behavior when HTTP/2 is enabled.
Let's verify the connection pooling behavior across different Node.js versions:
✅ Verification successful
Let me gather more information about the connection pooling behavior by examining the test file and related configurations.
Let me check the actual implementation of parallel requests and HTTP/2 configuration to verify the connection behavior.
Based on the gathered evidence, I can now provide the final response:
The updated assertion correctly reflects the HTTP/2 connection pooling behavior
The change from 1 to 4 connected clients is verified to be correct because:
- The test makes exactly 4 parallel requests using
Promise.all([httpClient.request(_url), ...]) - The client is configured with
allowH2: true, enabling HTTP/2 multiplexing - The dispatcher pool stats correctly track these concurrent connections
- Other test cases in the file consistently verify connection pool stats in similar scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if connection pooling behavior is consistent across Node.js versions
# by searching for related tests and configurations
# Look for other connection pooling related tests
rg -l "getDispatcherPoolStats|connected" --type ts
# Check for HTTP/2 specific configurations
ast-grep --pattern 'new HttpClient({
$$$
allowH2: $_,
$$$
})'
Length of output: 706
Script:
#!/bin/bash
# Check the test context around the assertion
rg "getDispatcherPoolStats" test/HttpClient.test.ts -B 5 -A 5
# Look for HTTP/2 related configurations in HttpClient
ast-grep --pattern 'class HttpClient {
$$$
getDispatcherPoolStats() {
$$$
}
$$$
}'
Length of output: 1360
Script:
#!/bin/bash
# Check the Promise.all implementation and HTTP/2 setup
rg "Promise\.all" test/HttpClient.test.ts -B 10 -A 2
# Look for HTTP/2 related configurations in the client
ast-grep --pattern 'describe($_, () => {
$$$
allowH2: true,
$$$
})'
Length of output: 1714
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/fetch.ts (2)
Line range hint
232-246: Consider improving type safety in error handling and response handlingThe current implementation has a few areas where type safety could be enhanced:
- The catch clause uses
anytype- Multiple non-null assertions on
rescould be handled more elegantlyConsider applying these improvements:
- } catch (e: any) { + } catch (e: unknown) { + const error = e instanceof Error ? e : new Error(String(e)); channels.response.publish({ fetch: fetchMeta, - error: e, + error, } as FetchResponseDiagnosticsMessage); channels.fetchResponse.publish({ request: reqMeta, response: urllibResponse, - error: e, + error, } as ResponseDiagnosticsMessage); - throw e; + throw error; } + if (!res) { + throw new Error('Response is unexpectedly null'); + } - const state = Reflect.get(res!, undiciFetchSymbols.kState) as Dispatcher.ResponseData; + const state = Reflect.get(res, undiciFetchSymbols.kState) as Dispatcher.ResponseData;
Line range hint
199-220: Consider improving type definitions for urllibResponseThe double type casting (
any+RawResponseWithMeta) suggests potential type definition issues that could lead to runtime errors.Consider creating a proper type definition:
interface UrllibResponseInit { status: number; statusCode: number; statusText: string; statusMessage: string; headers: IncomingHttpHeaders; size: number; aborted: boolean; rt: number; keepAliveSocket: boolean; requestUrls: string[]; timing: typeof timing; socket: SocketInfo; retries: number; socketErrorRetries: number; } const urllibResponse: UrllibResponseInit = { status: -1, statusCode: -1, // ... rest of the properties };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
package.json(1 hunks)src/fetch.ts(1 hunks)
🔇 Additional comments (3)
package.json (2)
109-110: LGTM! Proper dual package setup
The addition of the "module" field alongside the existing "main" field correctly establishes this as a dual CommonJS/ESM package. The paths align well with the exports configuration.
Line range hint 1-110: Verify Node.js 23 compatibility
Since this PR aims to support Node.js 23, let's verify compatibility with key dependencies.
✅ Verification successful
Node.js 23 compatibility looks promising, with a minor consideration
All dependencies are compatible with Node.js 23:
- undici (>=18.17)
- ylru (>= 18.19.0)
- type-fest (>=16)
- Other dependencies (formstream, mime-types, qs) have no strict upper bounds
The package's own engine requirement (node: ">= 18.19.0") aligns well with dependencies, and there are no open Node.js 23-specific issues in undici that would affect compatibility. The only open issue found is general flakiness which isn't Node.js 23 specific.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if dependencies support Node.js 23
# Check undici's package.json for engine requirements
curl -s https://registry.npmjs.org/undici/6.19.2 | jq -r '.engines'
# Check if there are any open issues related to Node.js 23 compatibility
gh api graphql -f query='
{
search(query: "repo:nodejs/undici is:issue is:open node23", type: ISSUE) {
nodes {
... on Issue {
title
url
state
}
}
}
}'
Length of output: 593
Script:
#!/bin/bash
# Description: Check if dependencies support Node.js 23 with pagination
# Check undici's package.json for engine requirements
curl -s https://registry.npmjs.org/undici/6.19.2 | jq -r '.engines'
# Check if there are any open issues related to Node.js 23 compatibility
gh api graphql -f query='
{
search(query: "repo:nodejs/undici is:issue is:open node23", type: ISSUE, first: 10) {
nodes {
... on Issue {
title
url
state
}
}
}
}'
# Let's also check other key dependencies
for pkg in formstream mime-types qs type-fest ylru; do
echo "Checking $pkg:"
curl -s "https://registry.npmjs.org/$pkg/latest" | jq -r '.engines // "No engine restrictions"'
done
Length of output: 1547
src/fetch.ts (1)
166-166: LGTM: Type cast improves type safety
The explicit type cast to FetchOpaque enhances type checking without affecting runtime behavior, ensuring the object structure matches the interceptor's expectations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
examples/search_github.cjs (3)
Line range hint
4-4: Warning: Using deprecated GitHub API endpointThe example is using GitHub's legacy API endpoint (
/legacy/user/search). This endpoint might be deprecated or removed in the future.Consider updating the example to use the current GitHub Search API:
-urllib.request('https://api.github.com/legacy/user/search/location:china', { +urllib.request('https://api.github.com/search/users?q=location:china', {
10-11: Consider structured logging and error handlingWhile adding response.data logging is helpful, the example could benefit from better error handling and structured logging.
Consider this enhancement:
}).then(response => { - console.log(response); - console.log(response.data); + console.log('Response metadata:', { + status: response.status, + headers: response.headers, + timing: response.timing + }); + console.log('Response data:', response.data); +}).catch(error => { + console.error('Request failed:', error.message); + process.exit(1); });
Line range hint
4-12: Add TypeScript example alongside CJSSince the PR includes TypeScript documentation improvements, consider adding a TypeScript version of this example.
Create a new file
examples/search_github.tswith proper type annotations to demonstrate TypeScript usage:import urllib from '../'; import type { HttpClientResponse } from '../'; interface GitHubUser { login: string; score: number; // add other relevant fields } interface GitHubSearchResponse { items: GitHubUser[]; total_count: number; } async function searchGitHubUsers(): Promise<void> { try { const response: HttpClientResponse<GitHubSearchResponse> = await urllib.request( 'https://api.github.com/search/users?q=location:china', { dataType: 'json', timing: true, timeout: 10000, } ); console.log('Response metadata:', { status: response.status, timing: response.timing }); console.log('Found users:', response.data.total_count); } catch (error) { console.error('Request failed:', error); process.exit(1); } } searchGitHubUsers();examples/timing.cjs (2)
Line range hint
13-28: Consider improving the example implementationWhile the example demonstrates timing functionality, there are several areas for improvement:
- The recursive request pattern using
setImmediatecould lead to memory issues with largecountvalues- Error handling is missing for failed requests
- The connect timeout is hardcoded
Consider this improved implementation:
-async function request(index) { - if (index === count) { - return; - } - const res = await httpClient.request(url + '?index=' + index, { - // data: { wd: 'nodejs' }, - // dataType: 'json', - }); - console.log('---------------------------'); - console.log('No#%d: %s, content size: %d, requestUrls: %o, socket: %o, rt: %o', - index, res.statusCode, res.data.length, res.res.requestUrls, res.res.socket, res.res.rt); - console.log(res.res.timing); - // console.log(res.res.timing, res.headers); - // console.log(res.data); - index++; - setImmediate(request.bind(null, index)); +async function request(index) { + try { + if (index === count) { + return; + } + const res = await httpClient.request(url + '?index=' + index, { + timeout: [1500, 5000], // connect timeout, response timeout + }); + console.log('---------------------------'); + console.log('No#%d: %s, content size: %d, requestUrls: %o, socket: %o, rt: %o', + index, res.statusCode, res.data.length, res.res.requestUrls, res.res.socket, res.res.rt); + console.log(res.res.timing); + + // Use Promise-based delay instead of setImmediate for better control + await new Promise(resolve => setTimeout(resolve, 100)); + await request(index + 1); + } catch (err) { + console.error(`Request ${index} failed:`, err.message); + } }
Line range hint
1-28: Add JSDoc comments to improve example documentationThe example would benefit from JSDoc comments explaining its purpose and configuration options.
Add this documentation at the start of the file:
+/** + * Example demonstrating urllib timing capabilities + * Shows how to: + * - Track request timing metrics + * - Handle connection timeouts + * - Access socket information + * + * Usage: + * node timing.cjs [url] + * Default URL: https://cnodejs.org + */ const { HttpClient } = require('..');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
examples/search_github.cjs(1 hunks)examples/timing.cjs(1 hunks)test/options.compressed.test.ts(2 hunks)
🔇 Additional comments (3)
examples/timing.cjs (1)
19-19: Verify the impact of removing json dataType
The commented out dataType: 'json' line suggests that the response will be treated as raw data instead of being automatically parsed as JSON. This might affect how res.data.length works on line 23, as it would now operate on the raw response buffer instead of a parsed JSON object.
Let's verify the response handling in the codebase:
✅ Verification successful
Let me analyze the results and generate a final response based on the findings from the codebase:
Removing dataType: 'json' is safe in this example file
The commented out dataType: 'json' line in examples/timing.cjs is intentional and safe because:
- The example file is focused on demonstrating timing functionality, not data processing
- From the codebase analysis, we can see that
dataType: 'json'is primarily used to:- Set the
accept: 'application/json'header automatically - Parse the response data as JSON when received
- Set the
- The example code only uses
res.data.lengthwhich works correctly on both raw buffer and parsed JSON responses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how dataType affects response handling
rg -A 5 "dataType.*json" --type ts
Length of output: 48421
test/options.compressed.test.ts (2)
6-6: LGTM! Clean import statement.
The import statement correctly includes only the utilities that are used in the test file.
Line range hint 196-200: Refactor version check for better Node.js 23 compatibility.
The current version check has several issues:
- It's too specific to v18.19.0 and doesn't account for Node.js 23
- Using exact version comparison is fragile
- The error code handling might break on Node.js 23
Consider using semver comparison and handling error codes based on major version ranges:
- if (process.version !== 'v18.19.0') {
+ const version = process.version.slice(1); // Remove 'v' prefix
+ const major = parseInt(version.split('.')[0], 10);
+ if (major >= 23) {
+ assert.equal(err.code, 'ERR__ERROR_FORMAT_PADDING_1');
+ } else if (major === 18) {
assert.equal(err.code, 'ERR_PADDING_1');
- } else {
- assert.equal(err.code, 'ERR__ERROR_FORMAT_PADDING_1');
}Let's verify the error codes across different Node.js versions:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
test/options.timeout.test.ts (1)
60-63: LGTM! Consider adding timing documentation.The increased server delay (1500ms) provides a better margin over the timeout values [400, 500], making the test more reliable.
Consider adding a comment explaining the timing values:
const response = await urllib.request(`${_url}mock-bytes?timeout=1500`, { + // Server delay of 1500ms ensures the 500ms body timeout will trigger timeout: [ 400, 500 ], });
Summary by CodeRabbit
New Features
Documentation
urllibpackage with new sections, examples, and detailed explanations of therequestmethod and its options.API DocandResponse Objectsections for better clarity.Tests
HttpClientclass, focusing on connection statistics and client options validation.writeStreamtests, reflecting updates for Node.js version 23 and improving robustness.options.compressedtests, ensuring accurate decompression error codes.options.timeouttests to align with updated expectations.