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

enable proxy if https_proxy taken by env #39

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

muddydixon
Copy link

Abstract

enable proxy if https_proxy taken by env.

@patapizza
Copy link
Member

I would have expected node-fetch to handle it directly (like request does).

Anyway, can you handle the following env vars:

  • HTTP_PROXY / http_proxy
  • HTTPS_PROXY / https_proxy
  • NO_PROXY / no_proxy

Thanks!

@patapizza patapizza mentioned this pull request Jun 9, 2016
@muddydixon
Copy link
Author

i see, i'll try it.

* ignore cases of http/s_proxy
* not apply if host include no_proxy
@muddydixon
Copy link
Author

got it. please check it

const getProxyAgent = (wit_url) => {
const url = Url.parse(wit_url);
const proxy = url.protocol === "http:" ?
process.env.http_proxy || process.env.HTTP_PROXY || null :
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would avoid the || null, for we are only checking for falseyness below

Copy link
Contributor

@stopachka stopachka left a comment

Choose a reason for hiding this comment

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

cool! there seems to be conflicts though. Apologies it has taken so long to review @muddydixon. Will merge as soon as this is rebased

process.env.http_proxy || process.env.HTTP_PROXY || null :
process.env.https_proxy || process.env.HTTPS_PROXY || null;
const noProxy = process.env.no_proxy || process.env.NO_PROXY;
if(!proxy) return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe we can rewrite like so ->

const shouldIgnore = noProxy && noProxy.indexOf(url.hostname > -1

if (proxy && !shouldIgnore) {
  return new HttpsProxyAgent(proxy)
}

@muddydixon
Copy link
Author

thx, fixed it

@patrick246
Copy link

Is there any progress on this? We need proxy support and would really like to use the official package.

@muddydixon
Copy link
Author

I fixed it above commit. you check it?
Or adjust master ?

@patrick246
Copy link

I wanted to know when this will be merged into master. Currently I have reimplemented muddydixon's solution in my own repository and we're using that, as the merge conflicts were too big to merge easily. For the future it would be nice if the official module supported this. My fork is currently on top of of master so it could be merged easily.

@ultrafez
Copy link

Hi, I found this issue as I need to use this package behind an HTTP proxy too. I'd be very keen on this PR being merged - @muddydixon, would you be able to rebase to fix conflicts so it can be merged? Currently I'm trying to work around this issue by using iptables to intercept requests to Wit's API which isn't ideal!

@JeSuisUnCaillou
Copy link

I think I'm not the only one interested by this, but I don't feel like I have the knowledge to finish the rebase of this branch.
Can this fix for handling proxies be merged now ? What is missing ?

@patapizza
Copy link
Member

I'll surface this back with the team to see if we can prioritize it.

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

Successfully merging this pull request may close these issues.

6 participants