-
Notifications
You must be signed in to change notification settings - Fork 39
feat: Replace agentkeepalive with native Node.js HTTP agents for proxy support
#788
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import http from 'node:http'; | ||
| import https from 'node:https'; | ||
| import os from 'node:os'; | ||
|
|
||
| import KeepAliveAgent from 'agentkeepalive'; | ||
| import type { RetryFunction } from 'async-retry'; | ||
| import retry from 'async-retry'; | ||
| import type { AxiosError, AxiosInstance, AxiosRequestConfig, AxiosResponse, InternalAxiosRequestConfig } from 'axios'; | ||
|
|
@@ -32,9 +33,9 @@ export class HttpClient { | |
|
|
||
| timeoutMillis: number; | ||
|
|
||
| httpAgent?: KeepAliveAgent; | ||
| httpAgent?: http.Agent; | ||
|
|
||
| httpsAgent?: KeepAliveAgent.HttpsAgent; | ||
| httpsAgent?: https.Agent; | ||
|
|
||
| axios: AxiosInstance; | ||
|
|
||
|
|
@@ -58,11 +59,13 @@ export class HttpClient { | |
| // is for inactive sockets, sometimes the platform would take | ||
| // long to process requests and the socket would time-out | ||
| // while waiting for the response. | ||
| const agentOpts = { | ||
| const agentOptions: http.AgentOptions = { | ||
| keepAlive: true, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I've just added commit c424e90 with these optimizations. |
||
| timeout: this.timeoutMillis, | ||
| }; | ||
| this.httpAgent = new KeepAliveAgent(agentOpts); | ||
| this.httpsAgent = new KeepAliveAgent.HttpsAgent(agentOpts); | ||
|
|
||
| this.httpAgent = new http.Agent(agentOptions); | ||
| this.httpsAgent = new https.Agent(agentOptions); | ||
| } | ||
|
|
||
| this.axios = axios.create({ | ||
|
|
||

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.
Can you please add both these modules as
externalstorsbuild.config.ts(link)? They are not used outside of Node, so we don't need them in the browser bundle. Together, they add around 250kB to the bundle size.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.
Sure thing, done in commit f986aa4
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.
Thank you!