Skip to content

Document require("stream/promises").pipeline end option #48970

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 2 commits into from
May 12, 2024
Merged

Document require("stream/promises").pipeline end option #48970

merged 2 commits into from
May 12, 2024

Conversation

aloisklink
Copy link
Contributor

Add documentation for the require("stream/promises").pipeline(...streams, {end: false}) option, that was added in #40886.

The lacking documentation has been mentioned before (see #34805 (comment), and #45821), but although #45832 did document that the end option existed, it didn't document what the end option did.

I've also added a test to sanity check that setting end: false doesn't stop ending transform streams when calling pipeline(), since this behavior seems a bit unintuitive to me.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jul 30, 2023
@aloisklink aloisklink changed the title Document pipeline end option Document require("stream/promises").pipeline end option Jul 30, 2023
@aloisklink
Copy link
Contributor Author

Pinging @ronag, just to make sure they get a notification in case they want to give a review, since they were the original person that added the end option to pipeline in #40886.

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53126/

@github-actions github-actions bot mentioned this pull request Aug 11, 2023
Open
29 tasks
@aloisklink
Copy link
Contributor Author

aloisklink commented Aug 20, 2023 •
edited
Loading

CI seems to have failed due to a Backing channel 'JNLP4-connect connection from ... is disconnected. error (see nodejs/reliability#634).

It sounds like this is just a random error unrelated to this PR due to a CI worker dying/losing connection.

Can a maintainer re-add a request-ci label?

Edit: It may also be worth labeling this PR as doc (and maybe test too)

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53569/

This was referenced Aug 27, 2023
Open
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53779/

@lpinca lpinca added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Sep 7, 2023
@aduh95 aduh95 force-pushed the document-pipeline-end-option branch from 7a279a1 to 7ea65be Compare May 11, 2024 14:15
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 11, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59105/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59147/

aloisklink added 2 commits May 12, 2024 11:18
@aloisklink @aduh95
test: test pipeline end on transform streams
f3f1aec 
Add test that confirms that
`stream.promises.pipeline(source, transform, dest, {end: false});`
only skips ending the destination stream.
`{end: false}` should still end any transform streams.

PR-URL: nodejs#48970
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@aloisklink @aduh95
doc: document pipeline with end option
ca2f874 
There is currently no documentation about what the `end` option in
`stream.promises.pipeline` does.

Refs: nodejs#40886
Refs: nodejs#34805 (comment)
Fixes: nodejs#45821
PR-URL: nodejs#48970
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@aduh95 aduh95 force-pushed the document-pipeline-end-option branch from 7ea65be to ca2f874 Compare May 12, 2024 09:19
@aduh95
Copy link
Contributor

aduh95 commented May 12, 2024

Landed in 1223294...ca2f874

@aduh95 aduh95 merged commit ca2f874 into nodejs:main May 12, 2024
26 of 27 checks passed
targos pushed a commit that referenced this pull request May 12, 2024
@aloisklink @targos
test: test pipeline end on transform streams
5469adf 
Add test that confirms that
`stream.promises.pipeline(source, transform, dest, {end: false});`
only skips ending the destination stream.
`{end: false}` should still end any transform streams.

PR-URL: #48970
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request May 12, 2024
@aloisklink @targos
doc: document pipeline with end option
80fa675 
There is currently no documentation about what the `end` option in
`stream.promises.pipeline` does.

Refs: #40886
Refs: #34805 (comment)
Fixes: #45821
PR-URL: #48970
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@aloisklink aloisklink deleted the document-pipeline-end-option branch May 13, 2024 06:05
@targos targos mentioned this pull request May 13, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@aloisklink @marco-ippolito
test: test pipeline end on transform streams
9ca114e 
Add test that confirms that
`stream.promises.pipeline(source, transform, dest, {end: false});`
only skips ending the destination stream.
`{end: false}` should still end any transform streams.

PR-URL: #48970
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@aloisklink @marco-ippolito
doc: document pipeline with end option
66575c9 
There is currently no documentation about what the `end` option in
`stream.promises.pipeline` does.

Refs: #40886
Refs: #34805 (comment)
Fixes: #45821
PR-URL: #48970
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@aloisklink @marco-ippolito
test: test pipeline end on transform streams
304bbd5 
Add test that confirms that
`stream.promises.pipeline(source, transform, dest, {end: false});`
only skips ending the destination stream.
`{end: false}` should still end any transform streams.

PR-URL: #48970
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@aloisklink @marco-ippolito
doc: document pipeline with end option
8665441 
There is currently no documentation about what the `end` option in
`stream.promises.pipeline` does.

Refs: #40886
Refs: #34805 (comment)
Fixes: #45821
PR-URL: #48970
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@aloisklink @marco-ippolito
test: test pipeline end on transform streams
d08c9a6 
Add test that confirms that
`stream.promises.pipeline(source, transform, dest, {end: false});`
only skips ending the destination stream.
`{end: false}` should still end any transform streams.

PR-URL: #48970
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@aloisklink @marco-ippolito
doc: document pipeline with end option
e62c6e2 
There is currently no documentation about what the `end` option in
`stream.promises.pipeline` does.

Refs: #40886
Refs: #34805 (comment)
Fixes: #45821
PR-URL: #48970
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
sophob pushed a commit to sophob/node that referenced this pull request Jun 20, 2024
@aloisklink @sophob
test: test pipeline end on transform streams
905c6ed 
Add test that confirms that
`stream.promises.pipeline(source, transform, dest, {end: false});`
only skips ending the destination stream.
`{end: false}` should still end any transform streams.

PR-URL: nodejs#48970
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
sophob pushed a commit to sophob/node that referenced this pull request Jun 20, 2024
@aloisklink @sophob
doc: document pipeline with end option
65567fa 
There is currently no documentation about what the `end` option in
`stream.promises.pipeline` does.

Refs: nodejs#40886
Refs: nodejs#34805 (comment)
Fixes: nodejs#45821
PR-URL: nodejs#48970
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@aloisklink @bmeck
test: test pipeline end on transform streams
9c8a36d 
Add test that confirms that
`stream.promises.pipeline(source, transform, dest, {end: false});`
only skips ending the destination stream.
`{end: false}` should still end any transform streams.

PR-URL: nodejs#48970
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@aloisklink @bmeck
doc: document pipeline with end option
4b68ad0 
There is currently no documentation about what the `end` option in
`stream.promises.pipeline` does.

Refs: nodejs#40886
Refs: nodejs#34805 (comment)
Fixes: nodejs#45821
PR-URL: nodejs#48970
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aloisklink @nodejs-github-bot @aduh95 @lpinca