doc: stdout, stderr and stdin are all Duplex streams #11194

seppevs · 2017-02-06T09:59:45Z

doc: stdout, stderr and stdin are all Duplex streams but documentation states otherwise

This is a fix for #9201

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

jasnell · 2017-02-06T15:08:58Z

I'm not convinced that we should document these as Duplex and it's not entirely clear if that's always going to be the case. For instance, it does not make sense to treat stdin as a Writable.

addaleax · 2017-02-06T15:13:51Z

For instance, it does not make sense to treat stdin as a Writable.

Just for clarification, that stdin is a “readable” stream and stdout/stderr are “writable” streams is only a convention and not enforced by the OS in any way (at least on UNIX and I think it applies to Windows, too).

I don’t think it’s Node’s place to enforce these convention when the OS doesn’t.

jasnell · 2017-02-06T15:25:05Z

True, but it's also not that practically useful unless there's an obscure use case I'm missing.

Fishrock123 · 2017-02-06T15:32:49Z

True, but it's also not that practically useful unless there's an obscure use case I'm missing.

Maybe we could start printing warnings to fd 0. 😛

The docs are still technically correct due to the weirdness that is the stdio impl.

I think changing it to note that it is net.Socket (inherits from stream.Duplex) unless it is from/to a file, in which case it is a fs.sync{Write|Read}Stream, could be more useful.

jasnell · 2017-02-06T15:34:44Z

That would be certainly more useful

sam-github · 2017-02-11T20:08:01Z

Just because it may not be used very often doesn't mean its not useful. Why should we have an opinion on this, particularly if its platform independent, and pre-existing? There are interesting use-cases, like gpg, where processes read and write from arbitrary fds, I like that such programs can be written in node, that its not just for webapps.

sam-github · 2017-02-11T20:09:52Z

Agree with #11194 (comment), we should doc its actual class.

seppevs · 2017-02-13T09:53:08Z

I've amended changes to the commit so the actual class is now documented.

sam-github

I left some suggestions, I find the "from/to" confusing, but basically LGTM.

sam-github · 2017-02-16T19:29:43Z

doc/api/process.md

-associated with `stderr` (fd `2`).
+The `process.stderr` property returns a stream equivalent to or
+associated with `stderr` (fd `2`). It is a `net.Socket` (inherits from `stream.Duplex`) 
+unless it is from/to a file, in which case it is a [Writable][] stream.


I suggest: "unless fd 2 refers to a file, "

@sam-github Thank you for reviewing. I made the requested changes and rebased with master. I had some merge conflicts on the same sections in the docs.
Can you review again? Thanks!

sam-github · 2017-02-16T19:29:55Z

doc/api/process.md

-The `process.stderr` property returns a [Writable][] stream equivalent to or
-associated with `stderr` (fd `2`).
+The `process.stderr` property returns a stream equivalent to or
+associated with `stderr` (fd `2`). It is a `net.Socket` (inherits from `stream.Duplex`) 


"(which inherits"

sam-github · 2017-02-16T20:12:42Z

code looks good, @Fishrock123 assigned this to himself, so he'll need a chance to review

you should shorten your commit message, guidelines are here: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit

jasnell

Almost there, small nit

jasnell · 2017-02-16T20:50:25Z

doc/api/process.md

-`stderr` (fd `2`).
+The `process.stderr` property returns a stream connected to
+`stderr` (fd `2`). It is a `net.Socket` (which inherits from 
+`stream.Duplex`) unless fd `2` refers to a file, in which case it is 


It would be worthwhile to have this (and the other reference below) be a link to the Duplex doc

seppevs · 2017-02-17T08:16:09Z

@jasnell: I made the changes, can you review again?

Fishrock123

LGTM with one small comment

Fishrock123 · 2017-02-17T20:40:46Z

doc/api/process.md

-The `process.stderr` property returns a [Writable][] stream connected to
-`stderr` (fd `2`).
+The `process.stderr` property returns a stream connected to
+`stderr` (fd `2`). It is a `net.Socket` (which is a [Duplex][] 


Could you link net.Socket to it's definition in net.md? Thanks!

@Fishrock123 done

fhinkel · 2017-03-26T10:13:16Z

@seppevs sorry this took so long, by now process.md has changed. Can you rebase and make [Duplex] look like the other entries? Thanks!

…n states otherwise This is a fix for nodejs#9201

seppevs · 2017-03-27T07:40:05Z

@fhinkel done!

stdout, stderr and stdin are all Duplex streams but documentation states otherwise Fixes #9201 PR-URL: #11194 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

aqrln · 2017-03-27T14:16:49Z

@cjihrig just for the record, this patch had been landed in 1005b1d before your approval.
@fhinkel was this an accidental push or you just forgot to close the PR?

Fishrock123 · 2017-03-27T14:21:32Z

Looks like she forgot to close it. Please re-open if this was in error.

stdout, stderr and stdin are all Duplex streams but documentation states otherwise Fixes #9201 PR-URL: #11194 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

MylesBorins · 2017-04-18T22:58:05Z

backported to v6.x-staging. If it should be backed out lmk

stdout, stderr and stdin are all Duplex streams but documentation states otherwise Fixes #9201 PR-URL: #11194 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

stdout, stderr and stdin are all Duplex streams but documentation states otherwise Fixes nodejs/node#9201 PR-URL: nodejs/node#11194 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Feb 6, 2017

seppevs force-pushed the docs_stdout_stderr_and_stdin_are_duplex_streams branch from 30d6058 to 5a00980 Compare February 13, 2017 09:51

Fishrock123 self-assigned this Feb 14, 2017

sam-github approved these changes Feb 16, 2017

View reviewed changes

seppevs force-pushed the docs_stdout_stderr_and_stdin_are_duplex_streams branch 2 times, most recently from b78f255 to a7b5e61 Compare February 16, 2017 20:00

sam-github approved these changes Feb 16, 2017

View reviewed changes

jasnell requested changes Feb 16, 2017

View reviewed changes

seppevs force-pushed the docs_stdout_stderr_and_stdin_are_duplex_streams branch from a7b5e61 to d5793b7 Compare February 17, 2017 08:15

jasnell approved these changes Feb 17, 2017

View reviewed changes

Fishrock123 approved these changes Feb 17, 2017

View reviewed changes

seppevs force-pushed the docs_stdout_stderr_and_stdin_are_duplex_streams branch from d5793b7 to a2f9c82 Compare February 22, 2017 09:39

doc: stdout, stderr and stdin are all Duplex streams but documentatio…

b3a6a25

…n states otherwise This is a fix for nodejs#9201

seppevs force-pushed the docs_stdout_stderr_and_stdin_are_duplex_streams branch from a2f9c82 to b3a6a25 Compare March 27, 2017 07:39

fhinkel approved these changes Mar 27, 2017

View reviewed changes

cjihrig approved these changes Mar 27, 2017

View reviewed changes

Fishrock123 closed this Mar 27, 2017

MylesBorins mentioned this pull request Mar 28, 2017

V7.8.0 proposal #12104

Merged

italoacasas mentioned this pull request Apr 10, 2017

v7.9.0 Release Proposal #12319

Merged

2 tasks

MylesBorins added the lts-watch-v6.x label Apr 18, 2017

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Apr 18, 2017

MylesBorins mentioned this pull request Apr 19, 2017

v6.10.3 proposal #12498

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: stdout, stderr and stdin are all Duplex streams #11194

doc: stdout, stderr and stdin are all Duplex streams #11194

seppevs commented Feb 6, 2017

jasnell commented Feb 6, 2017

addaleax commented Feb 6, 2017

jasnell commented Feb 6, 2017

Fishrock123 commented Feb 6, 2017 •

edited

Loading

jasnell commented Feb 6, 2017

sam-github commented Feb 11, 2017

sam-github commented Feb 11, 2017

seppevs commented Feb 13, 2017

sam-github left a comment

sam-github Feb 16, 2017

seppevs Feb 16, 2017

sam-github Feb 16, 2017

sam-github commented Feb 16, 2017

jasnell left a comment

jasnell Feb 16, 2017

seppevs commented Feb 17, 2017

Fishrock123 left a comment

Fishrock123 Feb 17, 2017

seppevs Feb 22, 2017

fhinkel commented Mar 26, 2017

seppevs commented Mar 27, 2017

aqrln commented Mar 27, 2017

Fishrock123 commented Mar 27, 2017

MylesBorins commented Apr 18, 2017

doc: stdout, stderr and stdin are all Duplex streams #11194

doc: stdout, stderr and stdin are all Duplex streams #11194

Conversation

seppevs commented Feb 6, 2017

Checklist

Affected core subsystem(s)

jasnell commented Feb 6, 2017

addaleax commented Feb 6, 2017

jasnell commented Feb 6, 2017

Fishrock123 commented Feb 6, 2017 • edited Loading

jasnell commented Feb 6, 2017

sam-github commented Feb 11, 2017

sam-github commented Feb 11, 2017

seppevs commented Feb 13, 2017

sam-github left a comment

Choose a reason for hiding this comment

sam-github Feb 16, 2017

Choose a reason for hiding this comment

seppevs Feb 16, 2017

Choose a reason for hiding this comment

sam-github Feb 16, 2017

Choose a reason for hiding this comment

sam-github commented Feb 16, 2017

jasnell left a comment

Choose a reason for hiding this comment

jasnell Feb 16, 2017

Choose a reason for hiding this comment

seppevs commented Feb 17, 2017

Fishrock123 left a comment

Choose a reason for hiding this comment

Fishrock123 Feb 17, 2017

Choose a reason for hiding this comment

seppevs Feb 22, 2017

Choose a reason for hiding this comment

fhinkel commented Mar 26, 2017

seppevs commented Mar 27, 2017

aqrln commented Mar 27, 2017

Fishrock123 commented Mar 27, 2017

MylesBorins commented Apr 18, 2017

Fishrock123 commented Feb 6, 2017 •

edited

Loading