Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use make-fetch-happen instead of request #3193

Merged
merged 4 commits into from Dec 29, 2021
Merged

Use make-fetch-happen instead of request #3193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a977dc
Update node-gyp, use make-fetch-happen
Oct 13, 2021
d0f027d
Do not drop strictSSL/rejectUnauthorized and do not attempt to update…
Oct 14, 2021
89647dd
Merge branch 'master' into update-node-gyp-make-fetch
xzyfer Dec 28, 2021
c7bb422
Restore and fix tests
Dec 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions package.json
View file
Open in desktop
Expand Up @@ -60,11 +60,10 @@
"get-stdin": "^4.0.1",
"glob": "^7.0.3",
"lodash": "^4.17.15",
"make-fetch-happen": "^9.1.0",
"meow": "^9.0.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
View file
Open in desktop
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') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does make-fetch-happen have a built-in way of handling timeouts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make-fetch-happen handles the timeout option that is passesed to node-fetch, that handles it according to this doc, which I assume should be similar to how request was handling that option.

Except that, for the timeout error handling, make-fetch-happen seems to have a built-in retry mechanism that could catch the ETIMEDOUT error (among others) and retry depending on the given configuration. Since we were not doing this here until now, I have not added it, but if this is something that could be useful from here, please let me know.

I'm unsure on how the timeout errors would be propagated and thus I thought this handler might become unused code, when testing timeouts (setting the timeout to 1, deleting the locale cached node-sass and running npm run install), I could produce the following error message which seems to be sufficient to me, but I might have missed the importance of having more specific messages (or handling more specific cases):

Downloading binary from https://github.com/sass/node-sass/releases/download/v6.0.1/linux-x64-93_binding.node
Cannot download "https://github.com/sass/node-sass/releases/download/v6.0.1/linux-x64-93_binding.node": 

HTTP error ERR_SOCKET_TIMEOUT request to https://github.com/sass/node-sass/releases/download/v6.0.1/linux-x64-93_binding.node failed, reason: Socket timeout

Hint: If github.com is not accessible in your location
      try setting a proxy via HTTP_PROXY, e.g. 

      export HTTP_PROXY=http://example.com:1234

or configure npm proxy via

      npm config set proxy http://example.com:8080

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
View file
Open in desktop
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
View file
Open in desktop
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