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

Pass error as argument to stream.destroy #36

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

Conversation

BenoitClaveau
Copy link

No description provided.

@@ -69,7 +69,7 @@ var pump = function () {
var writing = i > 0
return destroyer(stream, reading, writing, function (err) {
if (!error) error = err
if (err) destroys.forEach(call)
if (err) destroys.forEach(fn => fn(err))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No arrow functions

@mafintosh
Copy link
Owner

The reason we don't do this is because destroy behavior with an argument is pretty undefined in node land (until very recent node changes). Lots of streams out in the wild will do different things, some expect the argument to be an optional cb fx.

In node master there has recently been some good improvements on standardizing the behavior so we could start doing this in the future :)

@BenoitClaveau
Copy link
Author

@mafintosh ok I understand but in nodejs source https://github.com/nodejs/node/blob/master/lib/internal/streams/destroy.js error is the first argument not a cb.

@mafintosh
Copy link
Owner

@BenoitClaveau yea, that's the improvements in node core I was talking about. we still support older versions, plus other stream types so at the current moment we should still call it without and error to be sure. If you wanna feature detect somehow that it's a newer node core stream (and it's simple enough) I'd be ok adding it.

@BenoitClaveau
Copy link
Author

Great. If I understand I need to know if the stream is a node stream3. Is that right ?

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

3 participants