Skip to content
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

lib: Match proxy support with request module #1978

Closed
wants to merge 7 commits into from
Closed

lib: Match proxy support with request module #1978

wants to merge 7 commits into from

Conversation

imatlopez
Copy link
Contributor

@imatlopez imatlopez commented Nov 28, 2019

Checklist
  • npm install && npm test passes
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

Per #1176 (comment), closes #1751

When using the no_proxy environmental variable I noticed that some installations would fail when NODE_CONFIG_PROXY was set (or its equivalent). From inspecting this code plus how its dependency, https://github.com/request/request, I noticed it already has full proxy support. Changing env itself might lead to side effects.

@imatlopez imatlopez changed the title lib: Delegate proxy support to request module lib: Match proxy support with request module Nov 28, 2019
@rvagg
Copy link
Member

rvagg commented Nov 29, 2019

It's unfortunate we have to include so much additional logic just to get proper no_proxy support, we're basically digging a bigger hole because of the support for non-standard methods of specifying proxies. One option might be to just rip it out and tell the user to just use standard *_PROXY env vars and be done with it but that's probably going to be too painful for enterprise users that have this baked in already.

Regarding the breaking change - would it really be breaking? In this and the current version, gyp.opts.proxy clobbers whatever you have set. If you have HTTPS_PROXY set only then in the current version it'll flow through and request will handle it properly, in this version you're handling it in the same way as request would have handled it previously. When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

I could imagine some weirdness if you had a proxy setting that didn't pass our basic regex: if (/^https?:\/\//i.test(proxyUrl)) {. request does a url.parse() which I think is assumed to throw if it's bad and so there's scope for some permutation of a proxy setting in an env var that's not picked up here but is picked up by request. But, you'd get an informational message here and you're probably not doing something sensible if you're getting that anyway .. I think.

@imatlopez
Copy link
Contributor Author

but that's probably going to be too painful for enterprise users that have this baked in already

This is actually why I am here, the only way I make proxies work is to not specify them outside env var.

When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

Exactly, which is why no_proxy is never handled correctly. My initial concern was with env vars leaking elsewhere if they were changed within the program, so I ripped out all the request code related to it (including the url.parse part--which is deprecated sadly). A benefit is that proxy behavior is now set in stone if request goes away.

@imatlopez
Copy link
Contributor Author

@rvagg anything pending here?

rvagg pushed a commit that referenced this pull request Dec 15, 2019
PR-URL: #1978
Reviewed-By: Rod Vagg <rod@vagg.org>
@rvagg
Copy link
Member

rvagg commented Dec 15, 2019

landed in 3bcba2a, labelled semver-minor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants