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: outline that process.stdin emits multiple readable events #27350

Closed
wants to merge 2 commits into from

Conversation

anentropic
Copy link
Contributor

I commented here: #20503 (comment)

Basically I found the current docs quite unclear about the behaviour of process.stdin, particularly with regards to reading a large file from stdin.

@mcollina suggested making a PR to improve the docs, so here is my attempt.

I have adjusted the code comments in the existing js example (as I found them misleading for my purposes). I have added a new paragraph, derived from @mcollina 's comment here #20503 (comment) to try to explain the actual behaviour. Finally I have added a second code example reflecting what I think is a more typical use case.

Please review these carefully for terminology and accuracy, I don't have much experience with node.js

Checklist

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Apr 22, 2019
@BridgeAR BridgeAR changed the title document more clearly that process.stdin will emit multiple readable events doc: outline that process.stdin emits multiple readable events Apr 22, 2019

In "old" streams mode the `stdin` stream is paused by default, so one
must call `process.stdin.resume()` to read from it. Note also that calling
`process.stdin.resume()` itself would switch stream to "old" mode.
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer we did not mention "old" stream mode in this docs. A user can add a 'data' event to start reading as well, it's part of the documented API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This text is not something I added btw, I just moved it around. I'm happy to remove it though. Please be very specific about which part to remove or any alternative wording (I am just a casual node.js user so I don't have a good idea what is appropriate)

Copy link
Member

Choose a reason for hiding this comment

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

@mcollina PTAL

Copy link
Member

Choose a reason for hiding this comment

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

Ok! let's keep the text for now.

@BridgeAR
Copy link
Member

@ronag PTAL

@ronag
Copy link
Member

ronag commented Jan 12, 2020

Doesn’t this apply to streams in general? I haven’t looked at the specifics but if we’re changing anything shouldn’t that be in the streams docs?

@ronag
Copy link
Member

ronag commented Jan 12, 2020

i.e. I think any improvements should go into https://nodejs.org/dist/latest-v13.x/docs/api/stream.html#stream_readable_streams.

Unless there is something special about stdin I would suggest removing/moving the example from stdin doc, add more explicit link to streams documentation and maybe improve the stream docs with further examples and clarification.

@HarshithaKP
Copy link
Member

@anentropic, can you follow up with review comments ?

doc/api/process.md Outdated Show resolved Hide resolved
@anentropic
Copy link
Contributor Author

anentropic commented Mar 31, 2020

@anentropic, can you follow up with review comments ?

@HarshithaKP

re @Ronal's comments in January...

It looks to me that we should move/copy(?) the relevant parts of changed paragraph and example code from process.stdin to https://nodejs.org/dist/latest-v13.x/docs/api/stream.html#stream_readable_read_size where there is a very similar code example for readable.read()

Please confirm, and whether to move it or copy it

@HarshithaKP
Copy link
Member

Ping @ronag, I guess @anentropic miss spelled your name.

@ronag
Copy link
Member

ronag commented Apr 7, 2020

Please confirm, and whether to move it or copy it

I would personally prefer to move it and add a link in its place.

@anentropic anentropic force-pushed the stdin-docs branch 5 times, most recently from 0a2de14 to dc6f8e8 Compare April 9, 2020 21:04
@anentropic
Copy link
Contributor Author

Hi all, thanks for your reviews and feedback. I have amended my changes, hopefully in line with everyone's preferences. Let me know any further tweaks needed.

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

doc/api/stream.md Outdated Show resolved Hide resolved
@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
jasnell pushed a commit that referenced this pull request Jul 3, 2020
document more clearly that stdin will emit multiple readable events

PR-URL: #27350
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jul 3, 2020

Landed in 7c9df66

@jasnell jasnell closed this Jul 3, 2020
MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
document more clearly that stdin will emit multiple readable events

PR-URL: #27350
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 14, 2020
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
document more clearly that stdin will emit multiple readable events

PR-URL: #27350
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
document more clearly that stdin will emit multiple readable events

PR-URL: #27350
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Sep 28, 2020
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. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants