feat: HTTP(S) Proxy support for Node #1761
Conversation
Generated by 🚫 dangerJS |
|
Still need to write some tests around it though. |
| import { BaseTransport } from './base'; | ||
|
|
||
| /** Node https module transport */ | ||
| export class HTTPSTransport extends BaseTransport { | ||
| /** Create a new instance and set this.agent */ | ||
| public constructor(public options: TransportOptions) { | ||
| super(options); | ||
| this.client = new https.Agent({ keepAlive: true, maxSockets: 100 }); | ||
| const proxy = options.httpsProxy || options.httpProxy || process.env.https_proxy || process.env.http_proxy; |
There was a problem hiding this comment.
I am not sure about the order making sense here.
There was a problem hiding this comment.
From our docs:
When set a proxy can be configured that should be used for outbound requests. This is also used for HTTPS requests unless a separate https-proxy is configured.
I think that user specified options should always override env variables, thus order is like this.
| @@ -225,9 +225,17 @@ export interface SentryResponse { | |||
|
|
|||
| /** JSDoc */ | |||
| export interface TransportOptions { | |||
| [key: string]: any; | |||
There was a problem hiding this comment.
Otherwise we cannot assign httpProxy and friends to transportOptions and I wanted it to be consistent with other SDKs and not ask users to pass it directly there, eg.:
init({
transportOptions: {
httpProxy: 'http://something:8008'
}
})(which would be a better option imo, but it's not how it works in other SDKs)
kamilogorek
force-pushed
the
node-proxy
branch
from
ebd80b1
to
141b00f
Compare
kamilogorek
force-pushed
the
node-proxy
branch
from
141b00f
to
5f56f60
Compare
Fixes: #1382