Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mcollina
Copy link

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

See mafintosh/pumpify#11.

@mcollina
Copy link
Author

cc @targos

@targos
Copy link

targos commented Jun 12, 2019

I'm not familiar enough with stream implementation to review but thanks, I confirm this fixes mafintosh/pumpify#11

@vweevers
Copy link
Contributor

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

this.destroyed = false

@mafintosh
Copy link
Owner

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

@mcollina
Copy link
Author

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

@ErlendFax
Copy link

ErlendFax commented Sep 27, 2021

@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
Copy link
Owner

mafintosh commented Sep 27, 2021 via email

@ErlendFax
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants