streams: fixes for webstreams #51168
Conversation
|
Review requested:
|
lib/internal/webstreams/util.js
Outdated
| function ensureIsPromise(fn, thisArg, ...args) { | ||
| try { | ||
| const value = FunctionPrototypeCall(fn, thisArg, ...args); | ||
| return isPromise(value) ? value : PromiseResolve(value); | ||
| return new Promise((r) => r(value)); | ||
| } catch (error) { | ||
| return PromiseReject(error); | ||
| } | ||
| } |
There was a problem hiding this comment.
if it's always making a new promise, any reason not to do this?
| function ensureIsPromise(fn, thisArg, ...args) { | |
| try { | |
| const value = FunctionPrototypeCall(fn, thisArg, ...args); | |
| return isPromise(value) ? value : PromiseResolve(value); | |
| return new Promise((r) => r(value)); | |
| } catch (error) { | |
| return PromiseReject(error); | |
| } | |
| } | |
| async function ensureIsPromise(fn, thisArg, ...args) { | |
| return await FunctionPrototypeCall(fn, thisArg, ...args); | |
| } |
There was a problem hiding this comment.
I like that a lot! An async function always returns a fresh Promise, so this should satisfy Web IDL. 🙂
There was a problem hiding this comment.
Unfortunately, this change causes some tests to fail again... 😞
However, if you remove the await, it does work:
async function ensureIsPromise(fn, thisArg, ...args) {
return FunctionPrototypeCall(fn, thisArg, ...args);
}You still get a fresh Promise (from async function), errors are still correctly turned into rejections, and the timings match Web IDL.
Still, I don't like how sensitive these tests are to the number of microtasks. I'll try to fix this in upstream WPT.
|
cc @nodejs/whatwg-stream |
github-actions
bot
removed
the
request-ci
mcollina
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 20c6313 |
PR-URL: #51168 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
marco-ippolito
added
the
backport-requested-v20.x
|
this doesn't land cleanly on v20 requires a manual backport |
|
@marco-ippolito This PR builds upon #50107, specifically: 0d45e6f needs a85e418. However, that other PR is marked "dont-land-on-v20.x". 😕 I'm not sure how to proceed with a manual backport. Should I try to rework my changes without that PR, or should we backport #50107 to v20.x anyway? |
marco-ippolito
added
dont-land-on-v20.x
PR-URL: nodejs#51168 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
|
On second thought, I think I can rework my changes. Let me try something... 👨🔬 |
PR-URL: nodejs#51168 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
|
@marco-ippolito I managed to make it work without #50107, see #52773. However, If we also want to backport #50107, then I'll need to change it up again. 😛 Let me know how you want to proceed. |
|
I marked it dont-land because it depended on #47956, which was dont-land. Feel free to remove the label and backport. |
marco-ippolito
added
backport-open-v20.x
PR-URL: nodejs#51168 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#51168 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#51168 Backport-PR-URL: nodejs#52773 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Follow-up on #50126.
PromiseResolveinensureIsPromise. Web IDL requires this to always return a newPromise, whereasPromiseResolvemay return the samePromise. This leads to a different timing, sincenew Promise()takes at least one microtask to resolve. See my previous comment.ensureIsPromise().TeeReadableStream. This should construct an internal stream with internal callbacks usingsetupReadableStreamDefaultController, but it was incorrectly using user-land callbacks withsetupReadableStreamDefaultControllerFromSource.ReadableStreamconstructor, which is incorrect. The new implementation mirrors how we handle teeing "default" readable streams.ReadableStream.from(). Again, this was incorrectly using the user-landReadableStreamconstructor.TransformStream. Same thing again.ReadableStreamandWritableStream. This code was duplicated a few times, so it made sense to extract a helper function.ReadableStreaminternal state. This seemed like a copy-paste mistake: other classes (likeReadableStreamDefaultController) do have astreamfield in their state, butReadableStreamshouldn't.typeof callback === "function"by virtue of bluntly trying to callFunctionPrototypeBindon them. We no longer useFunctionPrototypeBindfor this, so we need to manually callvalidateFunctionfor each callback. I've wrapped that in acreatePromiseCallbackhelper.