Skip to content

Commit

Permalink
Use make-fetch-happen instead of request (sass#3193)
Browse files Browse the repository at this point in the history
Fixes sass#3206

Co-authored-by: Camille Drapier <[email protected]>
Co-authored-by: Michael Mifsud <[email protected]>
  • Loading branch information
3 people authored and Thinh Huynh (Jack) committed Feb 16, 2022
1 parent 165fdc9 commit 7f93d9e
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 70 deletions.
3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,9 @@
"glob": "^7.0.3",
"lodash": "^4.17.15",
"meow": "^10.1.0",
"make-fetch-happen": "^9.1.0",
"nan": "^2.13.2",
"node-gyp": "^8.4.1",
"npmlog": "^5.0.0",
"request": "^2.88.0",
"sass-graph": "4.0.0",
"stdout-stream": "^1.4.0",
"true-case-path": "^2.2.1"
Expand Down
57 changes: 10 additions & 47 deletions scripts/install.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
var fs = require('fs'),
eol = require('os').EOL,
path = require('path'),
request = require('request'),
log = require('npmlog'),
fetch = require('make-fetch-happen'),
sass = require('../lib/extensions'),
downloadOptions = require('./util/downloadoptions');

Expand All @@ -21,21 +20,8 @@ var fs = require('fs'),

function download(url, dest, cb) {
var reportError = function(err) {
var timeoutMessge;

if (err.code === 'ETIMEDOUT') {
if (err.connect === true) {
// timeout is hit while your client is attempting to establish a connection to a remote machine
timeoutMessge = 'Timed out attemping to establish a remote connection';
} else {
timeoutMessge = 'Timed out whilst downloading the prebuilt binary';
// occurs any time the server is too slow to send back a part of the response
}

}
cb(['Cannot download "', url, '": ', eol, eol,
typeof err.message === 'string' ? err.message : err, eol, eol,
timeoutMessge ? timeoutMessge + eol + eol : timeoutMessge,
'Hint: If github.com is not accessible in your location', eol,
' try setting a proxy via HTTP_PROXY, e.g. ', eol, eol,
' export HTTP_PROXY=http://example.com:1234',eol, eol,
Expand All @@ -44,45 +30,22 @@ function download(url, dest, cb) {
};

var successful = function(response) {
return response.statusCode >= 200 && response.statusCode < 300;
return response.status >= 200 && response.status < 300;
};

console.log('Downloading binary from', url);

try {
request(url, downloadOptions(), function(err, response, buffer) {
if (err) {
reportError(err);
} else if (!successful(response)) {
reportError(['HTTP error', response.statusCode, response.statusMessage].join(' '));
fetch(url, downloadOptions()).then(function (response) {
fs.createWriteStream(dest).on('error', cb).end(response.data, cb);
console.log('Download complete');
}).catch(function(err) {
if(!successful(err)) {
reportError(['HTTP error', err.code, err.message].join(' '));
} else {
console.log('Download complete');

if (successful(response)) {
fs.createWriteStream(dest)
.on('error', cb)
.end(buffer, cb);
} else {
cb();
}
reportError(err);
}
})
.on('response', function(response) {
var length = parseInt(response.headers['content-length'], 10);
var progress = log.newItem('', length);

// The `progress` is true by default. However if it has not
// been explicitly set it's `undefined` which is considered
// as far as npm is concerned.
if (process.env.npm_config_progress === 'true') {
log.enableProgress();

response.on('data', function(chunk) {
progress.completeWork(chunk.length);
})
.on('end', progress.finish);
}
});
});
} catch (err) {
cb(err);
}
Expand Down
12 changes: 3 additions & 9 deletions scripts/util/downloadoptions.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,18 @@ var proxy = require('./proxy'),
rejectUnauthorized = require('./rejectUnauthorized');

/**
* The options passed to request when downloading the bibary
* The options passed to make-fetch-happen when downloading the binary
*
* 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
* @return {Object} an options object for make-fetch-happen
* @api private
*/
module.exports = function() {
var options = {
rejectUnauthorized: rejectUnauthorized(),
strictSSL: rejectUnauthorized(),
timeout: 60000,
headers: {
'User-Agent': userAgent(),
},
encoding: null,
};

var proxyConfig = proxy();
Expand Down
18 changes: 6 additions & 12 deletions test/downloadoptions.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,11 @@ describe('util', function() {
describe('without a proxy', function() {
it('should look as we expect', function() {
var expected = {
rejectUnauthorized: true,
strictSSL: true,
timeout: 60000,
headers: {
'User-Agent': ua(),
},
encoding: null,
};

assert.deepStrictEqual(opts(), expected);
Expand All @@ -33,13 +32,12 @@ describe('util', function() {

it('should look as we expect', function() {
var expected = {
rejectUnauthorized: true,
strictSSL: true,
proxy: proxy,
timeout: 60000,
headers: {
'User-Agent': ua(),
},
encoding: null,
};

assert.deepStrictEqual(opts(), expected);
Expand All @@ -59,12 +57,11 @@ describe('util', function() {

it('should look as we expect', function() {
var expected = {
rejectUnauthorized: true,
strictSSL: true,
timeout: 60000,
headers: {
'User-Agent': ua(),
},
encoding: null,
};

assert.deepStrictEqual(opts(), expected);
Expand All @@ -78,12 +75,11 @@ describe('util', function() {

it('should look as we expect', function() {
var expected = {
rejectUnauthorized: false,
strictSSL: false,
timeout: 60000,
headers: {
'User-Agent': ua(),
},
encoding: null,
};

assert.deepStrictEqual(opts(), expected);
Expand All @@ -97,12 +93,11 @@ describe('util', function() {

it('should look as we expect', function() {
var expected = {
rejectUnauthorized: true,
strictSSL: true,
timeout: 60000,
headers: {
'User-Agent': ua(),
},
encoding: null,
};

assert.deepStrictEqual(opts(), expected);
Expand All @@ -116,12 +111,11 @@ describe('util', function() {

it('should look as we expect', function() {
var expected = {
rejectUnauthorized: true,
strictSSL: true,
timeout: 60000,
headers: {
'User-Agent': ua(),
},
encoding: null,
};

assert.deepStrictEqual(opts(), expected);
Expand Down

0 comments on commit 7f93d9e

Please sign in to comment.