Skip to content
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

ES6Promise.Promise.prototype.finally does not behave according to spec #336

Closed
codeworrior opened this issue Sep 5, 2018 · 3 comments
Closed

Comments

@codeworrior
Copy link
Contributor

According to draft 4 of the Promise.prototype.finally spec , the callback parameter is kind of optional. When it is not a Callable, then should be invoked with the callback "as is", e.g. something like this:

if ( isFunction(callback) ) {
    // (old) invoke callback and return new promise, retaining result/reason
    return promise.then(value => constructor.resolve(callback()).then(() => value),
                       reason => constructor.resolve(callback()).then(() => { throw reason; }));
}

// (new) don't fail when callback is not callable; also return a new promise
return promise.then(callback, callback);

I understand that it doesn't seem to be reasonable to use finally without a callback.
But generic code that receives some callback as parameter and propagates it to a Promise.prototype.finally now additionally has to check it for being a function.
And I guess, a polyfill in general should be as close to the spec as possible.

Chrome 68, Firefox 61, Safari 11.1 all don't fail when executing

Promise.resolve(5).finally().then(console.log, console.log);
Promise.reject(5).finally().then(console.log, console.log);
@stefanpenner
Copy link
Owner

Good catch! As a spec reviewer I am embarrassed this slipped past my implementations.

I’ll accept a PR, I should have time in the next day or so to fix myself if someone else doesn’t beat me to it

codeworrior added a commit to codeworrior/es6-promise that referenced this issue Sep 7, 2018
@codeworrior
Copy link
Contributor Author

-> PR #339.

Credits for the finding go to @ThomasChadzelek and @PeterBuchholz.

stefanpenner added a commit that referenced this issue Sep 10, 2018
…lback

[Fixes #336] Promise.prototype.finally must not fail for a non-callable callback
@stefanpenner
Copy link
Owner

stefanpenner commented Sep 10, 2018

fix released as v4.2.5 🎉

thanks @codeworrior, @ThomasChadzelek and @PeterBuchholz for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants