feat: allow proxy env variables

[javi11]

Include proxy standard env variables

http_proxy, https_proxy, HTTP_PROXY, HTTPS_PROXY, NO_PROXY, no_proxy

Set proxy parameter as optional since can be provided by an env variable

Closes #36

[javi11]

@delvedor do you think this pr brings value :P?

[MarkHerhold]

Looks like a great add! What does everyone else use for no_proxy if this hasn't been a part of the lib?

[franher]

Hello @delvedor do you think this PR is ready to be accepted and merged? Thank you

[MarkHerhold]

Side comment: I was curious how npm themselves solved this issue (e.g. with corporate proxies) and their (super clean) code is here: https://github.com/npm/make-fetch-happen/blob/4d7b9c7e9f243258bff8329eba0beae448304af8/lib/agent.js#L86-L102

[mbargiel]

The implementation is incorrect. The environment variable to use depends on the URL being targeted. There's no relationship between process.env.http_proxy and process.env.https_proxy. You can't determine which one to use until the target URL is known. _https_proxy is not a fallback for http_proxy (only different casing of the same env var can be considered as fallbacks) - that is a bug._ https://www.npmjs.com/package/proxy-from-env/v/1.0.0#_proxy

And it could be considered incomplete: all_proxy or ALL_PROXY could be supported too - as a fallback to any protocol, that one.

[mbargiel]

I'm also quite unsure about the no_proxy support. If no_proxy is the upstream host has a port set but it's the default port, it won't be "ignored" properly (because it wouldn't match the no_proxy definition (/host.example.com:80/.test('host.example.com') won't exclude it, as it should. None of the variable environments is a standard (least of them no_proxy!), but there are widely adopted conventions that should be followed. I'm not sure npm's implementation, although concise and relatively clear, is actually conformant to those conventions.

[mbargiel]

Before you fix the above, please consider one thing: when instantiating the HttpProxyAgent or HttpsProxyAgent, you already need to know which URL you're targeting. Using HttpsProxyAgent with a proxy returned by process.env.http_proxy makes no sense. You must use HttpProxyAgent if the URL to get is an http: URL, or HttpsProxyAgent if it's an https: URL); likewise, you must use http_proxy/HTTP_PROXY if the URL to get is an http: URL, and https_proxy/HTTPS_PROXY if the URL to get is an https: URL. For that reason, it makes a lot more sense to have the proxy computed by the user. (Or at least outside of the proxy agent classes!)

So all in all, I don't think this is a good addition to hpagent. hpagent should be an agent library. A different library could wrap it and provide a factory function that takes in a target URL and optional proxy, and returns an appropriate agent instance of the right class with the specified proxy or a proxy inferred from environment variables, if applicable. The latter could use proxy-from-env.

[javi11]

Hi @delvedor, yep sure let's close this one I will add the no proxy ;). Thanks @mbargiel and @delvedor for your review

[mbargiel]

Instead, I like the idea of the noProxy regex, this might be a great addition. Are you willing to work on it in a separate PR?

Why not use the proxy-from-env library in the client code. no_proxy support is not trivial and that library is pretty robust and tested.

Eg.

const getProxyUrl = require('proxy-from-env'). getProxyUrl

const proxy = getProxyUrl(serverUrl) // this already takes care of no_proxy
if (proxy) {
  const agent = new HttpsProxyAgent({
    // (your agent options)
    proxy
  })
  // ...
}