-
-
Notifications
You must be signed in to change notification settings - Fork 35
Added support for .destroy(err, cb) signature#30
Added support for .destroy(err, cb) signature#30mcollina wants to merge 1 commit intomafintosh:masterfrom
Conversation
Remove the internal destroy implementation and use the readable-stream@3 one.
See mafintosh/pumpify#11.
|
I'm not familiar enough with stream implementation to review but thanks, I confirm this fixes mafintosh/pumpify#11 |
|
Added a quick fix to master. Need to review this doesn't break all my stuff as it changes the behaviour. |
|
It doesn't, or it's not covered by the tests. All test suite for this pass without changes. |
|
@mafintosh @mcollina @vweevers Is it possible to get this merged? This package is used by google storage and we experience crashes when uploading files. I belive this PR fixes it. |
|
I'll look into it this week
... On 27 Sep 2021, at 09.50, Erlend Faxvaag ***@***.***> wrote:
@mafintosh @mcollina @vweevers Is it possible to get this merged?
This package is used by google storage and we experience crashes when uploading files - caused by this.
--
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
|
I might have jumped to conclusions here. When playing around it looked like this PR fixed it. I'm new to js and node. I described the overlaying error in an issue at nodejs google storage repo, and also created another new issue here with steps to reproduce. Btw, the first comment here says:
but it looks like |