stream: implement ReadableStream.from by debadree25 · Pull Request #48395 · nodejs/node · GitHub

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

cc @nodejs/whatwg-stream

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

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

lgtm

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

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

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?

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

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

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`
const originalReadableAsyncIterator = Readable.prototype[Symbol.asyncIterator];

// in readableStreamFromIterable
if (iterable instanceof Readable && iterable[Symbol.asyncIterator] === originalReadableAsyncIterator) {
 return Readable.toWeb(iterable);
}

This way, you can be certain that you're dealing with an unmodified Readable, 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. 😉

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

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

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

Fixes: nodejs#48389

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

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!

Landed in b361ad7

Fixes: nodejs#48389 PR-URL: nodejs#48395 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Fixes: #48389 PR-URL: #48395 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

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 cee7a5a Compare 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-actions Bot 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 outstanding review comments, 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

Comment thread lib/internal/webstreams/readablestream.js Outdated

benjamingr reviewed Jun 8, 2023

View reviewed changes

benjamingr approved these changes Jun 8, 2023

View reviewed changes

github-actions Bot 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

Comment thread lib/internal/webstreams/util.js Outdated

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 outstanding review comments, and a CI started. labels Jun 9, 2023

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

github-actions Bot 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

Comment thread lib/internal/webstreams/util.js Outdated

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-actions Bot mentioned this pull request Jun 13, 2023

CI Reliability 2023-06-13 nodejs/reliability#592

Open

22 tasks

github-actions Bot 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 ef12c54 Compare 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-actions Bot 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 outstanding review comments, 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:main Jul 18, 2023

MattiasBuelens mentioned this pull request Jul 20, 2023

Support ReadableStream.from() oven-sh/bun#3700

Closed

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

stream: implement ReadableStream.from by debadree25 · Pull Request #48395 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: implement ReadableStream.from - #48395

stream: implement ReadableStream.from#48395nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom debadree25:ft/readablestream-fromdebadree25/node:ft/readablestream-fromCopy head branch name to clipboard

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!

benjamingr Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25 Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25 Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

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

stream: implement ReadableStream.from#48395
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
debadree25:ft/readablestream-from

debadree25 commented Jun 8, 2023 •
edited
Loading