ref(npm): Add support for proxies in the NPM installer

[edcs]

When installing the CLI with NPM or Yarn behind a proxy server, Node.js fails to complete the download because no proxy is configured. This PR adds in https-proxy-agent (https://www.npmjs.com/package/https-proxy-agent) as a dependancy and updates the HTTPS request options so that the download can succeed.

Please could someone who is also working behind a proxy test this? I believe it to be working, but for some reason the final download from AWS 401s. I think this is because of the proxy configuration at my end as the changes I've made are as per the documentation of the agent package.

References: #228

[jan-auer]

Awesome, thanks for putting this together. The code looks good.

I'll try to perform a test myself a little later. The 401 is indeed strange since it's not even mentioned in the S3 documentation.

[jan-auer]

There seems to be some bug in http-proxy-agent. I wasn't able to reproduce 401 Unauthorized but keep getting a 403 Forbidden. It happens only when going through the HttpsProxyAgent, but not when using the same proxy via curl.

[edcs]

Ok sure, I'll look at an alternative solution for this one. I think there's other packages/methods for defining a proxy server in Node.

[jan-auer]

@edcs thanks for putting so much time in this. I was finally able to spend some time debugging the error. Turns out https-proxy-agent was indeed causing a not expected Host header in the target request. Created a fix for this here: TooTallNate/proxy-agents#43

Besides, I think we can drop support for Node 0.12 and migrate to node-fetch to remove the custom (and incomplete) redirect logic. I played around with it a little bit and ended up with something like:

  var HttpsProxyAgent = require('https-proxy-agent');
  var fetch = require('node-fetch');
  var getProxyForUrl = require('proxy-from-env').getProxyForUrl;

  var proxyUrl = getProxyForUrl(downloadUrl);
  var agent = proxyUrl ? new HttpsProxyAgent(proxyUrl) : null;
  fetch(downloadUrl, { redirect: 'follow', agent: agent }).then(function(response) {
    if (response.status < 200 || response.status >= 300)
      throw new Error('Received ' + response.status + ': ' + response.statusText);

    response.body.pipe(fs.createWriteStream(outputPath, { autoClose: true, mode: '0755' }));
  });

This obviously doesn't include the progress bar yet, but already demonstrates how much simpler the code gets. Let me know if you'd like me to edit your PR or do it yourself. In any case I will clean up (i.e. refactor to ES6) and update tests afterwards, so no need for you to spend too much effort on this.

[edcs]

Amazing - thanks for picking this up! I've not had time to work on it recently. Feel free to just update this PR so all the discussion is here in the same place.

[jan-auer]

@edcs Heads up, I took the liberty to rewrite the installer based on our discussion since I had some time. Let me know what you think and feel free to edit.

Also looping in @HazAT for a second opinion. FYI - the server.close() is now refactored into a process exit event so that we can be sure it always gets called.

Let's also keep in mind that we will need to wait for TooTallNate/proxy-agents#43 until we can merge this.

[HazAT]

@jan-auer Good job!
Looks much cleaner now.
Fix CI then I'll hit approve!

[jan-auer]

Ugh, just noticed that I killed the --version check after downloading

[jan-auer]

@HazAT updated. Should have tried it at least once before committing :)

When implementing the version-check I noticed this console.log - is it really necessary?

[jan-auer]

Travis is green, I'm done from my side.

[jan-auer]

@edcs could you please confirm that this works in your setup now?

[edcs]

That's working for me now! 🎉

[jan-auer]

Thank you so much for the bug report and the PR!