Skip to content

stream: add adapters for webstreams to node.js streams#39134

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
Closed
jasnell wants to merge 1 commit into from
Closed

stream: add adapters for webstreams to node.js streams #39134

jasnell wants to merge 1 commit into from

Conversation

@jasnell
Copy link
Member

@jasnelljasnell commented Jun 24, 2021•
edited
Loading

Experimental adapters for node.js streams and web streams.

Depends on #39062 (the first two commits here are from that PR and will be rebased out once that once lands)

@github-actionsgithub-actionsbot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jun 24, 2021
MattiasBuelens
MattiasBuelens reviewed Jun 24, 2021
View reviewed changes
@jasnelljasnell changed the title test: add WPT streams testsstream: add adapters for webstreams to node.js streamsJun 24, 2021
@jasnelljasnellforce-pushed the whatwg-streams-adapters branch 2 times, most recently from f359cb3 to 2db3dbbCompareJune 26, 2021 00:22
@mcollina
Copy link
Member

mcollina commented Jun 26, 2021

Good work!

@mcollina
Copy link
Member

mcollina commented Jun 26, 2021

I think this should include some code&test for finished() and pipeline() to support these.

@jasnelljasnellforce-pushed the whatwg-streams-adapters branch 2 times, most recently from 35c7181 to 0f4c4c9CompareJune 28, 2021 22:07
MattiasBuelens
MattiasBuelens reviewed Jun 28, 2021
View reviewed changes
@jasnell
Copy link
MemberAuthor

jasnell commented Jun 28, 2021

@mcollina:

I think this should include some code&test for finished() and pipeline() to support these.

Added!

@jasnelljasnell marked this pull request as ready for review June 28, 2021 23:14
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@jasnelljasnellforce-pushed the whatwg-streams-adapters branch from 9fa29ee to aebb324CompareJuly 8, 2021 14:17
@jasnelljasnell requested review from mcollina and ronagJuly 8, 2021 14:17
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 8, 2021

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

@mcollina
Copy link
Member

mcollina commented Jul 8, 2021

Added!

Where? I can't find them in the code. There are no changes to finished and pipeline to support whatwg streams.

@jasnell
Copy link
MemberAuthor

jasnell commented Jul 8, 2021

Where? I can't find them in the code.

I think I misunderstood. I added tests to show that the adapters work properly with finished and pipeline, but I have not modified either finish or pipeline to accept the web streams variants directly. I'd rather do that in a separate PR.

ronag
ronag reviewed Jul 8, 2021
View reviewed changes
Copy link
Member

@ronagronag left a comment

Choose a reason for hiding this comment

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

Could we land #39294 and use the utils from that?

ronag
ronag reviewed Jul 8, 2021
View reviewed changes
ronag
ronag reviewed Jul 8, 2021
View reviewed changes
ronag
ronag reviewed Jul 8, 2021
View reviewed changes
ronag
ronag reviewed Jul 8, 2021
View reviewed changes
Copy link
Member

@ronagronag left a comment

Choose a reason for hiding this comment

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

IMHO. The naming of the adapter methods are not very ergonomic....

ronag
ronag reviewed Jul 8, 2021
View reviewed changes
ronag
ronag reviewed Jul 8, 2021
View reviewed changes
ronag
ronag reviewed Jul 8, 2021
View reviewed changes
jasnell added a commit that referenced this pull request Jul 13, 2021
@jasnell
stream: implement streams to webstreams adapters
a99c230
Experimental adapters for the webstreams API Signed-off-by: James M Snell <[email protected]> PR-URL: #39134 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@jasnell
Copy link
MemberAuthor

jasnell commented Jul 13, 2021

Landed in a99c230

@jasnelljasnell closed this Jul 13, 2021
@bricssbricss mentioned this pull request Jul 13, 2021
Merged
@targos
Copy link
Member

targos commented Jul 17, 2021

This needs a backport to land on v16.x because it depends on the semver-major #39294

@MattiasBuelensMattiasBuelens mentioned this pull request Jul 20, 2021
Closed
This was referenced Jul 21, 2021
Closed
Closed
Closed
@targostargos mentioned this pull request Aug 22, 2021
Closed
@bricssbricss mentioned this pull request Aug 25, 2021
Merged
@targos
Copy link
Member

targos commented Oct 9, 2021

Does anyone want to backport this? Maybe @nodejs/backporters ?

@targostargos mentioned this pull request Oct 9, 2021
Closed
@Mesteery
Copy link
Contributor

Mesteery commented Oct 9, 2021

I'm willing to take care of it.

Mesteery pushed a commit to Mesteery/node that referenced this pull request Oct 9, 2021
@jasnell@Mesteery
stream: implement streams to webstreams adapters
5fb0526
Experimental adapters for the webstreams API Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#39134 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Mesteery pushed a commit to Mesteery/node that referenced this pull request Oct 9, 2021
@jasnell@Mesteery
stream: implement streams to webstreams adapters
155b677
Experimental adapters for the webstreams API Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#39134 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@MesteeryMesteery mentioned this pull request Oct 9, 2021
Closed
@benjamingr
Copy link
Member

benjamingr commented Oct 9, 2021

Hey just wondering - is there any reason you chose not to overload Readable.from? I skimmed discussion and missed it (toWeb makes sense regardless)

@MattiasBuelens
Copy link
Contributor

MattiasBuelens commented Oct 9, 2021•
edited
Loading

@benjamingrSee this comment. It's not off the table, but at least for now it seemed better to keep it separate as .fromWeb().

@targos
Copy link
Member

targos commented Oct 10, 2021

I'm going to keep the backport-requested label for some time, in case someone has an idea to backport without the need for semver-major changes.

@BethGriggsBethGriggs mentioned this pull request Oct 14, 2021
Merged
2 tasks
@juanarboljuanarbol mentioned this pull request May 31, 2022
Merged
@ehmickyehmicky mentioned this pull request Dec 15, 2023
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MattiasBuelensMattiasBuelensMattiasBuelens left review comments

@benjamingrbenjamingrbenjamingr left review comments

@targostargostargos left review comments

@mcollinamcollinamcollina approved these changes

@ronagronagronag approved these changes

+1 more reviewer

@devsnekdevsnekdevsnek left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@jasnell@mcollina@nodejs-github-bot@devsnek@ronag@MattiasBuelens@targos@Mesteery@benjamingr