Skip to content

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

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:whatwg-streams-adapters
Closed

stream: add adapters for webstreams to node.js streams#39134
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:whatwg-streams-adapters

Conversation

@jasnell

@jasnelljasnell commented Jun 24, 2021•
edited
Loading

Copy link
Copy Markdown
Member

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
Comment threadlib/internal/webstreams/adapters.js Outdated
Comment threadlib/internal/webstreams/adapters.js Outdated
Comment threadlib/internal/webstreams/adapters.js Outdated
Comment threadlib/internal/webstreams/adapters.js Outdated
Comment threadlib/internal/webstreams/adapters.js Outdated
Comment threadlib/internal/webstreams/adapters.js Outdated
Comment threadlib/internal/webstreams/adapters.js Outdated
Comment threadlib/internal/webstreams/adapters.js Outdated
@jasnelljasnell changed the title test: add WPT streams testsstream: add adapters for webstreams to node.js streamsJun 24, 2021
@jasnell
jasnellforce-pushed the whatwg-streams-adapters branch 2 times, most recently from f359cb3 to 2db3dbbCompareJune 26, 2021 00:22
@mcollina

mcollina commented Jun 26, 2021

Copy link
Copy Markdown
Member

Good work!

@mcollina

mcollina commented Jun 26, 2021

Copy link
Copy Markdown
Member

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

@jasnell
jasnellforce-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
Comment threadlib/internal/webstreams/adapters.js Outdated
Comment threadlib/internal/webstreams/adapters.js Outdated
@jasnell

jasnell commented Jun 28, 2021

Copy link
Copy Markdown
MemberAuthor

@mcollina:

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

Added!

@jasnell
jasnell 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
@jasnell
jasnellforce-pushed the whatwg-streams-adapters branch from 9fa29ee to aebb324CompareJuly 8, 2021 14:17
@jasnell
jasnell requested review from mcollina and ronagJuly 8, 2021 14:17
@nodejs-github-bot

nodejs-github-bot commented Jul 8, 2021

Copy link
Copy Markdown
Collaborator

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

@mcollina

mcollina commented Jul 8, 2021

Copy link
Copy Markdown
Member

Added!

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

@jasnell

jasnell commented Jul 8, 2021

Copy link
Copy Markdown
MemberAuthor

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

@ronagronag left a comment

Copy link
Copy Markdown
Member

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
Comment threadlib/internal/webstreams/adapters.js Outdated
ronag
ronag reviewed Jul 8, 2021
View reviewed changes
Comment threadlib/internal/webstreams/adapters.js Outdated
ronag
ronag reviewed Jul 8, 2021
View reviewed changes
Comment threadlib/internal/webstreams/adapters.js Outdated
ronag
ronag reviewed Jul 8, 2021
View reviewed changes

@ronagronag left a comment

Copy link
Copy Markdown
Member

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
Comment threadlib/internal/webstreams/adapters.js Outdated
ronag
ronag reviewed Jul 8, 2021
View reviewed changes
Comment threadlib/internal/webstreams/adapters.js Outdated
ronag
ronag reviewed Jul 8, 2021
View reviewed changes
Comment threadlib/internal/webstreams/adapters.js Outdated
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 <jasnell@gmail.com>
PR-URL: #39134
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell

jasnell commented Jul 13, 2021

Copy link
Copy Markdown
MemberAuthor

Landed in a99c230

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

targos commented Jul 17, 2021

Copy link
Copy Markdown
Member

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

targos commented Oct 9, 2021

Copy link
Copy Markdown
Member

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

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

Mesteery commented Oct 9, 2021

Copy link
Copy Markdown
Contributor

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
155b677
Experimental adapters for the webstreams API
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#39134
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MesteeryMesteery mentioned this pull request Oct 9, 2021
Closed
@benjamingr

benjamingr commented Oct 9, 2021

Copy link
Copy Markdown
Member

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

MattiasBuelens commented Oct 9, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@targos

targos commented Oct 10, 2021

Copy link
Copy Markdown
Member

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