stefanpenner / es6-promise

A polyfill for ES6-style Promises
MIT License
7.3k stars 593 forks source link

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

Closed codeworrior closed 5 years ago

codeworrior commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

-> PR #339.

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

stefanpenner commented 5 years ago

fix released as v4.2.5 🎉

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