feat(http): add HTTP2 support; (#7150)#1
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds experimental HTTP/2 support to the Axios HTTP adapter. The implementation introduces a httpVersion configuration option (1 or 2) and http2Options for passing additional session parameters, with sessions being reused across requests to the same authority.
Changes:
- Added HTTP/2 transport layer with session management and connection reuse
- Refactored test helper utilities to support both HTTP/1 and HTTP/2 server creation
- Updated adapter error handling to support HTTP/2-specific abort mechanisms
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/adapters/http.js | Implements HTTP/2 transport, session management, and protocol version validation |
| test/helpers/server.js | Adds HTTP/2 server creation capability with certificate generation and session tracking |
| test/unit/adapters/http.js | Adds comprehensive HTTP/2 test suite and updates existing tests to use refactored helpers |
| index.d.cts | Adds TypeScript definitions for httpVersion and http2Options configuration |
| package.json | Adds selfsigned package dependency for certificate generation |
| README.md | Documents the new HTTP/2 feature with usage examples |
| @@ -1,5 +1,4 @@ | |||
| 'use strict'; | |||
|
|
|||
| import { connect, constants } from 'http2'; | |||
There was a problem hiding this comment.
The 'use strict' directive was removed from the beginning of the file. Since this module uses ES6 imports and is likely treated as a module, strict mode is automatically applied. However, the project's coding guidelines explicitly require "'use strict';" at the top of files. Restore the 'use strict' directive at line 1 to maintain consistency with the project's conventions.
| options | ||
| ]; | ||
|
|
||
| entries ? this.sessions[authority].push(entry) : authoritySessions = this.sessions[authority] = [entry]; |
There was a problem hiding this comment.
The ternary operator combines assignment and mutation in a confusing way with the chained assignment authoritySessions = this.sessions[authority] = [entry]. Split this into two clear statements: one to check if entries exist and push, another to initialize the array if it doesn't exist.
| entries ? this.sessions[authority].push(entry) : authoritySessions = this.sessions[authority] = [entry]; | |
| if (entries) { | |
| this.sessions[authority].push(entry); | |
| } else { | |
| const newEntries = [entry]; | |
| this.sessions[authority] = newEntries; | |
| authoritySessions = newEntries; | |
| } |
|
|
||
| export const setTimeoutAsync = (ms) => new Promise(resolve=> setTimeout(resolve, ms)); | ||
|
|
||
| const certificate = selfsigned.generate(null, { keySize: 2048 }); |
There was a problem hiding this comment.
The certificate is generated at module load time and reused for all tests. While this improves performance, using a single certificate for all test servers on different ports may cause issues if tests run in parallel or need isolated certificates. Consider generating certificates per server instance if test isolation becomes a concern.
|
|
||
| const http2Transport = { | ||
| request(options, cb) { | ||
| const authority = options.protocol + '//' + options.hostname + ':' + (options.port || 80); |
There was a problem hiding this comment.
The default port is hardcoded to 80, but HTTP/2 is typically used over HTTPS (port 443). For HTTPS requests, this will construct an incorrect authority string. Use options.port || (options.protocol === 'https:' ? 443 : 80) to set the appropriate default port based on the protocol.
| const authority = options.protocol + '//' + options.hostname + ':' + (options.port || 80); | |
| const defaultPort = options.port || (options.protocol === 'https:' ? 443 : 80); | |
| const authority = options.protocol + '//' + options.hostname + ':' + defaultPort; |
| } | ||
|
|
||
| utils.forEach(headers, (header, name) => { | ||
| name.charAt(0) !== ':' && (http2Headers[name] = header); |
There was a problem hiding this comment.
Using charAt(0) followed by a side effect in a boolean expression is unclear. Refactor to an explicit if statement: if (name.charAt(0) !== ':') { http2Headers[name] = header; } for better readability.
| name.charAt(0) !== ':' && (http2Headers[name] = header); | |
| if (name.charAt(0) !== ':') { | |
| http2Headers[name] = header; | |
| } |
| @@ -705,7 +846,8 @@ export default isHttpAdapterSupported && function httpAdapter(config) { | |||
|
|
|||
| data.pipe(req); | |||
| } else { | |||
There was a problem hiding this comment.
The logic splits writing data and ending the request into two separate calls. For HTTP/2, if data is nullish, req.write() is skipped but req.end() is still called. Consider clarifying the intent by adding a comment explaining why this pattern differs from the previous req.end(data) approach, especially since HTTP/2 streams may have different behavior.
| } else { | |
| } else { | |
| // For HTTP/1.1 we could call `req.end(data)`, but Node's HTTP/2 `ClientHttp2Stream` | |
| // does not support passing the payload to `end()` in the same way. Keeping the write | |
| // and end calls separate ensures consistent behavior across HTTP/1.x and HTTP/2, | |
| // and still correctly terminates the stream when `data` is null or undefined. |
| @@ -2075,6 +1997,8 @@ describe('supports http with nodejs', function () { | |||
| }); | |||
|
|
|||
| describe('Rate limit', function () { | |||
There was a problem hiding this comment.
The timeout is increased to 30 seconds for the 'Rate limit' test suite, which is significantly higher than typical test timeouts. Consider adding a comment explaining why such a long timeout is necessary for these specific tests (e.g., testing rate limiting over 10 seconds requires additional buffer time).
| describe('Rate limit', function () { | |
| describe('Rate limit', function () { | |
| // Increased timeout: these tests enforce ~10s rate limiting and need extra buffer for slow/CI environments |
|
|
||
| describe('request aborting', function() { | ||
| it('should be able to abort the response stream', async function () { | ||
| //this.timeout(5000); |
There was a problem hiding this comment.
This commented-out timeout configuration should either be removed or uncommented with a clear explanation of why it's needed. Leaving commented code without context reduces maintainability.
| //this.timeout(5000); |
PR_021