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.

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

stream: add pipeline() for webstreams #46307

Merged
merged 26 commits into from Feb 2, 2023
Merged

stream: add pipeline() for webstreams #46307

merged 26 commits into from Feb 2, 2023

Conversation

debadree25
Copy link
Member

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

Refs: #39316

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 22, 2023
ronag
ronag reviewed Jan 22, 2023
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if we could avoid transforming into node stream

@debadree25
Copy link
Member Author

I would prefer if we could avoid transforming into node stream

Any suggestion what alternative you would prefer?

@ronag
Copy link
Member

ronag commented Jan 22, 2023

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

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 debadree25 marked this pull request as draft January 22, 2023 14:03
@debadree25
Copy link
Member Author

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

@debadree25 debadree25 closed this Jan 22, 2023
@debadree25 debadree25 reopened this Jan 22, 2023
@debadree25
Copy link
Member Author

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

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

would this be an acceptable path?

@debadree25
Copy link
Member Author

debadree25 commented Jan 25, 2023 •
edited

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
Copy link
Member

ronag commented Jan 26, 2023

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

@debadree25 debadree25 marked this pull request as ready for review January 26, 2023 14:49
@debadree25 debadree25 marked this pull request as draft January 26, 2023 14:49
@debadree25
Copy link
Member Author

Reopening for review all the tests passing!

@debadree25 debadree25 marked this pull request as ready for review January 26, 2023 16:10
@debadree25 debadree25 requested a review from ronag January 26, 2023 16:10
ronag
ronag reviewed Jan 26, 2023
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make two separate pump functions.

@debadree25
Copy link
Member Author

Ok refactoring

@debadree25
Copy link
Member Author

I would make two separate pump functions.

The code would be duplicated no?

@debadree25
Copy link
Member Author

Have updated to use a separate function

@debadree25 debadree25 requested a review from ronag January 26, 2023 18:48
ronag
ronag approved these changes Jan 29, 2023
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mcollina
mcollina approved these changes Jan 29, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2023
@nodejs-github-bot
Copy link
Collaborator

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

jasnell
jasnell reviewed Jan 29, 2023
View reviewed changes
test/parallel/test-webstreams-pipeline.js Outdated
});
const ws = new WritableStream({
write(chunk) {
values.push(chunk?.toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure updating

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@ronag ronag added stream Issues and PRs related to the stream subsystem. 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. web streams and removed needs-ci PRs that need a full CI run. labels Jan 30, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 30, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@lpinca 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 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 nodejs-github-bot merged commit 23effb2 into nodejs:main Feb 2, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 23effb2

@debadree25
Copy link
Member Author

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

MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
@debadree25 @MylesBorins
stream: add pipeline() for webstreams
c206853 
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>
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
Merged
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
@debadree25
stream: add pipeline() for webstreams
9933495 
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>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
@debadree25
stream: add pipeline() for webstreams
58f6443 
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>
@debadree25 debadree25 mentioned this pull request Feb 27, 2023
Closed
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@debadree25 @danielleadams
stream: add pipeline() for webstreams
6c454af 
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>
@danielleadams danielleadams mentioned this pull request Apr 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

@mcollina mcollina mcollina approved these changes

@benjamingr benjamingr benjamingr 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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. stream Issues and PRs related to the stream subsystem. web streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@debadree25 @nodejs-github-bot @ronag @mcollina @jasnell @benjamingr @lpinca