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

Http proxy support #366

Merged
merged 12 commits into from
Aug 24, 2016
Merged

Http proxy support #366

merged 12 commits into from
Aug 24, 2016

Conversation

bomsy
Copy link
Contributor

@bomsy bomsy commented Jul 5, 2016

  • Update documentation is specify proxy config
  • Support for process.env.http_proxy / https_proxy / HTTP_PROXY / HTTPS_PROXY

@bomsy
Copy link
Contributor Author

bomsy commented Jul 5, 2016

Should fix #107

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.876% when pulling a70434b on bomsy:http_proxy-support into 54f3a5d on mzabriskie:master.

if (config.proxy) {
options.host = config.proxy.host;
options.port = config.proxy.port;
options.path = parsed.protocol + '//' + parsed.hostname + options.path;
options.path = parsed.protocol + '//' + parsed.hostname + (parsed.port ? ':' + parsed.port : '') + options.path;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to refactor this a bit to eliminate duplication:

var proxy = config.proxy;
if (!proxy) {
 // try to retrieve proxy settings from env variables 
}

if (proxy) {
  // override options.host, options.port, and options.path
}

@nickuraltsev
Copy link
Contributor

Thank you for the PR! It looks great. I have added some comments, please take a look.

@bomsy
Copy link
Contributor Author

bomsy commented Jul 14, 2016

Thanks for the review. updates coming.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.876% when pulling d7ed28b on bomsy:http_proxy-support into 54f3a5d on mzabriskie:master.

@bomsy
Copy link
Contributor Author

bomsy commented Jul 19, 2016

@nickuraltsev updated!

var proxyEnv = parsed.protocol.slice(0, -1) + '_proxy';
var proxyUrl = process.env[proxyEnv] || process.env[proxyEnv.toUpperCase()];
if (proxyUrl) {
proxy = url.parse(proxyUrl);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use the hostname property of the URL object rather than host. The latter contains the host name and the port (e.g. 'foo.com:8080'), which is not what should be passed to http.request (see https://nodejs.org/api/http.html#http_http_request_options_callback).

var parsedProxyUrl = url.parse(proxyUrl);
proxy = {
  host: parsedProxyUrl.hostname,
  port: parsedProxyUrl.port
};

@nickuraltsev
Copy link
Contributor

@bomsy Thank you for the updates! I've just added one more comment.

@nickuraltsev
Copy link
Contributor

@bomsy ping

@bomsy
Copy link
Contributor Author

bomsy commented Aug 8, 2016

fixing the conflict now!

@bomsy bomsy force-pushed the http_proxy-support branch from 92e2524 to 65cf8c5 Compare August 9, 2016 00:04
@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.876% when pulling 65cf8c5 on bomsy:http_proxy-support into 8abe0d4 on mzabriskie:master.

@bomsy
Copy link
Contributor Author

bomsy commented Aug 9, 2016

done!

@nickuraltsev
Copy link
Contributor

Looks like I was not clear enough (sorry for that!). My suggestion was to set the proxy.host to the proxy URL's hostname:

var parsedProxyUrl = url.parse(proxyUrl);
proxy = {
  host: parsedProxyUrl.hostname,
  port: parsedProxyUrl.port
};

Replacing proxy.host with proxy.hostname would be a breaking change, so I would prefer to avoid it.

Thank you for working on this!

@nickuraltsev
Copy link
Contributor

@bomsy Hey! Are you still interested in working on this PR?

@bomsy
Copy link
Contributor Author

bomsy commented Aug 22, 2016

@nickuraltsev hey ... yeah. been busy. will get it out in a bit.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.07%) to 93.947% when pulling 338a08f on bomsy:http_proxy-support into 8abe0d4 on mzabriskie:master.

@bomsy
Copy link
Contributor Author

bomsy commented Aug 24, 2016

@nickuraltsev done!

@nickuraltsev nickuraltsev merged commit 93ae90a into axios:master Aug 24, 2016
@nickuraltsev
Copy link
Contributor

@bomsy Thank you!

@ghost
Copy link

ghost commented Nov 9, 2018

How do I tell if the version of axios I'm using has this fix?
I'm using v0.11.0 and I do set http_proxy and https_proxy before I make my request, but the script appears to ignore them.

var proxy = config.proxy;
if (!proxy) {
var proxyEnv = parsed.protocol.slice(0, -1) + '_proxy';
var proxyUrl = process.env[proxyEnv] || process.env[proxyEnv.toUpperCase()];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add change this line to var proxyUrl = process.env[proxyEnv] || process.env[proxyEnv.toUpperCase()] || process.env.all_proxy || process.env.ALL_PROXY] so that we can support all_proxy environment variables

@axios axios locked and limited conversation to collaborators May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants