replace https with request to allow for proxy configuration

[chewiebug]

I would like to use "gulp --verify" behind a proxy server. The current implementation does not seem to be able to support a proxy configuration.

Solution: Replace "https" by "request" in get-blacklist.js to allow for proxy configuration.

[chewiebug]

Test failure on travis-ci was due to a timeout executing "gulp --verify". I assume, that it has nothing to do with the changes in my pull request, since it didn't fail on appveyor nor in any other nodejs versions.

Would you still like me to improve something?

[phated]

We're not changing to request, sorry. It's a massive dependency that has vulnerabilities all the time. If you'd like to manually add proxy support in a new PR, we'll review.

[chewiebug]

Hi Blaine Ok, I'll give it a try. It will only support that kind of proxy that I have to use at work, though, because I won't be able to test any other. Does that sound ok? Best regards, Jörg

[yocontra]

@chewiebug Could easily implement this behavior https://github.com/request/request#controlling-proxy-behaviour-using-environment-variables using https://github.com/TooTallNate/node-proxy-agent (http_proxy, https_proxy, and no_proxy env vars - dont think we need to fuss with the other ones)

[yocontra]

It looks like they broke that out as well - https://www.npmjs.com/package/tunnel-agent

[yocontra]

Just curious - why don't you configure your proxy across your whole OS?

[chewiebug]

@contra Thank you very much for the hint concerning https://github.com/TooTallNate/node-proxy-agent! I'll try that package. Just curious - why don't you configure your proxy across your whole OS? On my local machine, I can do that. On our ci server I can't configure much. I need to be able to tell all tools to use the proxy.

[phated]

I'd prefer tunnel-agent over node-proxy-agent (which adds a bunch of deps). I also think we should use add configuration options to the .gulp.js config files instead of using environment variables. Maybe @sttk can jump in and help out (guidance or implementation)?

[chewiebug]

I see, that "tunnel-agent" is very small with just one dependency. But other than that, I have troubles finding more advantages. Looking at its github page, I have a doubts about this package: - it feels abandoned (last commit over 5 Mar 2017), since then, no reaction on any issues or pull requests - to fix a vulnerability, the current github version would need to be published to npmjs, no reaction (request/tunnel-agent#41). - I am not sure, if it will run on node 8 (request/tunnel-agent#23) - CONNECT method does not seem to be used in the right way (request/tunnel-agent#32). - it is completely undocumented (seems to be a copy of https://github.com/koichik/node-tunnel without reference to the original project - license is changed) I have tried to use "tunnel-agent" to proxy connection to the blacklist file - I failed. This does not mean, that it is impossible, I am not that good a Javascript developer. But still. Using "node-proxy-agent" for connecting to gulpjs.com was easy. Do you insist on using "tunnel-agent"? Then I'll need help using it. The examples on https://github.com/koichik/node-tunnel didn't help. I keep getting "tunneling socket could not be established, cause=connect ECONNREFUSED". Am I maybe experiencing the issue documented in issue 32?

[phated]

@chewiebug proxy-agent pulls together a ton of different modules but we're just using it for https - so you'd want to look at the https://github.com/TooTallNate/node-https-proxy-agent subdependency.

[chewiebug]

@phated: I have added an implementation using https-proxy-agent. Is this the right direction? chewiebug@236948e It is still using process.env.http(s)_proxy to configure the proxy. You mentioned, that you would prefer to use configuration via .gulp.js (https://github.com/gulpjs/gulp-cli#configuration). Does this mean, that a new flag (e.g. flags.proxy) should be introduced and "opts" (or opts.proxy) be passed to "getBlacklist()"? Should configuration via .gulp.js and via cli flag be supported?

[chewiebug]

I see, that adding a "proxy" option to "cli-options.js" is quite easy: chewiebug@cf79848 What do you think?

[sttk]

@chewiebug I think proxy configuration should be added not to cli option but .gulp.js config file as @phated saying.

To pass configurations specified in .gulp.js file into getBlacklist function, you should append cfg (or necessary properties in cfg) at this line to arguments of getBlacklist here.

[chewiebug]

Thank you very much for he detailed explanation! I suppose, I'll have to add the new flag to https://github.com/gulpjs/gulp-cli/blob/master/lib/shared/config/cli-flags.js, before I can use it, as you described. I'll look into configuration via .gulp.js next week. I am on holidays for a few days, now.

[sttk]

@chewiebug Since cli-flags.js merges configurations in .gulp.js to cli options, adding to cli-flags.js is needed only when you make new configuration available via both .gulp.js and cli option. But for only .gulp.js, it's not needed.

[chewiebug]

@sttk thank you very much for the additional clarification! I'll give it a try when I am back.

[chewiebug]

I have pushed a branch using https-proxy-agent supporting .gulp.* for configuration of the proxy: master...chewiebug:feature/add-proxy-support-for-verify

I see one big catch: https-proxy-agent:2.2.1 requires node >= 4. So this won't run on node 0.10 / 0.12 (a lot of unittests fail).

I see the following options to address this:

1. use https-proxy-agent:1.0.0; this is the last version to support node < 4
2. look for a different package, which supports node 0.10 / 0.12 in its latest version (hints appreciated)
3. copy tunnel-agent into gulp-cli's code base and try to make it run (not sure, if I'd succeed)
4. drop support of node < 4 for gulp-cli

What do you think? Do you see other options, that would be a better fit?