Can we restart the work on overridable globalAgent? #23281
Labels
feature request
Issues that request new features to be added to Node.js.
good first issue
Issues that are suitable for first-time contributors.
http
Issues or PRs related to the http subsystem.
Comments
Trott
added
feature request
|
@nodejs/http |
bnoordhuis
added
the
good first issue
|
@bitinn If you're interested in working on this, you're welcome to. I'll add the good-first-issue label. |
|
@bnoordhuis sorry I didn't get around to do this due to my own schedule, I hope someone with same need could pick this up. |
3 tasks
illBeRoy
added a commit
to illBeRoy/node
that referenced
this issue
Dec 21, 2018
Overriding `require('http[s]').globalAgent` is now respected by
consequent requests.
In order to achieve that, the following changes were made:
1. Implmentation in `http`: `module.exports.globalAgent` is now defined
through `Object.defineProperty`. Its getter and setter return \ set
`require('_http_agent').globalAgent`.
2. Implementation in `https`: the https `globalAgent` is not the same
as `_http_agent`, and is defined in `https` module itself. Therefore,
the fix here was to simply use `module.exports.globalAgent` to support
mutation.
3. According tests were added for both `http` and `https`, where in
both we create a server, set the default agent to a newly created
instance and make a request to that server. We then assert that the
given instance was actually used by inspecting its sockets property.
Fixes: nodejs#23281
|
Hey :) this issue was labelled as a "good first issue" so I've taken the liberty to attempt and fix it. This is the relevant PR. |
addaleax
pushed a commit
that referenced
this issue
Jan 9, 2019
Overriding `require('http[s]').globalAgent` is now respected by
consequent requests.
In order to achieve that, the following changes were made:
1. Implmentation in `http`: `module.exports.globalAgent` is now defined
through `Object.defineProperty`. Its getter and setter return \ set
`require('_http_agent').globalAgent`.
2. Implementation in `https`: the https `globalAgent` is not the same
as `_http_agent`, and is defined in `https` module itself. Therefore,
the fix here was to simply use `module.exports.globalAgent` to support
mutation.
3. According tests were added for both `http` and `https`, where in
both we create a server, set the default agent to a newly created
instance and make a request to that server. We then assert that the
given instance was actually used by inspecting its sockets property.
Fixes: #23281
PR-URL: #25170
Reviewed-By: James M Snell <jasnell@gmail.com>
refack
pushed a commit
to refack/node
that referenced
this issue
Jan 14, 2019
Overriding `require('http[s]').globalAgent` is now respected by
consequent requests.
In order to achieve that, the following changes were made:
1. Implmentation in `http`: `module.exports.globalAgent` is now defined
through `Object.defineProperty`. Its getter and setter return \ set
`require('_http_agent').globalAgent`.
2. Implementation in `https`: the https `globalAgent` is not the same
as `_http_agent`, and is defined in `https` module itself. Therefore,
the fix here was to simply use `module.exports.globalAgent` to support
mutation.
3. According tests were added for both `http` and `https`, where in
both we create a server, set the default agent to a newly created
instance and make a request to that server. We then assert that the
given instance was actually used by inspecting its sockets property.
Fixes: nodejs#23281
PR-URL: nodejs#25170
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR
pushed a commit
to BridgeAR/node
that referenced
this issue
Jan 16, 2019
Overriding `require('http[s]').globalAgent` is now respected by
consequent requests.
In order to achieve that, the following changes were made:
1. Implmentation in `http`: `module.exports.globalAgent` is now defined
through `Object.defineProperty`. Its getter and setter return \ set
`require('_http_agent').globalAgent`.
2. Implementation in `https`: the https `globalAgent` is not the same
as `_http_agent`, and is defined in `https` module itself. Therefore,
the fix here was to simply use `module.exports.globalAgent` to support
mutation.
3. According tests were added for both `http` and `https`, where in
both we create a server, set the default agent to a newly created
instance and make a request to that server. We then assert that the
given instance was actually used by inspecting its sockets property.
Fixes: nodejs#23281
PR-URL: nodejs#25170
Reviewed-By: James M Snell <jasnell@gmail.com>
|
@illBeRoy Has this been released? |
BethGriggs
pushed a commit
that referenced
this issue
Apr 28, 2019
Overriding `require('http[s]').globalAgent` is now respected by
consequent requests.
In order to achieve that, the following changes were made:
1. Implmentation in `http`: `module.exports.globalAgent` is now defined
through `Object.defineProperty`. Its getter and setter return \ set
`require('_http_agent').globalAgent`.
2. Implementation in `https`: the https `globalAgent` is not the same
as `_http_agent`, and is defined in `https` module itself. Therefore,
the fix here was to simply use `module.exports.globalAgent` to support
mutation.
3. According tests were added for both `http` and `https`, where in
both we create a server, set the default agent to a newly created
instance and make a request to that server. We then assert that the
given instance was actually used by inspecting its sockets property.
Fixes: #23281
PR-URL: #25170
Reviewed-By: James M Snell <jasnell@gmail.com>
|
I think this was merged to 11 already. |
|
I have since released https://github.com/gajus/global-agent. |
BethGriggs
pushed a commit
that referenced
this issue
May 10, 2019
Overriding `require('http[s]').globalAgent` is now respected by
consequent requests.
In order to achieve that, the following changes were made:
1. Implmentation in `http`: `module.exports.globalAgent` is now defined
through `Object.defineProperty`. Its getter and setter return \ set
`require('_http_agent').globalAgent`.
2. Implementation in `https`: the https `globalAgent` is not the same
as `_http_agent`, and is defined in `https` module itself. Therefore,
the fix here was to simply use `module.exports.globalAgent` to support
mutation.
3. According tests were added for both `http` and `https`, where in
both we create a server, set the default agent to a newly created
instance and make a request to that server. We then assert that the
given instance was actually used by inspecting its sockets property.
Fixes: #23281
PR-URL: #25170
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins
pushed a commit
that referenced
this issue
May 16, 2019
Overriding `require('http[s]').globalAgent` is now respected by
consequent requests.
In order to achieve that, the following changes were made:
1. Implmentation in `http`: `module.exports.globalAgent` is now defined
through `Object.defineProperty`. Its getter and setter return \ set
`require('_http_agent').globalAgent`.
2. Implementation in `https`: the https `globalAgent` is not the same
as `_http_agent`, and is defined in `https` module itself. Therefore,
the fix here was to simply use `module.exports.globalAgent` to support
mutation.
3. According tests were added for both `http` and `https`, where in
both we create a server, set the default agent to a newly created
instance and make a request to that server. We then assert that the
given instance was actually used by inspecting its sockets property.
Fixes: #23281
PR-URL: #25170
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Is your feature request related to a problem? Please describe.
There has been quite a few discussions here (#15620, #8381, #1490) on whether nodejs core should support environment variable for http proxy, and so far the consensus has been that proxy support in core is a slippery slope. Which I agree.
But we do need a fix for this headache: when a userland module use a http library, they often don't expose
agentoption, thus blocking users from using available userland proxy module like node-proxy-agent.Describe the solution you'd like
I personally believe making
http(s).globalAgentoverridable is the best way forward:http(s).globalAgentand be confident that ALL http library will respect it.globalAgentofhttpandhttpsoverridable #11249, Modify https.globalAgent doesn't effect #9057), we should follow it through.agentor support environment variable: it's often too niche a requirement (eg. why should an oauth lib exposeagent?), and many strongly believe it to be nodejs core problem.In short: overridable globalAgent has the lowest impact of all solutions, allow for highest flexibility when it comes to different proxy types, and has userland code ready to take advantage of it.
Describe alternatives you've considered
See linked discussions.
The text was updated successfully, but these errors were encountered: