Fetch: multi global tests #24601
Fetch: multi global tests #24601
Conversation
wpt-pr-bot
requested review from
domfarolino,
jdm,
mnot,
youennf and
yutakahirano
|
Feedback on the exception test which likely applies elsewhere:
|
annevk
force-pushed
the
annevk/fetch-multi-global
branch
from
f888fdc to
796a9fe
Compare
|
@domenic could you take another look at this? What I get now is that everything is relevant, except for rejected promises and their exception. |
| // Verify rejected promises | ||
| return Promise.all(bodyMethods.map(method => { | ||
| const rejectedPromise = currentFrame.contentWindow.Response.prototype[method].call(response); | ||
| assert_true(isObjectFromGlobal(rejectedPromise, currentFrame.contentWindow), `Response's ${method}()'s rejected promise should use the current global`) |
There was a problem hiding this comment.
Shouldn't this be relevant? The exception should be current, but the promise should be relevant, I believe.
I'm not too happy with that inconsistency myself, but the problem is overconstrained:
- We decided the web platform uses relevant by default
- ES uses current by default
- We decided Web IDL exception creation and throwing matches ES
- Promises are created explicitly and we decided they should match the web platform
There was a problem hiding this comment.
Yeah, I wanted to discuss this. I think the problem is that browsers let the IDL layer construct the promise. We discussed this previously about the difference between specifications using "throw" and "return a rejected promise" I think. As far as I can tell browsers might use "throw". Or am I missing something?
(Note that browsers pass this test as-is.)
cc @TimothyGu @yuki3
There was a problem hiding this comment.
Yeah, in Chrome at least a lot of code uses throw and the bindings layer converts it into a rejected promise. (I'm not sure how this works for in-parallel cases; maybe it's just for synchronously-detectable error conditions?) But I think the bindings layer could still convert it into a rejected promise in the relevant realm wrapping an exception in the current realm.
There was a problem hiding this comment.
I see, so you want browsers to change their behavior? I suspect that would also require changes to IDL.
There was a problem hiding this comment.
I mean, what I really want is to use current everywhere, but I gave up on that due to bz's pushback.
My fallback position is to maintain as much local consistency as possible, and as part of that I think it's important that rejected promises and fulfilled promises have the same realm. We could make that realm either current or relevant. Relevant seems to maximize consistency and minimize confusion in giving spec-authoring guidance more. If we did current we'd have to update our guidance to something like "use relevant, except for promises; use current for those". That seems a bit bad?
I guess we would need IDL changes; https://heycam.github.io/webidl/#dfn-create-operation-function implicitly uses the current Realm (via %Promise% syntax).
There was a problem hiding this comment.
About Chrome's bindings, nothing to add to domenic's understanding. In synchronous cases, "throw an exception -> convert it to a rejected promise" is done in bindings layer. Generally bindings layer doesn't take care of "in parallel" cases, but provides some utility functions.
As long as it's consistent and clean, spec authors won't be confused, and no trouble at the end users, I'm fine to change the realm of (rejected) promises to the current realm. I'm fine with a proposal of: interface => relevant, others => current.
Just my two cents.
There was a problem hiding this comment.
Thanks @yuki3. I guess we need to solve whatwg/webidl#135 first then.
For whatwg/fetch#777.