stream: add pipeline() for webstreams by debadree25 · Pull Request #46307 · nodejs/node · GitHub

Added support to using pipeline() for webstreams and added tests for both webstreams and mixture of node streams and webstreams with pipeline

Refs: #39316

Refs: nodejs#39316

Review requested:

@nodejs/streams

I would prefer if we could avoid transforming into node stream

I would prefer if we could avoid transforming into node stream

Any suggestion what alternative you would prefer?

I would prefer if we could avoid transforming into node stream

Any suggestion what alternative you would prefer?

What I wrote 😄. We don't convert generators to streams. Instead we have a custom function pumping.

I would prefer if we could avoid transforming into node stream

Any suggestion what alternative you would prefer?

What I wrote 😄. We don't convert generators to streams. Instead we have a custom function pumping.

Ah ok yes 😅😅, I think could try using pipeThrough of readable streams, converting the PR to draft

Ok this requires some work closing this for now will reopen with fresh version 😅😅

Hi @ronag we are in an interesting position in regards to this PR turns out pipeline already supports webstreams due to #46307 since the pipeline is converting streams to duplexes and duplex now supports webstreams, except it breaks if transform streams are added in between, so I am thinking could do two things

If the pipeline consists entirely of webstreams we could ensure that webstream native methods like pipeThrough, pipeTo is used
If there is interop between webstreams and node streams we could allow this Duplex interop to continue?

would this be an acceptable path?

Hello, have updated the code, 3 tests are failing which shall fix in a while but generally have updated to not convert everything to nodestreams 😅😅, could you please take a look again @ronag if the general direction seems to be correct?

Just took a very quick look but it seems to be right general direction.

Reopening for review all the tests passing!

I would make two separate pump functions.

Ok refactoring

I would make two separate pump functions.

The code would be duplicated no?

Have updated to use a separate function

lgtm

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

Nit: since the test is only pushing strings through, perhaps just simply values.push(chunk) ?

sure updating

Fixed

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

Landed in 23effb2

This one had taken quite some trial and error!
Thank you to all reviewers for your time and patience!

Refs: #39316 PR-URL: #46307 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Refs: nodejs#39316 PR-URL: nodejs#46307 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Refs: #39316 PR-URL: #46307 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

stream: add pipeline() for webstreams

4d04a5e

Refs: nodejs#39316

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 22, 2023

ronag reviewed Jan 22, 2023

View reviewed changes

debadree25 marked this pull request as draft January 22, 2023 14:03

debadree25 closed this Jan 22, 2023

debadree25 reopened this Jan 22, 2023

fixup! stop making everything nodestream

06b8fff

debadree25 added 2 commits January 26, 2023 20:13

fixup! final fixups for transform streams

08c333a

fixup! remove redundant

cb7de04

debadree25 marked this pull request as ready for review January 26, 2023 14:49

debadree25 marked this pull request as draft January 26, 2023 14:49

fixup! use proper eos and request type

22ee747

debadree25 marked this pull request as ready for review January 26, 2023 16:10

debadree25 requested a review from ronag January 26, 2023 16:10

ronag reviewed Jan 26, 2023

View reviewed changes

fixup! separate out pump function

2ec9a44

fixup! generator functions and related tests

d1919d9

debadree25 requested a review from ronag January 26, 2023 18:48

mcollina approved these changes Jan 29, 2023

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2023

jasnell reviewed Jan 29, 2023

View reviewed changes

Comment thread lib/internal/streams/pipeline.js

jasnell reviewed Jan 29, 2023

View reviewed changes

Comment thread lib/internal/streams/pipeline.js

jasnell reviewed Jan 29, 2023

View reviewed changes

benjamingr approved these changes Jan 29, 2023

View reviewed changes

fixup! nit on test

4c2c2eb

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 30, 2023

lpinca added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 2, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 2, 2023

nodejs-github-bot merged commit 23effb2 into nodejs:main Feb 2, 2023

MylesBorins mentioned this pull request Feb 19, 2023

v19.7.0 proposal #46725

Merged

debadree25 mentioned this pull request Feb 27, 2023

[v18.x backport] stream: streams and webstreams interop related changes #46314

Closed

danielleadams mentioned this pull request Apr 11, 2023

v18.16.0 release proposal #47502

Merged

dsanders11 mentioned this pull request May 28, 2026

doc: add webstreams to args for pipeline from stream/promises #63628

Merged

+                });
+                const ws = new WritableStream({
+                  write(chunk) {
+                    values.push(chunk?.toString());

stream: add pipeline() for webstreams by debadree25 · Pull Request #46307 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: add pipeline() for webstreams - #46307

stream: add pipeline() for webstreams#46307nodejs-github-bot merged 26 commits intonodejs:mainnodejs/node:mainfrom debadree25:ft/pipeline-webstreamsdebadree25/node:ft/pipeline-webstreamsCopy head branch name to clipboard

Conversation

debadree25 commented Jan 22, 2023

Uh oh!

nodejs-github-bot commented Jan 22, 2023

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

debadree25 commented Jan 22, 2023

Uh oh!

ronag commented Jan 22, 2023

Uh oh!

debadree25 commented Jan 22, 2023

Uh oh!

debadree25 commented Jan 22, 2023

Uh oh!

debadree25 commented Jan 22, 2023

Uh oh!

debadree25 commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Jan 26, 2023

Uh oh!

debadree25 commented Jan 26, 2023

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

debadree25 commented Jan 26, 2023

Uh oh!

debadree25 commented Jan 26, 2023

Uh oh!

debadree25 commented Jan 26, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jan 29, 2023

Uh oh!

Uh oh!

Uh oh!

jasnell Jan 29, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25 Jan 29, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25 Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jan 30, 2023

Uh oh!

nodejs-github-bot commented Feb 2, 2023

Uh oh!

debadree25 commented Feb 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

stream: add pipeline() for webstreams#46307
nodejs-github-bot merged 26 commits into
nodejs:mainfrom
debadree25:ft/pipeline-webstreams

debadree25 commented Jan 25, 2023 •
edited
Loading