FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: add adapters for webstreams to node.js streams by jasnell · Pull Request #39134 · nodejs/node · GitHub

/ node Public

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

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:whatwg-streams-adapters
ConversationCommits (1)ChecksFiles changed
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 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-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 reviewed Jun 24, 2021
View reviewed changes
Comment thread lib/internal/webstreams/adapters.js Outdated
Comment thread lib/internal/webstreams/adapters.js Outdated
Comment thread lib/internal/webstreams/adapters.js Outdated
Comment thread lib/internal/webstreams/adapters.js Outdated
Comment thread lib/internal/webstreams/adapters.js Outdated
Comment thread lib/internal/webstreams/adapters.js Outdated
Comment thread lib/internal/webstreams/adapters.js Outdated
Comment thread lib/internal/webstreams/adapters.js Outdated
jasnell changed the title test: add WPT streams tests stream: add adapters for webstreams to node.js streams Jun 24, 2021
jasnell force-pushed the whatwg-streams-adapters branch 2 times, most recently from f359cb3 to 2db3dbb Compare June 26, 2021 00:22

mcollina commented Jun 26, 2021

Copy link
Copy Markdown
Member

Good work!

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 force-pushed the whatwg-streams-adapters branch 2 times, most recently from 35c7181 to 0f4c4c9 Compare June 28, 2021 22:07
MattiasBuelens reviewed Jun 28, 2021
View reviewed changes
Comment thread lib/internal/webstreams/adapters.js Outdated
Comment thread lib/internal/webstreams/adapters.js Outdated

jasnell commented Jun 28, 2021

Copy link
Copy Markdown
Member Author

@mcollina:

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

Added!

jasnell marked this pull request as ready for review June 28, 2021 23:14

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view
jasnell force-pushed the whatwg-streams-adapters branch from 9fa29ee to aebb324 Compare July 8, 2021 14:17
jasnell requested review from mcollina and ronag July 8, 2021 14:17

nodejs-github-bot commented Jul 8, 2021

Copy link
Copy Markdown
Collaborator

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

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 commented Jul 8, 2021

Copy link
Copy Markdown
Member Author

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 reviewed Jul 8, 2021
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

ronag reviewed Jul 8, 2021
View reviewed changes
Comment thread lib/internal/webstreams/adapters.js Outdated
ronag reviewed Jul 8, 2021
View reviewed changes
Comment thread lib/internal/webstreams/adapters.js Outdated
ronag reviewed Jul 8, 2021
View reviewed changes
Comment thread lib/internal/webstreams/adapters.js Outdated
ronag reviewed Jul 8, 2021
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

ronag reviewed Jul 8, 2021
View reviewed changes
Comment thread lib/internal/webstreams/adapters.js Outdated
ronag reviewed Jul 8, 2021
View reviewed changes
Comment thread lib/internal/webstreams/adapters.js Outdated
ronag reviewed Jul 8, 2021
View reviewed changes
Comment thread lib/internal/webstreams/adapters.js Outdated
jasnell added a commit that referenced this pull request Jul 13, 2021
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 commented Jul 13, 2021

Copy link
Copy Markdown
Member Author

Landed in a99c230

jasnell closed this Jul 13, 2021
bricss mentioned this pull request Jul 13, 2021
Merged

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

MattiasBuelens mentioned this pull request Jul 20, 2021
Closed
This was referenced Jul 21, 2021
Closed
Closed
Closed
targos mentioned this pull request Aug 22, 2021
Closed
bricss mentioned this pull request Aug 25, 2021
Merged

targos commented Oct 9, 2021

Copy link
Copy Markdown
Member

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

targos mentioned this pull request Oct 9, 2021
Closed

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
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 mentioned this pull request Oct 9, 2021
Closed

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 commented Oct 9, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

BethGriggs mentioned this pull request Oct 14, 2021
Merged
2 tasks
juanarbol mentioned this pull request May 31, 2022
Merged
ehmicky mentioned this pull request Dec 15, 2023
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

MattiasBuelens MattiasBuelens left review comments
benjamingr benjamingr left review comments
mcollina mcollina approved these changes
ronag ronag approved these changes
+2 more reviewers
targos targos left review comments
devsnek devsnek left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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.

9 participants

Back | FazBrowse Home | New Git URL