Merge pull request #1789 from sass/fix/download-proxy-opts

Fix proxy config for binary download

scripts/install.js

@@ -9,9 +9,7 @@ var fs = require('fs'),
   sass = require('../lib/extensions'),
   request = require('request'),
   log = require('npmlog'),
-  pkg = require('../package.json'),
-  proxy = require('./util/proxy'),
-  userAgent = require('./util/useragent');
+  downloadOptions = require('./util/downloadoptions');
 
 /**
  * Download file, if succeeds save, if not delete
@@ -50,19 +48,10 @@ function download(url, dest, cb) {
     return response.statusCode >= 200 && response.statusCode < 300;
   };
 
-  var options = {
-    rejectUnauthorized: false,
-    proxy: proxy(),
-    timeout: 60000,
-    headers: {
-      'User-Agent': userAgent(),
-    }
-  };
-
   console.log('Start downloading binary at', url);
 
   try {
-    request(url, options, function(err, response) {
+    request(url, downloadOptions(), function(err, response) {
       if (err) {
         reportError(err);
       } else if (!successful(response)) {

scripts/util/downloadoptions.js

+var proxy = require('./proxy'),
+  userAgent = require('./useragent');
+
+/**
+ * The options passed to request when downloading the bibary
+ *
+ * There some nuance to how request handles options. Specifically
+ * we've been caught by their usage of `hasOwnProperty` rather than
+ * falsey checks. By moving the options generation into a util helper
+ * we can test for regressions.
+ *
+ * @return {Object} an options object for request
+ * @api private
+ */
+module.exports = function() {
+  var options = {
+    rejectUnauthorized: false,
+    timeout: 60000,
+    headers: {
+      'User-Agent': userAgent(),
+    }
+  };
+
+  var proxyConfig = proxy();
+  if (proxyConfig) {
+    options.proxy = proxyConfig;
+  }
+
+  return options;
+};

test/downloadoptions.js

+var assert = require('assert'),
+  ua = require('../scripts/util/useragent'),
+  opts = require('../scripts/util/downloadoptions');
+
+
+describe('util', function() {
+  describe('downloadoptions', function() {
+    describe('without a proxy', function() {
+      it('should look as we expect', function() {
+        var expected = {
+          rejectUnauthorized: false,
+          timeout: 60000,
+          headers: {
+            'User-Agent': ua(),
+          }
+        };
+
+        assert.deepEqual(opts(), expected);
+      });
+    });
+
+    describe('with an npm config proxy', function() {
+      var proxy = 'http://test.proxy:1234';
+
+      before(function() {
+        process.env.npm_config_proxy = proxy;
+      });
+
+      after(function() {
+        delete process.env.npm_config_proxy;
+      });
+
+      it('should look as we expect', function() {
+        var expected = {
+          rejectUnauthorized: false,
+          proxy: proxy,
+          timeout: 60000,
+          headers: {
+            'User-Agent': ua(),
+          }
+        };
+
+        assert.deepEqual(opts(), expected);
+      });
+    });
+
+    describe('with an env proxy proxy', function() {
+      var proxy = 'http://test.proxy:1234';
+
+      before(function() {
+        process.env.HTTP_PROXY = proxy;
+      });
+
+      after(function() {
+        delete process.env.HTTP_PROXY;
+      });
+
+      it('should look as we expect', function() {
+        var expected = {
+          rejectUnauthorized: false,
+          timeout: 60000,
+          headers: {
+            'User-Agent': ua(),
+          }
+        };
+
+        assert.deepEqual(opts(), expected);
+      });
+    });
+  });
+});