stream: implement ReadableStream.from#48395

debadree25 · 2023-06-08T18:03:33Z

Implements ReadableStream.from

Updating the WPTs add a lot of new tests, and failures that weren't relevant to this PR, I have added them to expected failures.

Please do take a look at the node errors used, I have used ERR_INVALID_STATE.TypeError in all the places since all the webstreams code seems to be reliant on that.

Fixes: #48389

debadree25 · 2023-06-08T18:41:15Z

cc @nodejs/whatwg-stream

debadree25 · 2023-06-08T18:43:16Z

Also, would this need node tests too? or WPTs are enough?

nodejs-github-bot · 2023-06-08T18:52:10Z

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

mcollina

lgtm

lib/internal/webstreams/readablestream.js

benjamingr · 2023-06-08T20:22:05Z

lib/internal/webstreams/readablestream.js

+constiterator=iteratorRecord.iterator;
+letreturnMethod;
+try{
+returnMethod=iterator.return;


Can't this be an async function which would allow the catch to not exist nd the below return PromiseResolve() to just be a return etc?

Hmm i think it could, let me make it so and see if the WPTs pass

Hey! updated this, the WPTs don't complain and reading the spec i doesn't seem to say anything about the function having to sync or async

Indeed, this is equivalent. I wanted the error handling to be very explicit in the reference implementation, but practical implementations can be more succinct. 🙂

benjamingr

Mostly LGTM (esp for a first version).

A few notes:

We can simplify this a bit with async functions
We should short circuit if we get a Node stream and use toWeb machinary if the spec allows

lib/internal/webstreams/util.js

debadree25 · 2023-06-09T17:19:52Z

We should short circuit if we get a Node stream and use toWeb machinary if the spec allows

where to check for this? should make a issue on the spec repo? maybe @MattiasBuelens can help?

debadree25 · 2023-06-09T17:59:45Z

have addressed all the comments and will wait for some time if we would like to add .toWeb functionality like @benjamingr suggested

nodejs-github-bot · 2023-06-09T18:11:48Z

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

lib/internal/webstreams/util.js

MattiasBuelens · 2023-06-12T09:34:01Z

We should short circuit if we get a Node stream and use toWeb machinary if the spec allows
where to check for this? should make a issue on the spec repo? maybe @MattiasBuelens can help?

I would recommend something like:

// at the top-level// ideally, this is exported by `lib/internal/streams/readable.js`constoriginalReadableAsyncIterator=Readable.prototype[Symbol.asyncIterator];// in readableStreamFromIterableif(iterableinstanceofReadable&&iterable[Symbol.asyncIterator]===originalReadableAsyncIterator){returnReadable.toWeb(iterable);}

This way, you can be certain that you're dealing with an unmodifiedReadable, and that there will be no behavioral differences compared to "going the long way" using Readable's async iterator.

I also suggest you write additional tests with polluted Readable instances, or with a polluted Readable.prototype. 😉

debadree25 · 2023-06-13T20:28:19Z

Hey one thing is it ok to export things from lib/internal/streams/readable.js something like originalReadableAsyncIterator as suggested? i know there is little bit restrictions in what we can export import in around the readable node stream part since its used in readablestream@benjamingr @mcollina

nodejs-github-bot · 2023-07-13T18:05:36Z

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

nodejs-github-bot · 2023-07-13T18:24:16Z

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

Fixes: nodejs#48389

nodejs-github-bot · 2023-07-13T18:39:22Z

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

debadree25 · 2023-07-14T06:52:14Z

Hello everyone! I have reverted the change of toWeb for now (I shall do it in a follow up, since i was getting confused 😅) PTAL it seems ready to land!

nodejs-github-bot · 2023-07-18T09:50:24Z

Landed in b361ad7

Fixes: nodejs#48389 PR-URL: nodejs#48395 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Fixes: #48389 PR-URL: #48395 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Jun 8, 2023

debadree25 marked this pull request as ready for review June 8, 2023 18:29

debadree25 force-pushed the ft/readablestream-from branch from 48a8af4 to cee7a5aCompare June 8, 2023 18:39

debadree25 added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2023

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2023

mcollina approved these changes Jun 8, 2023
View reviewed changes

debadree25 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 8, 2023

KhafraDev approved these changes Jun 8, 2023
View reviewed changes

benjamingr reviewed Jun 8, 2023
View reviewed changes

lib/internal/webstreams/readablestream.js Outdated Show resolvedHide resolved

benjamingr reviewed Jun 8, 2023
View reviewed changes

benjamingr approved these changes Jun 8, 2023
View reviewed changes

github-actionsbot mentioned this pull request Jun 9, 2023
CI Reliability 2023-06-09 nodejs/reliability#588
Open
24 tasks

marco-ippolito reviewed Jun 9, 2023
View reviewed changes

lib/internal/webstreams/util.jsShow resolvedHide resolved

debadree25 added request-ci Add this label to start a Jenkins CI on a PR. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 9, 2023

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 9, 2023

github-actionsbot mentioned this pull request Jun 10, 2023
CI Reliability 2023-06-10 nodejs/reliability#589
Open
23 tasks

aduh95 reviewed Jun 10, 2023
View reviewed changes

lib/internal/webstreams/util.jsShow resolvedHide resolved

This was referenced Jun 11, 2023
CI Reliability 2023-06-11 nodejs/reliability#590
Open
CI Reliability 2023-06-12 nodejs/reliability#591
Open

github-actionsbot mentioned this pull request Jun 13, 2023
CI Reliability 2023-06-13 nodejs/reliability#592
Open
22 tasks

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 13, 2023

stream: implement ReadableStream.from
ef12c54
Fixes: nodejs#48389

debadree25 force-pushed the ft/readablestream-from branch from bfefd98 to ef12c54Compare July 13, 2023 18:30

debadree25 added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 13, 2023

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 13, 2023

debadree25 requested review from benjamingr and mcollina July 14, 2023 06:53

debadree25 removed the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jul 14, 2023

This was referenced Jul 16, 2023
CI Reliability 2023-07-16 nodejs/reliability#614
Open
CI Reliability 2023-07-17 nodejs/reliability#615
Open

benjamingr approved these changes Jul 18, 2023
View reviewed changes

debadree25 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 18, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 18, 2023

nodejs-github-bot merged commit b361ad7 into nodejs:mainJul 18, 2023

MattiasBuelens mentioned this pull request Jul 20, 2023
Support ReadableStream.from()oven-sh/bun#3700
Open

RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
stream: implement ReadableStream.from
609b25f
Fixes: #48389 PR-URL: #48395 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

UlisesGascon mentioned this pull request Aug 15, 2023
v20.6.0 proposal #49185
Merged

mcollina mentioned this pull request Sep 8, 2023
regression: Response does not accept a async-iterator anymore #49551
Closed

jcbhmr mentioned this pull request Nov 3, 2023
api.ReadableStream.from_static - nodejs/deno versions mdn/browser-compat-data#21152
Closed

Uh oh!

stream: implement ReadableStream.from#48395

stream: implement ReadableStream.from #48395

Uh oh!

Conversation

debadree25 commented Jun 8, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

debadree25 commented Jun 8, 2023

Uh oh!

debadree25 commented Jun 8, 2023

Uh oh!

nodejs-github-bot commented Jun 8, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benjamingrJun 8, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

MattiasBuelensJun 26, 2023

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

debadree25 commented Jun 9, 2023

Uh oh!

debadree25 commented Jun 9, 2023

Uh oh!

nodejs-github-bot commented Jun 9, 2023

Uh oh!

Uh oh!

MattiasBuelens commented Jun 12, 2023

Uh oh!

debadree25 commented Jun 13, 2023

Uh oh!

nodejs-github-bot commented Jul 13, 2023

Uh oh!

nodejs-github-bot commented Jul 13, 2023

Uh oh!

nodejs-github-bot commented Jul 13, 2023

Uh oh!

debadree25 commented Jul 14, 2023

Uh oh!

nodejs-github-bot commented Jul 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

debadree25 commented Jun 8, 2023•
edited
Loading