-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add download
util
#84
Conversation
Codecov Report
@@ Coverage Diff @@
## master #84 +/- ##
==========================================
- Coverage 91.59% 90.07% -1.53%
==========================================
Files 9 10 +1
Lines 226 282 +56
==========================================
+ Hits 207 254 +47
- Misses 19 28 +9
Continue to review full report at Codecov.
|
@@ -0,0 +1,3 @@ | |||
## Download | |||
|
|||
Functionality adapted from currently unmaintained `download` package: https://github.com/kevva/download |
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.
It'll be good to clarify why we did that, and explain how issue was fixed (was upgrade of got
good enough`?)
Is issue we're introducing workaround for reported at download
repository? If it is, it'll be good to link it, otherwise it'll be good to create report over there and link it here.
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.
That's a great call, I'll add all explanations and changeset to the documentation. There's an issue on the download
repo side, where I posted a comment with our specific case: kevva/download#208
711fa2b
to
016deb0
Compare
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 great 👍
Based on https://github.com/kevva/download and brought in due to outdated
got
dependency in original library which didn't property handle timeouts for streams, causinginteractive CLI
flow to hang for the duration of the timeout.I've tried to change as little code as possible in the original module (hence I left a lot of promise-based code without refatoring to async) - tests had to be adjusted more due to the use of different testing framework in the original module.