doc: outline that process.stdin emits multiple readable events#27350
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added doc 
BridgeAR changed the title 
doc/api/process.md Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
BridgeAR commented Jan 12, 2020
@ronag PTAL |
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? |
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 commented Mar 31, 2020
@anentropic, can you follow up with review comments ? |
Uh oh!
There was an error while loading. Please reload this page.
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 Please confirm, and whether to move it or copy it |
HarshithaKP commented Apr 7, 2020
Ping @ronag, I guess @anentropic miss spelled your name. |
ronag commented Apr 7, 2020
I would personally prefer to move it and add a link in its place. |
anentropicforce-pushed the stdin-docs branch 5 times, most recently from 0a2de14 to dc6f8e8Compareanentropic commented Apr 9, 2020
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. |
Uh oh!
There was an error while loading. Please reload this page.
…t from process.html stdin
BridgeARforce-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72Compare
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 commented Jul 3, 2020
Landed in 7c9df66 |
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]>
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]>
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]>
7 participants
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