-
Notifications
You must be signed in to change notification settings - Fork 162
feat: update minimal supported version of Node.js to v20 #638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR upgrades the minimal supported Node.js version from v14 to v20 to align with active LTS versions (20, 22, 24) and address the Node.js 18 end-of-life. The upgrade addresses Node.js 20's stricter HTTP parsing and default keep-alive behavior changes.
Changes:
- Updated minimum Node.js version requirement from >=14 to >=20
- Disabled HTTP keep-alive across test servers to prevent connection tracking issues
- Added logic to skip 51 tests on Node.js 20+ that rely on lenient HTTP parsing
- Updated error message handling for connection failures to account for Node.js 20 behavior changes
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updated engines.node requirement from >=14 to >=20 |
| .github/workflows/check.yaml | Updated test matrix to Node.js 16, 18, 20 and changed lint condition to run on Node 20 |
| test/Dockerfile | Updated base Docker image from Node 18.20.8 to Node 20.19.6 |
| test/utils/target_server.js | Disabled keep-alive on HTTP test server by setting keepAliveTimeout to 0 |
| test/server.js | Added Connection: 'close' header to test requests and disabled keep-alive on proxy servers; added Node.js version check to skip tests for stricter HTTP parsing |
| test/https-stress-test.js | Disabled keep-alive on global HTTP agent and proxy server |
| test/anonymize_proxy.js | Updated error assertion to accept both 'ECONNREFUSED' and 'socket hang up' messages |
| test/anonymize_proxy_no_password.js | Updated error assertion to accept both 'ECONNREFUSED' and 'socket hang up' messages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/check.yaml
Outdated
| strategy: | ||
| matrix: | ||
| node-version: [14, 16, 18] | ||
| node-version: [16, 18, 20] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we running checks on unsupported Node.js versions? Shouldn't this be [20, 22, 24]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. For some reason I kept this because all tests passed. But, you're probably right to drop old versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
danpoletaev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Left few suggestions that needs to be reviewed before merging
| }, | ||
| "engines": { | ||
| "node": ">=14" | ||
| "node": ">=20" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't minimal node version be 16?
It should just warn user to use node 20, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also update @types/node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to set minimal engine version to current oldest supported LTS version of Node. As of today it's Node 20.
Node 18 both active and security support period has ended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also update @types/node
Good point. Upgraded.
| @@ -1,4 +1,4 @@ | |||
| ARG NODE_IMAGE=node:18.20.8-bookworm | |||
| ARG NODE_IMAGE=node:20.19.6-bookworm | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to also update scripts/test-docker-all.sh, since there're hardcoded node versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Gonna fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
This PR upgrades minimal supported version of Node.js to v20.
Why Node.js 20?
I think for us it makes sense to support last 3 LTS versions of node. As of today they are: 24, 22 and 20.
Also I'll keep tests at Github Actions for Node.js 16, 18 and 20 (will be added) to make sure we're not breaking too much things. At least right now. However this might change in next major release.
Migration Flow
Migration from Node.js 18 to 20 wasn't complicated. Here are the main issues which we've faced:
keep-aliveby default. This should be turned off in tests explicitly.socket hang upinstead ofECONNREFUSEDfor some special casesSkipped Tests On Node.js 20
Overview
There was 2242 tests cases for Node.js v18. Currently there are 2191 (minus 51 test case) for Node.js v20. See details below:
!useUpstreamProxy:!useUpstreamProxy && !useSsl:Grand total: 34 + 17 = 51 skipped tests
Why these tests are skipped
Both tests verify the proxy's behavior when receiving malformed HTTP responses from upstream servers:
In Node.js 20, the
--insecure-http-parserflag no longer relaxes HTTP client parsing for these edge cases. The HTTP client throws parse errors before the proxy can even process them. This is a Node.js security hardening change, not a proxy bug.Close #601