Sitelet https://web.archive.org/web/20211021115756/https://github.com/nodejs/node/pull/40193
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
+45 −7

Conversation

@Mesteery
Copy link
Member

@Mesteery Mesteery commented Sep 23, 2021

Fixes: #40191
@Mesteery
stream: support array of streams in promises pipeline
c9ed79f 
Fixes: nodejs#40191
@lpinca
Copy link
Member

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

@Mesteery 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 lpinca commented Sep 23, 2021

Ah ok. Can you please add a test?

Mesteery added 3 commits Sep 23, 2021
@Mesteery
fixup! stream: support array of streams in promises pipeline
7af0af2
@Mesteery
fixup! fixup! stream: support array of streams in promises pipeline
a7f4073
@Mesteery
Update promises.js
dbc3a05
lpinca
lpinca approved these changes Sep 24, 2021
View changes
test/parallel/test-stream-pipeline.js Show resolved Hide resolved
ronag
ronag approved these changes Sep 24, 2021
View changes
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Oct 7, 2021

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

@github-actions github-actions bot mentioned this pull request Oct 8, 2021
Open
23 tasks
@github-actions github-actions bot mentioned this pull request Oct 9, 2021
Open
21 tasks
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Oct 9, 2021

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

@github-actions github-actions bot mentioned this pull request Oct 10, 2021
Open
18 tasks
@github-actions github-actions bot mentioned this pull request Oct 11, 2021
Open
19 tasks
@Mesteery Mesteery removed the needs-ci label Oct 12, 2021
@github-actions github-actions bot mentioned this pull request Oct 13, 2021
Open
22 tasks
@Ayase-252
Copy link
Member

@Ayase-252 Ayase-252 commented Oct 20, 2021

Landed in b920a10

@Ayase-252 Ayase-252 closed this Oct 20, 2021
Ayase-252 added a commit that referenced this issue 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 Oct 20, 2021
targos added a commit that referenced this issue 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 issue 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 issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca

@jasnell jasnell

@ronag ronag

Assignees
No one assigned
Labels
author ready stream
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

stream.promises.pipeline doesn't support arrays of streams since node 16.10
6 participants
@Mesteery @lpinca @nodejs-github-bot @Ayase-252 @jasnell @ronag