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

Conversation

@bomsy
Copy link
Contributor

commented Jul 5, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2016

Should fix #107

@coveralls

This comment has been minimized.

Copy link

commented Jul 5, 2016

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;
}

This comment has been minimized.

Copy link
@nickuraltsev

nickuraltsev Jul 8, 2016

Member

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
}

}).listen(4000, function() {
// set the env variable
process.env.http_proxy = 'http://localhost:4000/';

This comment has been minimized.

Copy link
@nickuraltsev

nickuraltsev Jul 8, 2016

Member

Can setting http_proxy affect other tests (executed after this test)? If so, maybe we should reset http_proxy in tearDown.

@nickuraltsev

This comment has been minimized.

Copy link
Member

commented Jul 8, 2016

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

@bomsy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2016

Thanks for the review. updates coming.

@coveralls

This comment has been minimized.

Copy link

commented Jul 19, 2016

Coverage Status

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

@bomsy

This comment has been minimized.

Copy link
Contributor Author

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);

This comment has been minimized.

Copy link
@nickuraltsev

nickuraltsev Jul 25, 2016

Member

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

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

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

@nickuraltsev

This comment has been minimized.

Copy link
Member

commented Aug 8, 2016

@bomsy ping

@bomsy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2016

fixing the conflict now!

@bomsy bomsy force-pushed the bomsy:http_proxy-support branch from 92e2524 to 65cf8c5 Aug 9, 2016

@coveralls

This comment has been minimized.

Copy link

commented Aug 9, 2016

Coverage Status

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

@bomsy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2016

done!

@nickuraltsev

This comment has been minimized.

Copy link
Member

commented Aug 9, 2016

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

This comment has been minimized.

Copy link
Member

commented Aug 22, 2016

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

@bomsy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2016

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

@coveralls

This comment has been minimized.

Copy link

commented Aug 24, 2016

Coverage Status

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

@bomsy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2016

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nickuraltsev

This comment has been minimized.

Copy link
Member

commented Aug 24, 2016

@bomsy Thank you!

@CaffeinateOften

This comment has been minimized.

Copy link

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()];

This comment has been minimized.

Copy link
@pbdm

pbdm Dec 28, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.