-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Implement Promise.allSettled #6138
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
Conversation
d747645
to
ce9eb8a
Compare
{ | ||
Assert(values != nullptr); | ||
|
||
TryCallResolveOrRejectHandler(promiseCapability->GetResolve(), values, scriptContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At https://tc39.github.io/proposal-promise-allSettled/#sec-performpromiseallsettled step 6.d.iii.2, it seems that if this call throws, then it is "caught" by allSettled
step 7, which then rejects the resulting promise with the exception value.
I believe the following:
function FakePromise(fn) {
function resolve() { print('resolve called'); throw new Error('oops'); }
function reject(e) { print('reject called: ' + e.message) }
fn(resolve, reject);
this.then = function(onResolve, onReject) {};
}
FakePromise.resolve = function() {};
Promise.allSettled.call(FakePromise, []);
should print:
resolve called
reject called: oops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are right, good catch. We have the same issue with Promise.all
, it looks like. I'll make the fix to both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we retrigger CI for those jobs that failed?
Wait, is that actually right? I thought the whole deal with |
@zenparsing I don't actually know how to retrigger the tests which failed in the new CI. These failures aren't related to my change, though, they've been failing across the CI lately. I'll rebase the branch which will trigger them to rerun, at least. |
@fatcerberus Yes, this reading of the spec is correct. We call |
Fixes: 6056
…completion while calling Promise.resolve When calling these methods with an empty iterator, they will call Promise.resolve synchronously with an empty array result. If Promise.resolve results in an abrupt completion, we are supposed to catch this and reject the result promise. Previously we were just letting this abrupt completion be leaked.
bb520b4
to
42c894d
Compare
Merge pull request #6138 from boingoing:promiseallsettled Implement Promise.allSettled This is now in Stage 3 and supported in V8 and JSC. See spec: https://tc39.github.io/proposal-promise-allSettled/ Fixes: #6056
Implement Promise.allSettled
This is now in Stage 3 and supported in V8 and JSC. See spec:
https://tc39.github.io/proposal-promise-allSettled/
Fixes: #6056