Conversation
BREAKING CHANGE: Minimum supported Node.js version is now 22.0.0. - Remove patchForNode16() polyfills from src/utils.ts and src/index.ts - Remove old Node.js CI workflow and test patches - Remove polyfill devDependencies (@ungap/structured-clone, string.prototype.towellformed) - Simplify version-conditional test code (keySize, skipIf, assertions) - Update CI matrix to test Node.js 22, 24, 25 only Closes #718
📝 WalkthroughWalkthroughThis PR removes Node.js 16 and 18 support, establishing Node.js 22 as the minimum required version. Changes include deleting deprecated CI workflows, removing runtime polyfills and patches, eliminating version-conditional test logic, and simplifying TLS key generation across tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #722 +/- ##
==========================================
+ Coverage 92.77% 94.60% +1.83%
==========================================
Files 10 10
Lines 747 723 -24
Branches 233 226 -7
==========================================
- Hits 693 684 -9
+ Misses 51 36 -15
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the minimum required Node.js version to 22.0.0 and removes legacy compatibility code, including the patchForNode16 utility, version-specific test skips, and polyfill dependencies like @ungap/structured-clone. Test configurations and server fixtures have been simplified to utilize modern Node.js APIs directly. I have no feedback to provide.
There was a problem hiding this comment.
Pull request overview
This PR updates the project’s runtime baseline to Node.js >= 22, removing legacy polyfills and simplifying tests/CI that previously supported older Node versions.
Changes:
- Raise the minimum supported Node.js version to >= 22.0.0 and remove Node 20/18/16 compatibility code.
- Remove Node 16-era polyfill patching from runtime and test setup, and delete the old-versions CI workflow.
- Simplify version-conditional test logic and drop polyfill-only devDependencies.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.ts | Removes Vitest setupFiles now that legacy runtime patching is no longer needed. |
| test/utils.ts | Removes nodeMajorVersion() helper used for old-version branching. |
| test/urllib.options.rejectUnauthorized-false.test.ts | Removes Node-version keysize conditional; fixes key size at 2048. |
| test/setup.ts | Deletes Node 16 patch bootstrap for tests. |
| test/patch-for-node16-18.cjs | Deletes polyfills needed only for Node 16/18 test runs. |
| test/options.writeStream.test.ts | Removes Node 16 skip and legacy comments tied to old stream behavior. |
| test/options.timeout.test.ts | Removes Node-version keysize conditional; fixes key size at 2048. |
| test/options.fixJSONCtlChars.test.ts | Removes Node-version-specific comment; keeps assertion general. |
| test/options.dataType.test.ts | Removes Node-version branching in JSON parse error assertions. |
| test/options.compressed.test.ts | Removes Node-version branching for brotli error code assertion. |
| test/fixtures/server.ts | Removes Node 16 connection-tracking fallback; uses closeAllConnections() directly; fixes HTTPS key size at 2048. |
| test/diagnostics_channel.test.ts | Removes Node-version keysize conditional; fixes key size at 2048. |
| test/HttpClient.test.ts | Removes Node 16 skips and lookup branching tied to older Node DNS behavior. |
| src/utils.ts | Removes patchForNode16() and related imports/polyfill logic from production code. |
| src/index.ts | Stops invoking patchForNode16() at module init. |
| src/HttpAgent.ts | Updates comments related to DNS lookup behavior under newer Node versions. |
| pnpm-lock.yaml | Removes polyfill-related dependencies from the lockfile. |
| package.json | Bumps engines.node to >= 22.0.0, removes polyfill devDeps, and simplifies prepare. |
| .github/workflows/nodejs.yml | Updates CI Node matrix to 22/24/25. |
| .github/workflows/nodejs-old-versions.yml | Deletes old Node 16/18 CI workflow. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/HttpClient.test.ts (1)
128-183:⚠️ Potential issue | 🟠 MajorClose the HTTP/2 server in this test to prevent resource leaks.
The test creates a server at line 151 but never closes it, leaving an open handle. This can cause test flakiness and resource leaks. Add
server.close()in afinallyblock to ensure cleanup.Suggested fix
server.listen(0); await once(server, 'listening'); - + try { const httpClient = new HttpClient({ allowH2: true, connect: { rejectUnauthorized: false, }, }); const url = `https://localhost:${(server.address() as AddressInfo).port}`; let response = await httpClient.request<string>(url, { dataType: 'text', headers: { 'x-my-header': 'foo', }, }); assert.equal(response.status, 200); assert.equal(response.headers['x-custom-h2'], 'hello'); assert.equal(response.data, 'hello h2!'); await sleep(200); response = await httpClient.request<string>(url, { dataType: 'text', headers: { 'x-my-header': 'foo2', }, }); assert.equal(response.status, 200); assert.equal(response.headers['x-custom-h2'], 'hello'); assert.equal(response.data, 'hello h2!'); + } finally { + await new Promise<void>((resolve, reject) => { + server.close((err?: Error) => (err ? reject(err) : resolve())); + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/HttpClient.test.ts` around lines 128 - 183, The test "should not exit after other side closed error" creates a server (variable server) but never closes it; wrap the request logic in a try/finally so you always call server.close() in the finally block (and optionally await the 'close' event with once(server, 'close')) to ensure the HTTP/2 server is cleaned up and no open handles remain; keep the existing assertions and HttpClient.request usage as-is and only add the finally cleanup around them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/HttpAgent.ts`:
- Around line 44-49: Update the inline comments in HttpAgent.ts to correctly
reflect dns.lookup behavior: change the comment near the args destructuring
(refs: args[0] -> address, args[1] -> family) to say "address will be a string
when dnsOptions.all defaults to false" and update the later comment inside the
options.checkAddress block to "dnsOptions.all defaults to false, dns.lookup
returns a single address string"; keep the surrounding logic unchanged
(references: address, family, args, options.checkAddress, dns.lookup,
dnsOptions.all).
In `@test/fixtures/server.ts`:
- Around line 441-445: The shutdown order in closeServer() is inverted; call
server.close(callback) first to stop accepting new connections, then call
server.closeAllConnections() to terminate existing ones. Update the
closeServer() function so it invokes server.close(resolve) (or wraps
server.close in a Promise) before calling server.closeAllConnections(),
referencing the existing closeServer, server.close, and
server.closeAllConnections symbols.
---
Outside diff comments:
In `@test/HttpClient.test.ts`:
- Around line 128-183: The test "should not exit after other side closed error"
creates a server (variable server) but never closes it; wrap the request logic
in a try/finally so you always call server.close() in the finally block (and
optionally await the 'close' event with once(server, 'close')) to ensure the
HTTP/2 server is cleaned up and no open handles remain; keep the existing
assertions and HttpClient.request usage as-is and only add the finally cleanup
around them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cee608f9-d2cd-46c6-9857-21970ac9d4cd
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.github/workflows/nodejs-old-versions.yml.github/workflows/nodejs.ymlpackage.jsonsrc/HttpAgent.tssrc/index.tssrc/utils.tstest/HttpClient.test.tstest/diagnostics_channel.test.tstest/fixtures/server.tstest/options.compressed.test.tstest/options.dataType.test.tstest/options.fixJSONCtlChars.test.tstest/options.timeout.test.tstest/options.writeStream.test.tstest/patch-for-node16-18.cjstest/setup.tstest/urllib.options.rejectUnauthorized-false.test.tstest/utils.tsvite.config.ts
💤 Files with no reviewable changes (8)
- src/index.ts
- test/setup.ts
- test/options.fixJSONCtlChars.test.ts
- test/utils.ts
- vite.config.ts
- src/utils.ts
- test/patch-for-node16-18.cjs
- .github/workflows/nodejs-old-versions.yml
| // address will be array when dnsOptions.all is set to default | ||
| const address = args[0]; | ||
| const family = args[1]; | ||
| if (err) return (callback as any)(err, address, family); | ||
| if (options.checkAddress) { | ||
| // dnsOptions.all set to default on Node.js >= 20, dns.lookup will return address array object | ||
| // dnsOptions.all set to default, dns.lookup will return address array object |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the default value of dnsOptions.all in Node.js dns.lookup
# Test the actual behavior of dns.lookup with default options
node --eval "
const dns = require('node:dns');
console.log('Testing dns.lookup with default options (no all parameter):');
dns.lookup('example.com', {}, (err, address, family) => {
console.log('Type of address:', typeof address);
console.log('Is array:', Array.isArray(address));
console.log('Address value:', address);
console.log('Family:', family);
});
setTimeout(() => {
console.log('\nTesting dns.lookup with all: true:');
dns.lookup('example.com', { all: true }, (err, addresses) => {
console.log('Type of addresses:', typeof addresses);
console.log('Is array:', Array.isArray(addresses));
console.log('Addresses:', addresses);
});
}, 1000);
"Repository: node-modules/urllib
Length of output: 478
🌐 Web query:
What is the default value of the 'all' option in Node.js dns.lookup() function? Does it default to true or false?
💡 Result:
The default value of the 'all' option in Node.js dns.lookup is false. When 'all' is false (default), the callback receives a single address; when true, it receives an array of all resolved addresses.
Citations:
- 1: https://nodejs.org/api/dns.html
- 2: https://stackoverflow.com/questions/33594431/whichs-options-in-dns-lookup-to-get-all-addresses
- 3: https://www.geeksforgeeks.org/node-js/node-js-dns-lookup-method/
Fix comments to accurately describe dns.lookup behavior.
Lines 44 and 49 incorrectly state that address will be an array when dnsOptions.all is at its default. The all option defaults to false, which means dns.lookup returns a single address string by default. The array behavior only occurs when all is explicitly set to true.
Update the comments to accurately reflect this:
- Line 44: "address will be a string when dnsOptions.all defaults to false"
- Line 49: "dnsOptions.all defaults to false, dns.lookup returns a single address string"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HttpAgent.ts` around lines 44 - 49, Update the inline comments in
HttpAgent.ts to correctly reflect dns.lookup behavior: change the comment near
the args destructuring (refs: args[0] -> address, args[1] -> family) to say
"address will be a string when dnsOptions.all defaults to false" and update the
later comment inside the options.checkAddress block to "dnsOptions.all defaults
to false, dns.lookup returns a single address string"; keep the surrounding
logic unchanged (references: address, family, args, options.checkAddress,
dns.lookup, dnsOptions.all).
| closeServer() { | ||
| if (hasCloseAllConnections) { | ||
| (server as any).closeAllConnections(); | ||
| } else { | ||
| // console.log('Closing %d http connections', connections.length); | ||
| for (const connection of connections) { | ||
| connection.destroy(); | ||
| } | ||
| } | ||
| server.closeAllConnections(); | ||
| return new Promise((resolve) => { | ||
| server.close(resolve); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js server.closeAllConnections() vs server.close() order shutdown
💡 Result:
For graceful shutdown in Node.js HTTP servers (v18.2.0+), first call server.close to stop accepting new connections and allow active requests to complete, then call server.closeAllConnections to forcefully close any remaining connections (including idle keep-alive ones). This ensures the server.close callback fires promptly without hanging on persistent connections. Example: process.on('SIGTERM', => { server.close((err) => { if (err) { console.error(err); process.exit(1); } console.log('Connections drained'); // Additional cleanup (DB, etc.) process.exit(0); }); server.closeAllConnections; // Force close remaining sockets }); server.close: Stops new connections, waits for all connections to naturally close after active requests finish (may hang on idle keep-alive). Callback fires when no connections remain. server.closeAllConnections: Forcefully closes/destroys all server connections (idle and active HTTP ones, not upgraded WebSockets). Use after close for non-graceful draining of lingerers. No callback; synchronous effect on connection count. For truly graceful: Use server.closeIdleConnections after close instead, to close only idle ones while letting active finish. Always set a timeout to force process.exit if shutdown hangs.
Citations:
- 1: https://runebook.dev/en/articles/node/http/servercloseallconnections
- 2: https://stackoverflow.com/questions/18874689/force-close-all-connections-in-a-node-js-http-server
- 3: https://stackoverflow.com/questions/77196622/does-server-closeallconnections-also-close-idle-connections
- 4: http: server.closeAllConnections does not destroy upgraded (web)sockets nodejs/node#53536
- 5: https://stackoverflow.com/questions/14626636/how-do-i-shut-down-a-node-js-https-server-immediately
- 6: https://stackoverflow.com/questions/77247270/closing-a-node-http-server-gracefully-server-close-callback-not-firing
- 7: https://stackoverflow.com/questions/28053659/why-is-server-close-callback-never-invoked
- 8: https://loke.dev/blog/node-graceful-shutdown-dropped-connections
- 9: http: added closeAllConnections and closeIdleConnections to http.server nodejs/node#42812
🌐 Web query:
Node.js http.Server.close() and closeAllConnections() correct sequence
💡 Result:
The correct sequence for shutting down a Node.js http.Server using close and closeAllConnections depends on the desired behavior: graceful (allow in-flight requests to complete) vs forceful (immediate closure). For graceful shutdown (recommended for production): 1. Call server.close([callback]) - Stops accepting new connections, allows active requests to finish and idle keep-alive connections to drain naturally. 2. Optionally, after calling close, call server.closeIdleConnections (if available, v18.2.0+) to close idle keep-alive connections faster without interrupting active requests. For forceful shutdown (e.g., tests, immediate termination): 1. Call server.closeAllConnections - Force-closes ALL connections (idle and active), then 2. Call server.close([callback]) - Ensures no new connections and triggers the 'close' event/callback once drained. Example (forceful): server.close( => { console.log('Server closed'); }); server.closeAllConnections; Example (graceful): server.close( => { console.log('Server closed'); }); // Optional: server.closeIdleConnections; closeAllConnections was added in v18.2.0. Prior versions require manual socket tracking via 'connection' event. Since today's date is 2026-03-29 and current Node.js is v25+, these methods are available. Use graceful shutdown unless immediate closure is required, as closeAllConnections interrupts active requests.
Citations:
- 1: https://nodejs.org/api/http.html
- 2: https://stackoverflow.com/questions/14626636/how-do-i-shut-down-a-node-js-https-server-immediately
- 3: https://stackoverflow.com/questions/18874689/force-close-all-connections-in-a-node-js-http-server
- 4: https://nodejs.org/api/https.html
- 5: https://nodejs.org/download/release/v22.12.0/docs/api/http.html
- 6: http: added closeAllConnections and closeIdleConnections to http.server nodejs/node#42812
🏁 Script executed:
cat -n test/fixtures/server.ts | sed -n '438,448p'Repository: node-modules/urllib
Length of output: 462
Swap the shutdown order in closeServer().
server.close() must be called before server.closeAllConnections(). The current code closes existing connections first, leaving the server still accepting new connections until close() is called. This creates a race condition where a connection can slip in between the two calls and cause teardown to hang unexpectedly.
🛠️ Proposed fix
closeServer() {
- server.closeAllConnections();
return new Promise((resolve) => {
server.close(resolve);
+ server.closeAllConnections();
});
},📝 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.
| closeServer() { | |
| if (hasCloseAllConnections) { | |
| (server as any).closeAllConnections(); | |
| } else { | |
| // console.log('Closing %d http connections', connections.length); | |
| for (const connection of connections) { | |
| connection.destroy(); | |
| } | |
| } | |
| server.closeAllConnections(); | |
| return new Promise((resolve) => { | |
| server.close(resolve); | |
| }); | |
| closeServer() { | |
| return new Promise((resolve) => { | |
| server.close(resolve); | |
| server.closeAllConnections(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/fixtures/server.ts` around lines 441 - 445, The shutdown order in
closeServer() is inverted; call server.close(callback) first to stop accepting
new connections, then call server.closeAllConnections() to terminate existing
ones. Update the closeServer() function so it invokes server.close(resolve) (or
wraps server.close in a Promise) before calling server.closeAllConnections(),
referencing the existing closeServer, server.close, and
server.closeAllConnections symbols.
Summary
patchForNode16()polyfills from production code (src/utils.ts,src/index.ts).github/workflows/nodejs-old-versions.yml) and test patches (test/patch-for-node16-18.cjs,test/setup.ts)@ungap/structured-clone,string.prototype.towellformed)Test plan
vp checkpasses (lint, format)vp test runpasses (40 test files, 202 tests)Closes #718
🤖 Generated with Claude Code
Summary by CodeRabbit