Use make-fetch-happen instead of request #3193
Conversation
|
Thanks, but there is too much going on here. Dropping the rejectUnauthorized is being tracked in #3149 as a breaking change. Likewise, node-gyp/python2 dropping support will be when 12 goes EOL. |
|
Sorry for taking your time with this, I was merely trying to see if this would be reasonable enough.. while doing the changes locally it seemed okayish~~ but after creating the PR, I thought it might involve too many things that I did not understand enough. Let's try to only replace the request/fetch, I missed seeing that the strictSSL/rejectUnauthorized option was supported by |
CamilleDrapier
changed the title
| @@ -21,21 +20,8 @@ var fs = require('fs'), | |||
|
|
|||
| function download(url, dest, cb) { | |||
| var reportError = function(err) { | |||
| var timeoutMessge; | |||
|
|
|||
| if (err.code === 'ETIMEDOUT') { | |||
There was a problem hiding this comment.
Does make-fetch-happen have a built-in way of handling timeouts?
There was a problem hiding this comment.
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
test/downloadoptions.js
Outdated
| }); | ||
| }); | ||
|
|
||
| describe('with SASS_REJECT_UNAUTHORIZED set to false', function() { |
There was a problem hiding this comment.
If we restore this test I think we're ok to proceed.
|
I've rebase this on the latest master |
CamilleDrapier
force-pushed
the
update-node-gyp-make-fetch
branch
from
c603ac5
to
29ccc47
Compare
CamilleDrapier
force-pushed
the
update-node-gyp-make-fetch
branch
from
29ccc47
to
c7bb422
Compare
|
Thanks for your work @CamilleDrapier |
Fixes sass#3206 Co-authored-by: Camille Drapier <c.drapier@allm.inc> Co-authored-by: Michael Mifsud <xzyfer@gmail.com>
* [CHANGELOG](https://github.com/ljharb/qs/blob/v6.10.3/CHANGELOG.md) * The upstream library with a dependency on ~6.5.2 is `node-sass`, but that will not be addressed until sass/node-sass#3193 is included in a release. Resolves opensearch-project#1375 Signed-off-by: Tommy Markley <markleyt@amazon.com>
* [CHANGELOG](https://github.com/ljharb/qs/blob/v6.10.3/CHANGELOG.md) * The upstream library with a dependency on ~6.5.2 is `node-sass`, but that will not be addressed until sass/node-sass#3193 is included in a release. Resolves #1375 Signed-off-by: Tommy Markley <markleyt@amazon.com>
|
This was accidentally released in v7.0.2 and subsequently broke binary downloads. See #3297 |
|
Sorry about this! Not really sure what is the underlying problem, but please let me know if I can help 🙇 |
|
No worries @CamilleDrapier, it was my mistake. My comment was intended for posterity. I've opened #3300 to track fixing it before releasing 8.0.0. |
cf: #2851
Follows the implementation that was done here: #2961 ; also remove
npmlogthat seems unused from here on.Because the latest node-gyp does not support python2 (https://github.com/nodejs/node-gyp/blob/master/CHANGELOG.md#810-2021-05-28), use python3 for all nodes builds/tests. (Not sure if
src/libsass/script/ci-report-coverage:8should be affected?)