stream: implement ReadableStream.from#48395
Conversation
48a8af4 to
cee7a5a
Compare
|
cc @nodejs/whatwg-stream |
|
Also, would this need node tests too? or WPTs are enough? |
| const iterator = iteratorRecord.iterator; | ||
| let returnMethod; | ||
| try { | ||
| returnMethod = iterator.return; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Hmm i think it could, let me make it so and see if the WPTs pass
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
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 |
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 I also suggest you write additional tests with polluted |
|
Hey one thing is it ok to export things from |
bfefd98 to
ef12c54
Compare
|
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: 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: 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: 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: 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>
Implements
ReadableStream.fromUpdating 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.TypeErrorin all the places since all the webstreams code seems to be reliant on that.Fixes: #48389