Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Npm: download the required binaries during installation #188
Npm: download the required binaries during installation #188
Changes from 12 commits
bf7b1a4
ecc4670
d0e8d2e
d74b6ef
601b7b5
12645c2
4c46696
743fde3
f83aff8
2c38489
c7b3bbb
85ae694
1ad305b
394ac75
4017e5d
c0c7e3d
7621e81
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does throwing the error here prevent retrying? What I had here allowed the code to retry downloading only throw the error in the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is good question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it actually prevent retrying. But I believe it is a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should revert this change. What I had here allowed retrying, and also printed the actual error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the problem is that printing doesn't work (at least with yarn). I will experiment with error throwing again later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could replace this
throw e
withconsole.error(e)
, and then revert what I had to throw the last error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.error
output isn't visible when yarn is used to install package.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then there is no way to print the intermediate error messages with Yarn. I recommend throwing the last error for those on Yarn and let the algorithm work normally for others. This is not a bug in
node-downloader-helper
. The error callback expects a working code, and if we again rethrow the error in the error callback, we are doing redundant work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted my commit
I couldn't make yarn to print error message in a descriptive way I want because yarn is exiting on the first
error
event thrown (and I insist that this is caused by the bug in node-download helper, see hgouveia/node-downloader-helper#57 (comment)).I give up. Let it be so.