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 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

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 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
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 June 26, 2021 00:22
@mcollina
Copy link
Member

Good work!

@mcollina
Copy link
Member

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 June 28, 2021 22:07
MattiasBuelens
MattiasBuelens reviewed Jun 28, 2021
View reviewed 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 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 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 jasnell requested review from mcollina and ronag July 8, 2021 14:17
@nodejs-github-bot
Copy link
Collaborator

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

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

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

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

ronag
ronag reviewed Jul 8, 2021
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.

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

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

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
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

@MattiasBuelens MattiasBuelens mentioned this pull request Jul 20, 2021
Closed
This was referenced Jul 21, 2021
Closed
Closed
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 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 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 <jasnell@gmail.com>

PR-URL: nodejs#39134
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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>
@Mesteery Mesteery mentioned this pull request Oct 9, 2021
Closed
@benjamingr
Copy link
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
Copy link
Contributor

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().

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

@BethGriggs BethGriggs mentioned this pull request Oct 14, 2021
Merged
2 tasks
@juanarbol juanarbol mentioned this pull request May 31, 2022
Merged
@ehmicky ehmicky mentioned this pull request Dec 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MattiasBuelens MattiasBuelens MattiasBuelens left review comments

@benjamingr benjamingr benjamingr left review comments

@targos targos targos left review comments

@mcollina mcollina mcollina approved these changes

@ronag ronag ronag approved these changes

@devsnek devsnek devsnek left review comments

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
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