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 modes of writable #29745

Closed
wants to merge 8 commits into from
Closed

Conversation

ronag
Copy link
Member

@ronag ronag commented Sep 28, 2019

Documents the error modes of Writable

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 Sep 28, 2019
@ronag ronag changed the title doc: document error modes of writable.write doc: error modes of writable.write Sep 28, 2019
@ronag ronag force-pushed the doc-write-errors branch 4 times, most recently from 6c6beba to b2c4f5a Compare September 28, 2019 08:11
@ronag ronag changed the title doc: error modes of writable.write doc: error modes of writable Sep 28, 2019
@ronag ronag force-pushed the doc-write-errors branch 5 times, most recently from 5e422d0 to b030cf2 Compare September 28, 2019 10:03
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@Trott
Copy link
Member

Trott commented Oct 1, 2019

doc/api/stream.md Outdated Show resolved Hide resolved
Co-Authored-By: Rich Trott <rtrott@gmail.com>
@ronag ronag requested a review from Trott November 24, 2019 09:56
@ronag
Copy link
Member Author

ronag commented Nov 24, 2019

Could this get another review from nodejs/streams?

@Trott
Copy link
Member

Trott commented Nov 24, 2019

@nodejs/streams

@@ -432,6 +432,18 @@ file.end('world!');
// Writing more now is not allowed!
```

[`stream.end()`][stream-end] will error with (in order of precedence):
Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify if it will throw or emit an error?

Copy link
Member Author

Choose a reason for hiding this comment

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

How about now?

@@ -619,6 +631,14 @@ write('hello', () => {

A `Writable` stream in object mode will always ignore the `encoding` argument.

`stream.write()` will error with (in order of precedence):
Copy link
Member

Choose a reason for hiding this comment

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

Same

doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
Co-Authored-By: Yorkie Liu <yorkiefixer@gmail.com>
@@ -619,6 +632,15 @@ write('hello', () => {

A `Writable` stream in object mode will always ignore the `encoding` argument.

`stream.write()` will call it's callback and emit `'error'`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`stream.write()` will call it's callback and emit `'error'`
`stream.write()` will call its callback and emit `'error'`

doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
Co-Authored-By: Denys Otrishko <9109612+lundibundi@users.noreply.github.com>
@yorkie
Copy link
Contributor

yorkie commented Nov 24, 2019

This PR seems to document how does the write/end methods emit the errors, it's a bit more easy to be changed in somehow and without changing this docs, if we would have links to functions, it will feel less.

@ronag
Copy link
Member Author

ronag commented Nov 24, 2019

This PR seems to document how does the write/end methods emit the errors, it's a bit more easy to be changed in somehow and without changing this docs, if we would have links to functions, it will feel less.

Sorry, I don't understand. Do you think you could try to rephrase?

@yorkie
Copy link
Contributor

yorkie commented Nov 24, 2019

Sorry, I don't understand. Do you think you could try to rephrase?

Never mind, I'm not proposing any changes on this PR. FYI, I was considering that it's easy to forget to update here when someone changed the algorithm of write/end in our streams.

@ronag
Copy link
Member Author

ronag commented Dec 1, 2019

@yorkie @lundibundi Can this PR get your approvals and maybe land?

doc/api/stream.md Outdated Show resolved Hide resolved
@lundibundi
Copy link
Member

ping @mcollina

Co-Authored-By: Denys Otrishko <9109612+lundibundi@users.noreply.github.com>
@lundibundi
Copy link
Member

@ronag
Copy link
Member Author

ronag commented Dec 29, 2019

Currently blocked by whatever happens with #29197 which affects some details of this.

@lundibundi lundibundi added the blocked PRs that are blocked by other issues or PRs. label Dec 30, 2019
@BridgeAR
Copy link
Member

This needs a rebase.

@ronag
Copy link
Member Author

ronag commented Feb 14, 2020

The error modes of streams is still a fluid situation so I think it's better re-open this at a future stage.

@ronag ronag closed this Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. 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.

8 participants