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

Promise.prototype.finally [fixes #232] #233

Merged
merged 1 commit into from
Dec 19, 2017
Merged

Conversation

stefanpenner
Copy link
Owner

No description provided.

@stefanpenner
Copy link
Owner Author

@ljharb should I wait until stage 3?

@ljharb
Copy link
Contributor

ljharb commented Mar 11, 2017

Officially yes, but at this point I do not anticipate changes prior to stage 3 that would affect this PR.

Take that with the appropriate grains of salt :-)

@eLarocque
Copy link

Hey guys.
Wondering what the update is on this since the finally promise is in stage 3.
https://github.com/tc39/proposals/blob/master/README.md

There a chance we will see an merge in near future?
Thanks

Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

So far this looks great; two comments, and then there's actually a difference with Promise.resolve in the spec and here, which is that this one returns the promise unchanged when it's a base Promise and finally's does not - that might end up changing tho.

es6-promise.d.ts Outdated
*
* @param onSettled called when/if "promise" settles
*/
finally <U> (onSettled?: (callback: any) => U | Thenable<U>): Promise<U>;
Copy link
Contributor

Choose a reason for hiding this comment

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

onFinally is the name in the spec

*/
finally(callback) {
let promise = this;
let constructor = promise.constructor;
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to use Symbol.species to look up the constructor

Copy link
Owner Author

Choose a reason for hiding this comment

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

We don't currently support species stuff here.

@l1bbcsg
Copy link

l1bbcsg commented Dec 19, 2017

Any updates on this? finally has hit stage 3 several moths ago now.

Also I'd like to remind you that just like catch this creates problems in browsers not compliant with ES5 allowing reserved words to be used as property names.
Assuming build process handles that and will quote it just like 'catch', it's worth noting in the readme at least.

@stefanpenner
Copy link
Owner Author

Any updates on this? finally has hit stage 3 several moths ago now.

Thanks for the reminder! I have also been eager to land this. Taking a look now.

Also I'd like to remind you that just like catch this creates problems in browsers not compliant with ES5 allowing reserved words to be used as property names.
Assuming build process handles that and will quote it just like 'catch', it's worth noting in the readme at least.

@stefanpenner
Copy link
Owner Author

@l1bbcsg updated, waiting for CI to complete

Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Seems legit; modulo Symbol.species stuff

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

Successfully merging this pull request may close these issues.

4 participants