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

debadree25 · 2023-01-22T13:52:07Z

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

nodejs-github-bot · 2023-01-22T13:52:11Z

Review requested:

@nodejs/streams

ronag

I would prefer if we could avoid transforming into node stream

debadree25 · 2023-01-22T13:55:35Z

I would prefer if we could avoid transforming into node stream

Any suggestion what alternative you would prefer?

ronag · 2023-01-22T14:01:12Z

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.

debadree25 · 2023-01-22T14:03:25Z

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

debadree25 · 2023-01-22T15:03:39Z

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

debadree25 · 2023-01-22T19:26:18Z

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?

debadree25 · 2023-01-25T19:59:57Z

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?

ronag · 2023-01-26T07:34:08Z

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

debadree25 · 2023-01-26T16:10:27Z

Reopening for review all the tests passing!

ronag

I would make two separate pump functions.

debadree25 · 2023-01-26T16:18:18Z

Ok refactoring

debadree25 · 2023-01-26T16:33:20Z

I would make two separate pump functions.

The code would be duplicated no?

debadree25 · 2023-01-26T17:12:43Z

Have updated to use a separate function

mcollina

lgtm

nodejs-github-bot · 2023-01-29T15:53:40Z

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

jasnell · 2023-01-29T16:44:11Z

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


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

sure updating

nodejs-github-bot · 2023-01-30T07:54:18Z

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

nodejs-github-bot · 2023-02-02T19:45:45Z

Landed in 23effb2

debadree25 · 2023-02-02T19:53:44Z

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-actionsBot 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 threadlib/internal/streams/pipeline.js

jasnell reviewed Jan 29, 2023
View reviewed changes

Comment threadlib/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-actionsBot 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:mainFeb 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

Uh oh!

Uh oh!

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!

jasnellJan 29, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25Jan 29, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25Jan 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

debadree25 commented Jan 25, 2023•
edited
Loading