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.

Sign up for GitHub

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

Jump to bottom

doc: clarify recommendations in stream.md #42555

Merged
merged 1 commit into from
Apr 3, 2022
Merged

doc: clarify recommendations in stream.md #42555

merged 1 commit into from
Apr 3, 2022

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 1, 2022

No description provided.

@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 Apr 1, 2022
benjamingr
benjamingr reviewed Apr 1, 2022
View reviewed changes
doc/api/stream.md
implemented to provide the easiest way of consuming stream data. Developers that
require more fine-grained control over the transfer and generation of data can
use the [`EventEmitter`][] and `readable.on('readable')`/`readable.read()`
`readable.pipe()` provides the easiest way to consume stream data. Developers
Copy link
Member

Choose a reason for hiding this comment

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

This statement has not been true for years (at least since we had async/iterators support).

I would remove these 3 lines altogether or maybe replace them with:

readable.pipe is a low-level API to connect streams. It is preferred to use [pipeline][] or [compose][] to connect streams. For convenience you can iterate streams with for... await loops or .toArray() a stream which is convenient but not as performant.

Or a better phrasing of that

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/42555
FetchError: Invalid response body while trying to fetch https://api.github.com/graphql: Premature close
    at consumeBody (file:///opt/hostedtoolcache/node/16.14.0/x64/lib/node_modules/node-core-utils/node_modules/node-fetch/src/body.js:234:60)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Response.text (file:///opt/hostedtoolcache/node/16.14.0/x64/lib/node_modules/node-core-utils/node_modules/node-fetch/src/body.js:158:18)
    at async Request.json (file:///opt/hostedtoolcache/node/16.14.0/x64/lib/node_modules/node-core-utils/lib/request.js:49:18)
    at async Request.query (file:///opt/hostedtoolcache/node/16.14.0/x64/lib/node_modules/node-core-utils/lib/request.js:107:20)
    at async Request.queryAll (file:///opt/hostedtoolcache/node/16.14.0/x64/lib/node_modules/node-core-utils/lib/request.js:134:20)
    at async Request.gql (file:///opt/hostedtoolcache/node/16.14.0/x64/lib/node_modules/node-core-utils/lib/request.js:64:22)
    at async PRData.getComments (file:///opt/hostedtoolcache/node/16.14.0/x64/lib/node_modules/node-core-utils/lib/pr_data.js:97:21)
    at async Promise.all (index 2)
    at async Promise.all (index 1) {
  type: 'system',
  errno: 'ERR_STREAM_PREMATURE_CLOSE',
  code: 'ERR_STREAM_PREMATURE_CLOSE',
  erroredSysCall: undefined
}
https://github.com/nodejs/node/actions/runs/2084348110

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Apr 3, 2022
@Trott
doc: clarify recommendations in stream.md
b717795 
PR-URL: nodejs#42555
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
@Trott
Copy link
Member Author

Trott commented Apr 3, 2022

Landed in b717795

@Trott Trott merged commit b717795 into nodejs:master Apr 3, 2022
@Trott Trott deleted the stream branch April 3, 2022 05:47
juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022
@Trott @juanarbol
doc: clarify recommendations in stream.md
8dd88cc 
PR-URL: nodejs#42555
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
This was referenced Apr 5, 2022
Closed
Merged
juanarbol pushed a commit that referenced this pull request Apr 6, 2022
@Trott @juanarbol
doc: clarify recommendations in stream.md
c036800 
PR-URL: #42555
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
@Trott @xtx1130
doc: clarify recommendations in stream.md
aa5bf3f 
PR-URL: nodejs#42555
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
juanarbol pushed a commit that referenced this pull request May 31, 2022
@Trott @juanarbol
doc: clarify recommendations in stream.md
5a15cb8 
PR-URL: #42555
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
@juanarbol juanarbol mentioned this pull request May 31, 2022
Closed
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
@Trott @danielleadams
doc: clarify recommendations in stream.md
3b83f91 
PR-URL: #42555
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2022
@Trott @targos
doc: clarify recommendations in stream.md
c746cd3 
PR-URL: #42555
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@Trott @targos
doc: clarify recommendations in stream.md
c361129 
PR-URL: #42555
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@Trott @guangwong
doc: clarify recommendations in stream.md
ebb9f0d 
PR-URL: nodejs/node#42555
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mesteery Mesteery Mesteery approved these changes

@marsonya marsonya marsonya approved these changes

@benjamingr benjamingr benjamingr approved these changes

@bnb bnb bnb approved these changes

Assignees
No one assigned
Labels
commit-queue-failed An error occurred while landing this pull request using GitHub Actions. doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Trott @nodejs-github-bot @bnb @benjamingr @marsonya @Mesteery