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

doc: error event is optionally emitted after .destroy() #26589

Closed
wants to merge 1 commit into from
Closed

doc: error event is optionally emitted after .destroy() #26589

wants to merge 1 commit into from

Conversation

tadjik1
Copy link
Contributor

@tadjik1 tadjik1 commented Mar 11, 2019

error event on each kind of stream is optionally emitted when .destroy() method is called. It depends on ._destroy() implementation. In default implementation this event will no be fired unless error parameter has been provided.

It was already mentioned for writable.destroy([error]), so I just copied same sentence for the other streams.

Checklist

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Mar 11, 2019
doc/api/stream.md Outdated Show resolved Hide resolved
@vsemozhetbyt
Copy link
Contributor

@Trott
Copy link
Member

Trott commented Mar 11, 2019

"optionally" makes it sound like it's an option chosen by the user, but this really means "emit an 'error' event if there is an error", right?

Just a suggestion for improvement but not blocking this change. What is here is already an improvement over what's currently in the docs now.

@tadjik1
Copy link
Contributor Author

tadjik1 commented Mar 11, 2019

@Trott good catch! that's true, it's not about user to decide whether or not emit this event.
however it's not 100% true that this behaviour depends on error argument - this is the default implementation, but it could be overrided:

const readable = new stream.Readable({
  read() {},
  _destroy(err, cb) {
    // won't pass error further -> no event will be fired 
    cb();
  }
});

In this case even if you pass Error in readable.destroy() it won't emit this event.

@tadjik1
Copy link
Contributor Author

tadjik1 commented Mar 13, 2019

Working on #26638 I've realised that in some situations streams are not emitting close event. Option emitClose could prevent this. So this phrase always emit a "close" events is also wrong.

`error` event on each kind of stream is optionally emitted when
`.destroy()` method is called. It depends on `._destroy()`
implementation. In default implementation this event will no be
fired unless `error` parameter has been provided.

It was already mentioned for `writable.destroy([error])`, so I
just copied same sentence for the other streams.
@mcollina
Copy link
Member

cc @nodejs/streams

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt
Copy link
Contributor

Landed in 78162ad
Thank you!

vsemozhetbyt pushed a commit that referenced this pull request Mar 19, 2019
`error` event on each kind of stream is optionally emitted when
`.destroy()` method is called. It depends on `._destroy()`
implementation. In default implementation this event will no be
fired unless `error` parameter has been provided.

It was already mentioned for `writable.destroy([error])`, so I
just copied same sentence for the other streams.

PR-URL: #26589
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
`error` event on each kind of stream is optionally emitted when
`.destroy()` method is called. It depends on `._destroy()`
implementation. In default implementation this event will no be
fired unless `error` parameter has been provided.

It was already mentioned for `writable.destroy([error])`, so I
just copied same sentence for the other streams.

PR-URL: nodejs#26589
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Mar 27, 2019
`error` event on each kind of stream is optionally emitted when
`.destroy()` method is called. It depends on `._destroy()`
implementation. In default implementation this event will no be
fired unless `error` parameter has been provided.

It was already mentioned for `writable.destroy([error])`, so I
just copied same sentence for the other streams.

PR-URL: #26589
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants