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.

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 adapters for webstreams to node.js streams #39134

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from
+2,358 −0

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jun 24, 2021 •
edited

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-actions github-actions bot added lib / src needs-ci labels Jun 24, 2021
MattiasBuelens
MattiasBuelens reviewed Jun 24, 2021
View changes
lib/internal/webstreams/adapters.js Outdated Show resolved Hide resolved
lib/internal/webstreams/adapters.js Outdated Show resolved Hide resolved
lib/internal/webstreams/adapters.js Outdated Show resolved Hide resolved
lib/internal/webstreams/adapters.js Outdated Show resolved Hide resolved
lib/internal/webstreams/adapters.js Outdated Show resolved Hide resolved
lib/internal/webstreams/adapters.js Show resolved Hide resolved
lib/internal/webstreams/adapters.js Outdated Show resolved Hide resolved
lib/internal/webstreams/adapters.js Outdated Show resolved Hide resolved
@jasnell jasnell changed the title test: add WPT streams tests stream: add adapters for webstreams to node.js streams Jun 24, 2021
@jasnell jasnell force-pushed the whatwg-streams-adapters branch 2 times, most recently from f359cb3 to 2db3dbb Compare Jun 26, 2021
@mcollina
Copy link
Member

@mcollina mcollina commented Jun 26, 2021

Good work!

@mcollina
Copy link
Member

@mcollina mcollina commented Jun 26, 2021

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

@jasnell jasnell force-pushed the whatwg-streams-adapters branch 2 times, most recently from 35c7181 to 0f4c4c9 Compare Jun 28, 2021
MattiasBuelens
MattiasBuelens reviewed Jun 28, 2021
View changes
lib/internal/webstreams/adapters.js Outdated Show resolved Hide resolved
lib/internal/webstreams/adapters.js Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

@jasnell jasnell commented Jun 28, 2021

@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 Jun 28, 2021
@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 jasnell requested review from mcollina and ronag Jul 8, 2021
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jul 8, 2021

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

@mcollina
Copy link
Member

@mcollina 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
Member Author

@jasnell 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 changes
Copy link
Member

@ronag ronag left a comment

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

ronag
ronag reviewed Jul 8, 2021
View changes
lib/internal/webstreams/adapters.js Outdated Show resolved Hide resolved
ronag
ronag reviewed Jul 8, 2021
View changes
lib/internal/webstreams/adapters.js Outdated Show resolved Hide resolved
ronag
ronag reviewed Jul 8, 2021
View changes
lib/internal/webstreams/adapters.js Outdated Show resolved Hide resolved
ronag
ronag reviewed Jul 8, 2021
View changes
Copy link
Member

@ronag ronag left a comment

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

ronag
ronag reviewed Jul 8, 2021
View changes
lib/internal/webstreams/adapters.js Outdated Show resolved Hide resolved
@jasnell
streams: implement streams to webstreams adapters
2201e82 
Experimental adapters for the webstreams API

Signed-off-by: James M Snell <jasnell@gmail.com>
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jul 13, 2021 •
edited by jasnell

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

jasnell added a commit that referenced this issue 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
Copy link
Member Author

@jasnell jasnell commented Jul 13, 2021

Landed in a99c230

@jasnell jasnell closed this Jul 13, 2021
@bricss bricss mentioned this pull request Jul 13, 2021
Merged
@targos targos added the backport-requested-v16.x label Jul 17, 2021
@targos
Copy link
Member

@targos targos commented Jul 17, 2021

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

@MattiasBuelens MattiasBuelens mentioned this pull request Jul 20, 2021
Closed
@targos targos mentioned this pull request Jul 21, 2021
Closed
@targos targos mentioned this pull request Jul 25, 2021
Closed
@targos targos mentioned this pull request Jul 26, 2021
Closed
@targos targos mentioned this pull request Aug 22, 2021
Closed
@bricss bricss mentioned this pull request Aug 25, 2021
Merged
@targos
Copy link
Member

@targos targos commented Oct 9, 2021

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

@targos targos mentioned this pull request Oct 9, 2021
Closed
@Mesteery
Copy link
Member

@Mesteery Mesteery commented Oct 9, 2021

I'm willing to take care of it.

Mesteery added a commit to Mesteery/node that referenced this issue Oct 9, 2021
@jasnell @Mesteery
stream: implement streams to webstreams adapters
5fb0526 
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>
Mesteery added a commit to Mesteery/node that referenced this issue 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>
@Mesteery Mesteery mentioned this pull request Oct 9, 2021
Closed
@Mesteery Mesteery added the backport-open-v16.x label Oct 9, 2021
@targos targos removed the backport-requested-v16.x label Oct 9, 2021
@benjamingr
Copy link
Member

@benjamingr 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 MattiasBuelens commented Oct 9, 2021 •
edited

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

@Mesteery Mesteery removed the backport-open-v16.x label Oct 9, 2021
@targos targos added the backport-requested-v16.x label Oct 9, 2021
@targos
Copy link
Member

@targos 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.

@BethGriggs BethGriggs mentioned this pull request Oct 14, 2021
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MattiasBuelens MattiasBuelens

@benjamingr benjamingr

@targos targos

@mcollina mcollina

@ronag ronag

@devsnek devsnek

Assignees
No one assigned
Labels
author ready backport-requested-v16.x lib / src needs-ci
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@jasnell @mcollina @nodejs-github-bot @devsnek @ronag @MattiasBuelens @targos @Mesteery @benjamingr