Skip to content

Conversation

@hwillson
Copy link
Contributor

Adds Promise.prototype.finally support to Meteor's promise package. Let me know if I missed anything. Thanks!

Fixes #9639.

Adds `Promise.prototype.finally` support to Meteor's
`promise` package.

Fixes meteor#9639.
@benjamn
Copy link
Contributor

benjamn commented Feb 12, 2018

Looking at https://github.com/zloirock/core-js/blob/master/modules/es7.promise.finally.js for comparison, there's some extra logic to allow Promise subclasses to override the static Promise.resolve method. Essentially, instead of calling exports.Promise.resolve(f()), it would be better to call this.constructor.resolve(f()), since this.constructor might be a subclass that inherits from Promise.

The core-js implementation also checks if this.constructor[Symbol.species] is defined, just in case that should be used instead of this.constructor. Here's an explanation of why Symbol.species exists.

However, it strikes me that this is a lot of pedantry/pageantry just to call Promise.resolve, and there may be a way to rewrite this code so that it doesn't need to call Promise.resolve explicitly. That would probably be a worthwhile PR to core-js, too, if it works.

I'll push a commit in a few minutes if I can figure this out.

@hwillson
Copy link
Contributor Author

That would be awesome @benjamn! I was following the promise/lib/finally approach, but the core-js implementation does seem safer. Let me know how far you get and if there's anything I can dive back into. Thanks!

Ben Newman and others added 2 commits February 12, 2018 12:50
@benjamn benjamn merged commit 8b95173 into meteor:devel Feb 13, 2018
@Floriferous
Copy link
Contributor

Does this mean it is only supported on meteor server side? I'm getting this error on MS Edge:Object doesn't support property or method 'finally'.

Even when using babel, I have to add a a polyfill for this to work everywhere?

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.

3 participants