Skip to content

stream: support array of streams in promises pipeline#40193

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
Closed
Mesteery wants to merge 4 commits into from
Closed

stream: support array of streams in promises pipeline #40193

Mesteery wants to merge 4 commits into from
+45 −7

Conversation

@Mesteery
Copy link
Contributor

@MesteeryMesteery commented Sep 23, 2021

Fixes: #40191

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 23, 2021
@lpinca
Copy link
Member

lpinca commented Sep 23, 2021

It should be already supported

node/lib/internal/streams/pipeline.js

Lines 172 to 174 in fd86dad

if(ArrayIsArray(streams[0])&&streams.length===1){
streams=streams[0];
}
no?

@MesteeryMesteery added the stream Issues and PRs related to the stream subsystem. label Sep 23, 2021
@Mesteery
Copy link
ContributorAuthor

Mesteery commented Sep 23, 2021•
edited
Loading

Since c04d621, promises.pipeline calls directly pipelineImpl:

node/lib/stream/promises.js

Line 14 in c9ed79f

const{pipelineImpl: pl}=require('internal/streams/pipeline');

@lpinca
Copy link
Member

lpinca commented Sep 23, 2021

Ah ok. Can you please add a test?

lpinca
lpinca approved these changes Sep 24, 2021
View reviewed changes
@Ayase-252Ayase-252 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 7, 2021
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 7, 2021

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

This was referenced Oct 8, 2021
Open
Open
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 9, 2021

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

This was referenced Oct 10, 2021
Open
Open
@MesteeryMesteery removed the needs-ci PRs that need a full CI run. label Oct 12, 2021
@github-actionsgithub-actionsbot mentioned this pull request Oct 13, 2021
Open
22 tasks
@Ayase-252
Copy link
Member

Ayase-252 commented Oct 20, 2021

Landed in b920a10

Ayase-252 pushed a commit that referenced this pull request Oct 20, 2021
@Mesteery@Ayase-252
stream: support array of streams in promises pipeline
b920a10
Fixes: #40191 PR-URL: #40193 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MesteeryMesteery deleted the patch-2 branch October 20, 2021 11:56
targos pushed a commit that referenced this pull request Oct 20, 2021
@Mesteery@targos
stream: support array of streams in promises pipeline
b84f101
Fixes: #40191 PR-URL: #40193 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos added a commit that referenced this pull request Oct 20, 2021
@targos
2021-10-20, Version 17.0.1 (Current)
d68dd62
Notable changes: Fixed distribution for native addon builds This release fixes an issue introduced in Node.js v17.0.0, where some V8 headers were missing from the distributed tarball, making it impossible to build native addons. These headers are now included. #40526 Fixed stream issues * Fixed a regression in `stream.promises.pipeline`, which was introduced in version 16.10.0, is fixed. It is now possible again to pass an array of streams to the function. #40193 * Fixed a bug in `stream.Duplex.from`, which didn't work properly when an async generator function was passed to it. #40499 PR-URL: #40535
@targostargos mentioned this pull request Oct 20, 2021
Merged
targos added a commit that referenced this pull request Oct 20, 2021
@targos
2021-10-20, Version 17.0.1 (Current)
73b2d3b
Notable changes: Fixed distribution for native addon builds This release fixes an issue introduced in Node.js v17.0.0, where some V8 headers were missing from the distributed tarball, making it impossible to build native addons. These headers are now included. #40526 Fixed stream issues * Fixed a regression in `stream.promises.pipeline`, which was introduced in version 16.10.0, is fixed. It is now possible again to pass an array of streams to the function. #40193 * Fixed a bug in `stream.Duplex.from`, which didn't work properly when an async generator function was passed to it. #40499 PR-URL: #40535
targos pushed a commit that referenced this pull request Nov 4, 2021
@Mesteery@targos
stream: support array of streams in promises pipeline
36d3b12
Fixes: #40191 PR-URL: #40193 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: James M Snell <[email protected]>
@BethGriggsBethGriggs mentioned this pull request Nov 26, 2021
Merged
1 task
@yume-chanyume-chan mentioned this pull request Mar 14, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpincalpincalpinca approved these changes

@jasnelljasnelljasnell approved these changes

@ronagronagronag approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

stream.promises.pipeline doesn't support arrays of streams since node 16.10

7 participants

@Mesteery@lpinca@nodejs-github-bot@Ayase-252@jasnell@ronag@targos