search

mafintosh / duplexify

Turn a writable and readable stream into a streams2 duplex stream with support for async initialization and streams1/streams2 input
MIT License
190 stars 36 forks source link

Added support for .destroy(err, cb) signature #30

Open mcollina opened 5 years ago

mcollina commented 5 years ago

Remove the internal destroy implementation and use the readable-stream@3 one.

See https://github.com/mafintosh/pumpify/issues/11.

mcollina commented 5 years ago

cc @targos

targos commented 5 years ago

I'm not familiar enough with stream implementation to review but thanks, I confirm this fixes https://github.com/mafintosh/pumpify/issues/11

vweevers commented 5 years ago

@mcollina We can also remove this from the constructor now:

https://github.com/mafintosh/duplexify/blob/5a6b7dd1a71c836fbeca569ffedce31b06c8ef78/index.js#L57

mafintosh commented 5 years ago

Added a quick fix to master. Need to review this doesn't break all my stuff as it changes the behaviour.

mcollina commented 5 years ago

It doesn’t, or it’s not covered by the tests. All test suite for this pass without changes.

ErlendFax commented 3 years ago

@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.

mafintosh commented 3 years ago

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.

ErlendFax commented 3 years ago

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:

Remove the internal destroy implementation

but it looks like .destroy(...) was removed and ._destroy(...) was kept.