upgrade https-proxy-agent #747
Conversation
|
@cclauss if you're able to give this one a look, this is the last of the libraries needing to be upgraded |
|
@cclauss I just fixed the merge conflict on this one, so it should be ready to go now |
lib/install.js
Outdated
| @@ -56,11 +56,13 @@ function place_binary(uri, targetDir, opts, callback) { | |||
| process.env.npm_config_proxy; | |||
| let agent; | |||
| if (proxyUrl) { | |||
| const ProxyAgent = require('https-proxy-agent'); | |||
| agent = new ProxyAgent(proxyUrl); | |||
| // TODO: replace with undici's ProxyAgent | |||
There was a problem hiding this comment.
I have some minor concerns about using a different proxy agent that I will bring up in #743.
The gist is that npm and node-gyp use the same http stack by design so that historically things like env vars are picked up the exact same. These concerns might be outdated and I know at least this specific one about env vars is.
My only feedback in this PR is to remove these comments and we can address all of it in the other PR.
There was a problem hiding this comment.
updated. thanks!
benmccann
force-pushed
the
proxy-upgrade
branch
2 times, most recently
from
e37223e to
12c2b08
Compare
|
@lukekarrys I've rebased this PR against |
|
@cclauss @lukekarrys could we merge this PR? thanks! |
benmccann
force-pushed
the
proxy-upgrade
branch
from
e8465b6 to
b49baa2
Compare
|
@cclauss if you could take a look at this one, it's the last one I'd like to get in before our next release as it's the last outdated dependency that we have |
|
Thanks. I am glad you are now a maintainer. FInally, things are moving foreword. |
|
that's great! thank you so much!! I think that's pretty much all the code changes we need to try for another release. It seems only the changelog and release tooling are left. I just updated the changelog PR if you're able to take a look at it Happy Thanksgiving! |
closes #706