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: piping from async generators using pipeline() #33992

Closed
wants to merge 1 commit into from

Conversation

WilliamConnatser
Copy link
Contributor

This will be my first contribution, so please let me know if I did something wrong. Thank you!

This PR is an attempt to implement #33644

Although the issue asked I remove the old snippet, after thinking about it more I decided to keep the manual way of handling backpressure and backpressure-related errors in the docs.

Firstly, I think there may be some use case case for having to handle this manually. Secondly, having the manual way to handle this will be useful for anyone looking to gain insight as to how things work under the hood of pipeline().

I did mention pipeline() first and its benefits (that it abstracts the backpressure handling away), so I thought it was a sufficient compromise but I can remove it if ya'll think otherwise.

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 Jun 20, 2020
@WilliamConnatser WilliamConnatser force-pushed the master branch 2 times, most recently from 43d87e5 to d67ecbf Compare June 20, 2020 18:44
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

@nodejs/streams

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
@ronag
Copy link
Member

ronag commented Jun 21, 2020

Maybe this should wait for #33991?

@mcollina
Copy link
Member

I think we might want to iterate on that a bit more, so if this is ready we should land.

@WilliamConnatser
Copy link
Contributor Author

Thank ya'll for your feedback. I have implemented all edits which were suggested in review.

Let me know if anyone has any more suggestions.

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

@mcollina
Copy link
Member

mcollina pushed a commit that referenced this pull request Jun 22, 2020
PR-URL: #33992
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mcollina
Copy link
Member

Landed in bfc0e3f

@mcollina mcollina closed this Jun 22, 2020
codebytere pushed a commit that referenced this pull request Jun 27, 2020
PR-URL: #33992
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
PR-URL: #33992
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants