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

stream: implement ReadableStream.from #48395

Merged
merged 1 commit into from
Jul 18, 2023

Conversation

debadree25
Copy link
Member

@debadree25 debadree25 commented Jun 8, 2023

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

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Jun 8, 2023
@debadree25 debadree25 marked this pull request as ready for review June 8, 2023 18:29
@debadree25
Copy link
Member Author

cc @nodejs/whatwg-stream

@debadree25
Copy link
Member Author

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

@debadree25 debadree25 added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@debadree25 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
const iterator = iteratorRecord.iterator;
let returnMethod;
try {
returnMethod = iterator.return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@debadree25
Copy link
Member Author

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

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

@debadree25 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-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 9, 2023
@nodejs-github-bot
Copy link
Collaborator

@MattiasBuelens
Copy link
Contributor

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

@debadree25
Copy link
Member Author

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

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 13, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@debadree25 debadree25 added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 13, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 13, 2023
@nodejs-github-bot
Copy link
Collaborator

@debadree25
Copy link
Member Author

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!

@debadree25 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
@debadree25 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 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 nodejs-github-bot merged commit b361ad7 into nodejs:main Jul 18, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in b361ad7

pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
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>
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
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>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
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>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
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>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. web streams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Streams: support ReadableStream.from()
8 participants