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

Jump to bottom

stream: support array of streams in promises pipeline #40193

Closed
wants to merge 4 commits into from
Closed

stream: support array of streams in promises pipeline #40193

wants to merge 4 commits into from

Conversation

Mesteery
Copy link
Member

Fixes: #40191

@nodejs-github-bot nodejs-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?

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

Mesteery commented Sep 23, 2021 •
edited

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?

@Ayase-252 Ayase-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-actions github-actions bot 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

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

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

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

Landed in b920a10

@Ayase-252 Ayase-252 closed this Oct 20, 2021
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 <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Mesteery Mesteery 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 <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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
@targos targos 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 <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Nov 26, 2021
Merged
1 task
@yume-chan yume-chan mentioned this pull request Mar 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@ronag ronag ronag 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. 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.

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