Skip to content

bug(esnext): add definition for promise.finally #20511

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

Merged
merged 1 commit into from
Jan 4, 2018

Conversation

benbraou
Copy link
Contributor

@benbraou benbraou commented Dec 6, 2017

This PR adds definition for Promise.prototype.finally

promise.finally is marked as esnext for the moment as it is not yet part of the finished proposals

Fixes #20411

*/
finally<TResult = never>(
onfinally?: (() => TResult | PromiseLike<TResult>) | undefined | null
): Promise<T | TResult>;
Copy link

@Jessidhia Jessidhia Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result of the onfinally callback is always ignored (unless it's a rejection). The way it works on the spec is pretty complicated, but it essentially wraps the onfinally argument with a function that returns a Promise with the original Promise's state. It does allow for the onfinally callback to throw, though, and that new exception would become the new rejection value, but it's not possible to alter the resolve value.

finally(onfinally?: (() => void) | undefined | null): Promise<T>

is the correct signature, unless we want to provide some way to support the result of Promise.reject as a return value; but that can be done just as well by throwing inside the onfinally.

Copy link
Contributor Author

@benbraou benbraou Dec 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kovensky thanks for the review.
I have one remark though, going with the return type void also means that the compiler won't complain if onfinally simply throws an error or returns any thing (since that is assignable to void). I guess that this is what we would like to have.
Also, it is indeed possible to have a Promise.reject (assignable to void) within finally

const generatePromise = (value)=> new Promise((resolve,reject)=>{
	resolve(value);
});
generatePromise(13)
  .finally(() => Promise.reject(10))
  .then(data=>console.log(data))
  .catch(data=>console.log(data))

I see however, that DefinitelyTyped definition for promise.prototype.finally is not going in the same direction as you proposed

    interface Promise<T> {
        finally<U>(onFinally?: () => U | PromiseLike<U>): Promise<T>;
    }

I would like to have a confirmation first before making the change.
@Andy-MS I see that at some point you contributed to the file containing the definitions for promise.prototype.finally , could you please give us a recommendation on which definition to adopt

Thanks!

Copy link

@ghost ghost Jan 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think void is best. Accepting U for an arbitrary value of U just means accepting anything.
(I didn't contribute that definition; I just moved the file.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Andy-MS @Kovensky
PR updated

* @param onfinally The callback to execute when the Promise is settled (fulfilled or rejected).
* @returns A Promise for the completion of the callback.
*/
finally(onfinally?: (() => void) | undefined | null): Promise<T>
Copy link

@Jessidhia Jessidhia Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a return type of this be appropriate here, or better to keep Promise<T>?

It doesn't return the receiver itself; but the returned value can have the same type as the receiver...... if the receiver implements [Symbol.species]. This is probably out of scope of what the type system can do...

@mhegazy mhegazy merged commit b36d614 into microsoft:master Jan 4, 2018
mhegazy added a commit to microsoft/TypeScript-Handbook that referenced this pull request Jan 4, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants