node
node copied to clipboard
nodejs
stream: implement ReadableStream.from
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: https://github.com/nodejs/node/issues/48389
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
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
Updated the docs, just question from https://github.com/nodejs/node/pull/48395#issuecomment-1589977209 remains
friendly ping regards to this question https://github.com/nodejs/node/pull/48395#issuecomment-1589977209 @benjamingr 😅😄
Hey @benjamingr added the short circuit with toWeb() as you suggested used symbols to communicate whether the iterator prototype is original or polluted, PTAL if it makes sense!
Thank You!
Does anyone know what this failure means:
"readable-streams/read-task-handling.window.js": {
"fail": {
"unexpected": [
"evaluation in WPTRunner.runJsTests()"
]
}
seems to be failing in some systems so inclined to believe this is something flakey
added skips for some browser-specific tests, please PTAL and re-approve 😅
Hey everyone pinging again just blocked on what should be dont for https://github.com/nodejs/node/pull/48395#pullrequestreview-1470701774 cc @mcollina @benjamingr maybe we could land this implementation and do the toWeb thing in a follow up?
As a casual observer, mixing node streams with web streams will only lead to harder to maintain code. IMO all of the node stream stuff should be removed.
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!