-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
Added support for .destroy(err, cb) signature #30
base: master
Are you sure you want to change the base?
Conversation
cc @targos |
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 |
Remove the internal destroy implementation and use the readable-stream@3 one.
See mafintosh/pumpify#11.