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

Implement ES2018 Promise.prototype.finally in dojo/promise/Promise #362

merged 1 commit into from Sep 4, 2019

Implement ES2018 Promise.prototype.finally in dojo/promise/Promise #362

merged 1 commit into from Sep 4, 2019


Copy link

@RoyTinker RoyTinker commented Aug 31, 2019

Supersedes #352

Implementing ES2018's Promise.prototype.finally in dojo/promise/Promise instead of dojo/Deferred because:

  • Implementing in dojo/Deferred leaves an API hole -- we have an abstract "finally" method in dojo/promise/Promise that is left without an implementation in Promise instances built in dojo/_base/Deferred. So new baseDeferred().promise.finally(function(){}) would throw the "abstract" TypeError. Implementing in dojo/promise/Promise allows all Deferreds that use this Promise class to inherit finally in their promise objects -- dojo/Deferred, dojo/_base/Deferred, and even any (hypothetical?) custom Deferred classes built by users.
  • finally can be implemented directly in terms of then with reduced complexity over the previous implementation - at runtime, computationally, and in the source.

Includes comprehensive, passing unit tests in tests/unit/promise/Promise.js.

dylans approved these changes Sep 4, 2019
Copy link

@dylans dylans commented Sep 4, 2019

Landing in master... cannot backport as it's a change in functionality.

@dylans dylans merged commit 6da2db8 into dojo:master Sep 4, 2019
1 check passed
@dylans dylans added this to the 1.16.0 milestone Sep 4, 2019
Copy link
Contributor Author

@RoyTinker RoyTinker commented Sep 4, 2019

Thanks @dylans

@RoyTinker RoyTinker deleted the promise-finally branch Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants