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 #18

Closed
domenic opened this Issue Sep 3, 2013 · 27 comments

Comments

Projects
None yet
@domenic
Owner

domenic commented Sep 3, 2013

I am not sure why this wasn't in the original DOM spec, nor in @erights's concurrency strawman, but it's damn useful and not trivial to implement yourself.

Promise.prototype.finally = function (callback) {
    return this.then(
        value => this.constructor.resolve(callback()).then(() => value),
        reason => this.constructor.resolve(callback()).then(() => { throw reason; })
    );
};

Notably, like JS finally clause:

  • no value or reason is passed in to the finally code (i.e. to callback).
  • Does not affect the result, i.e. the resulting promise is still fulfilled with value or rejected with reason, just like the original one, unless...
  • If the finally code blows up (i.e. if callback throws, or returns a rejected promise), it propagates that failure onward, instead of the original success or failure.

Use cases collected so far:

  • Clean up event listeners, regardless of whether the promise succeeds or fails 1.
  • Hiding the spinner after a network request succeeds or fails 2.
  • Test teardown 3.
@erights

This comment has been minimized.

Show comment
Hide comment
@erights

erights Sep 3, 2013

Collaborator

Absent from concurrency strawman, due only to oversight. I don't have experience with it. Sounds plausible. Can you point at some examples that illustrate well how it is useful?

Collaborator

erights commented Sep 3, 2013

Absent from concurrency strawman, due only to oversight. I don't have experience with it. Sounds plausible. Can you point at some examples that illustrate well how it is useful?

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Sep 3, 2013

Owner

Here is one quick example, where we use it to clean up event listeners: https://gist.github.com/domenic/6419679

Owner

domenic commented Sep 3, 2013

Here is one quick example, where we use it to clean up event listeners: https://gist.github.com/domenic/6419679

@erights

This comment has been minimized.

Show comment
Hide comment
@erights

erights Sep 3, 2013

Collaborator

Nice.
On Sep 3, 2013 5:33 AM, "Domenic Denicola" notifications@github.com wrote:

Here is one quick example, where we use it to clean up event listeners:
https://gist.github.com/domenic/6419679


Reply to this email directly or view it on GitHubhttps://github.com/domenic/promises-unwrapping/issues/18#issuecomment-23708973
.

Collaborator

erights commented Sep 3, 2013

Nice.
On Sep 3, 2013 5:33 AM, "Domenic Denicola" notifications@github.com wrote:

Here is one quick example, where we use it to clean up event listeners:
https://gist.github.com/domenic/6419679


Reply to this email directly or view it on GitHubhttps://github.com/domenic/promises-unwrapping/issues/18#issuecomment-23708973
.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Sep 3, 2013

Collaborator

If we think we can get consensus on this, seems fine, otherwise, please lets not add new features that will delay the whole thing longer.

Collaborator

annevk commented Sep 3, 2013

If we think we can get consensus on this, seems fine, otherwise, please lets not add new features that will delay the whole thing longer.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Sep 3, 2013

Owner

Thanks for reining us in, @annevk :). My new position then is to default to not speccing it unless there's a sudden upswell of support due to your email this morning. We can always make it a high-priority addition later.

I'll close this in a day or so if said upswell does not appear.

Owner

domenic commented Sep 3, 2013

Thanks for reining us in, @annevk :). My new position then is to default to not speccing it unless there's a sudden upswell of support due to your email this morning. We can always make it a high-priority addition later.

I'll close this in a day or so if said upswell does not appear.

@mhofman

This comment has been minimized.

Show comment
Hide comment
@mhofman

mhofman Sep 3, 2013

I'd like to voice support for finally. I've had to implement it myself with the current version of DOM Promises and having it built-in would allow us to remove that piece of code which is basically there to restore a functionality that exists in synchronous operations.

mhofman commented Sep 3, 2013

I'd like to voice support for finally. I've had to implement it myself with the current version of DOM Promises and having it built-in would allow us to remove that piece of code which is basically there to restore a functionality that exists in synchronous operations.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Sep 4, 2013

Owner

Closing for now. I would like this a lot but getting something done is more important.

Owner

domenic commented Sep 4, 2013

Closing for now. I would like this a lot but getting something done is more important.

@jakearchibald

This comment has been minimized.

Show comment
Hide comment
@jakearchibald

jakearchibald Nov 4, 2013

Another use-case for this is hiding a spinner after a network request succeeds or fails https://gist.github.com/jakearchibald/785f79b0dea5bfe0c448

Another use-case for this is hiding a spinner after a network request succeeds or fails https://gist.github.com/jakearchibald/785f79b0dea5bfe0c448

@kriskowal

This comment has been minimized.

Show comment
Hide comment
@kriskowal

kriskowal Nov 4, 2013

@jakearchibald Thanks. I often use finally for test teardown…

var HTTP = require("q-io/http");
var server = HTTP.Server(app);
return server.listen(0)
.then(function () {
    // run test
})
.finally(server.stop)

@jakearchibald Thanks. I often use finally for test teardown…

var HTTP = require("q-io/http");
var server = HTTP.Server(app);
return server.listen(0)
.then(function () {
    // run test
})
.finally(server.stop)
@rauschma

This comment has been minimized.

Show comment
Hide comment
@rauschma

rauschma Oct 3, 2014

Minor correction: Promise.prototype.finally itself can’t be an arrow function – you are using this inside it.

rauschma commented Oct 3, 2014

Minor correction: Promise.prototype.finally itself can’t be an arrow function – you are using this inside it.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Oct 3, 2014

@rauschma no it is correct, as it retains the this from the containing closure.

  // es3+ version
  'finally': function(callback) {
    var constructor = this.constructor;

    return this.then(function(value) {
      return constructor.resolve(callback()).then(function(){
        return value;
      });
    }, function(reason) {
      return constructor.resolve(callback()).then(function(){
        throw reason;
      });
    });
  }

@rauschma no it is correct, as it retains the this from the containing closure.

  // es3+ version
  'finally': function(callback) {
    var constructor = this.constructor;

    return this.then(function(value) {
      return constructor.resolve(callback()).then(function(){
        return value;
      });
    }, function(reason) {
      return constructor.resolve(callback()).then(function(){
        throw reason;
      });
    });
  }
@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Oct 3, 2014

oops i misread @rauschma you are correct!

oops i misread @rauschma you are correct!

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Oct 3, 2014

Owner

Fixed the OP

Owner

domenic commented Oct 3, 2014

Fixed the OP

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Oct 3, 2014

@domenic i suppose class syntax will make that nice again :)

@domenic i suppose class syntax will make that nice again :)

@rauschma

This comment has been minimized.

Show comment
Hide comment
@rauschma

rauschma Oct 3, 2014

@stefanpenner Object.assign() is a possibility. For a single method, I’m not yet sure whether I prefer it or not (it’s also verbose, but differently).

Object.assign(Promise.prototype, {
    finally(callback) {
        ...
    }
});

Versus:

Promise.prototype.finally = function (callback) {
    ...
};

rauschma commented Oct 3, 2014

@stefanpenner Object.assign() is a possibility. For a single method, I’m not yet sure whether I prefer it or not (it’s also verbose, but differently).

Object.assign(Promise.prototype, {
    finally(callback) {
        ...
    }
});

Versus:

Promise.prototype.finally = function (callback) {
    ...
};
@matthew-andrews

This comment has been minimized.

Show comment
Hide comment
@matthew-andrews

matthew-andrews Oct 11, 2014

Quick flyby - I've packaged up @stefanpenner's implementation into something resembling a polyfill (and optimistically described it as one) for browser/node:-
https://github.com/matthew-andrews/Promise.prototype.finally

Quick flyby - I've packaged up @stefanpenner's implementation into something resembling a polyfill (and optimistically described it as one) for browser/node:-
https://github.com/matthew-andrews/Promise.prototype.finally

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@theefer

This comment has been minimized.

Show comment
Hide comment
@theefer

theefer Nov 18, 2014

I was surprised to see finally didn't make it into the spec (and hence in Traceur). Is there anything we can do to make it happen in a future spec/in browsers? Is it recommended to use polyfills such as @matthew-andrews' in the meantime?

theefer commented Nov 18, 2014

I was surprised to see finally didn't make it into the spec (and hence in Traceur). Is there anything we can do to make it happen in a future spec/in browsers? Is it recommended to use polyfills such as @matthew-andrews' in the meantime?

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 18, 2014

Collaborator

@theefer follow the steps at the end of https://github.com/tc39/ecma262 to make it happen.

Collaborator

annevk commented Nov 18, 2014

@theefer follow the steps at the end of https://github.com/tc39/ecma262 to make it happen.

@getify

This comment has been minimized.

Show comment
Hide comment
@getify

getify Nov 18, 2014

@annevk is there a process to follow to make it NOT happen? :) I think finally is a really bad idea, personally.

getify commented Nov 18, 2014

@annevk is there a process to follow to make it NOT happen? :) I think finally is a really bad idea, personally.

@benjamingr

This comment has been minimized.

Show comment
Hide comment

Why?

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Nov 18, 2014

@getify easy, choose to not use it :trollface:

More seriously, I see myself as a strong advocate for finally so I would like to understand your concerns and make sure I am not overlooking something.

@getify easy, choose to not use it :trollface:

More seriously, I see myself as a strong advocate for finally so I would like to understand your concerns and make sure I am not overlooking something.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Nov 18, 2014

From my perspective: people ask for it all the time in StackOverflow. It's also pretty useful for cleanup, logging etc.

From my perspective: people ask for it all the time in StackOverflow. It's also pretty useful for cleanup, logging etc.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Nov 18, 2014

people ask for it all the time

doesn't mean it should exist.

It's also pretty useful for cleanup

this is absolutely the scenario i believe requires it. As it is non-trivial and error prone to try an implement finally behavior without finally.

people ask for it all the time

doesn't mean it should exist.

It's also pretty useful for cleanup

this is absolutely the scenario i believe requires it. As it is non-trivial and error prone to try an implement finally behavior without finally.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 18, 2014

Collaborator

@getify argue against it on es-discuss.

However, given that synchronous JavaScript already has try/catch/finally, it seems asynchronous JavaScript should offer the same.

Collaborator

annevk commented Nov 18, 2014

@getify argue against it on es-discuss.

However, given that synchronous JavaScript already has try/catch/finally, it seems asynchronous JavaScript should offer the same.

@getify

This comment has been minimized.

Show comment
Hide comment
@getify

getify Nov 18, 2014

I didn't mean to hijack this already-closed thread to have such a debate. My troll to @annevk was a gentle nudge just to point out that finally is not necessarily unanimously thought of as "a great idea to include with promises".

argue against it on es-discuss.

I know. I thought it would obviously be seen as a lighthearted troll. Hence the ":)".


given that synchronous JavaScript already has try/catch/finally, it seems asynchronous JavaScript should offer the same.

Actually, with all due respect, this is one of the poorer arguments I've seen in favor of Promise#finally.

In try..catch..finally, there's a definitive end to the execution context, which is the predictable trigger for when finally will happen.

With an asynchronous operation, there isn't nearly as clear of an answer when finally should actually occur. For example:

  1. The OP snippet indicates this trigger point is "whenever there is a resolution, either fulfillment or rejection".

    That's a fine answer, as long as there's never such a thing as cancel, in which case it becomes dubious whether canceling a promise should or should not trigger the finally handler. I can make strong arguments on both sides of that, and looking at prior art among existing libraries, there is no definitive "right answer".

  2. But what about a promise that never gets resolved either way? Other posts here (and elsewhere) imply that finally would be interpreted as "cleanup" upon GC (either during the page life time or at the end of the program). Of course, being able to trigger behavior on GC is a really, really bad idea, as it makes GC observable. Others have suggest promises should have a built-in "timeout" concept.

There's lots more detail here. I won't vomit it all here for some sake of brevity. The point is, it's much more complicated than "well, try..catch..finally is cool".

getify commented Nov 18, 2014

I didn't mean to hijack this already-closed thread to have such a debate. My troll to @annevk was a gentle nudge just to point out that finally is not necessarily unanimously thought of as "a great idea to include with promises".

argue against it on es-discuss.

I know. I thought it would obviously be seen as a lighthearted troll. Hence the ":)".


given that synchronous JavaScript already has try/catch/finally, it seems asynchronous JavaScript should offer the same.

Actually, with all due respect, this is one of the poorer arguments I've seen in favor of Promise#finally.

In try..catch..finally, there's a definitive end to the execution context, which is the predictable trigger for when finally will happen.

With an asynchronous operation, there isn't nearly as clear of an answer when finally should actually occur. For example:

  1. The OP snippet indicates this trigger point is "whenever there is a resolution, either fulfillment or rejection".

    That's a fine answer, as long as there's never such a thing as cancel, in which case it becomes dubious whether canceling a promise should or should not trigger the finally handler. I can make strong arguments on both sides of that, and looking at prior art among existing libraries, there is no definitive "right answer".

  2. But what about a promise that never gets resolved either way? Other posts here (and elsewhere) imply that finally would be interpreted as "cleanup" upon GC (either during the page life time or at the end of the program). Of course, being able to trigger behavior on GC is a really, really bad idea, as it makes GC observable. Others have suggest promises should have a built-in "timeout" concept.

There's lots more detail here. I won't vomit it all here for some sake of brevity. The point is, it's much more complicated than "well, try..catch..finally is cool".

@getify

This comment has been minimized.

Show comment
Hide comment
@getify

getify Nov 18, 2014

(perhaps we should now move the debate elsewhere?)

I don't have a particular problem with the concept of "cleanup" via finally. In fact, I've written about how I think the technique can be quite useful. I think we have to get a much, much crisper idea of the "when" behind finally, because it's far too murky thus far.

But my problem with the current formulation of finally (the soft proposals based on prior art, essentially) is symmetric with my much deeper concerns over cancel proposals. These sorts of functionalities can definitely be "useful", but I don't believe they belong directly on promise instances.

I believe an individual promise being well-defined as a singular immutable future-value container is the proper design. If you want to clean up after a single promise, you just do similar as the OP did above, in which case a finally is just thin API sugar. Any promise library can easily do that. It's not clear that we need native promises to do them.

But when you start chaining promises together, approximating async flow control, I believe you've stepped up into another layer of abstraction. The problem is that ES6 Promise is only giving us the individual promise abstraction, and when you start trying to reason about the entire chain as a single entity, there's lots of "limitations" that arise.

I believe finally and cancel and other such capabilities are reasonable only at this higher level of abstraction, where there's a single entity to refer to the entire chain. And I happen to call that abstraction a "sequence".

If we want to reason about cancel and finally use cases, I believe we should be discussing the bigger issue of putting a label and handle on the thing-that-is-currently-just-a-hidden-linked-promises-chain, and then putting those functionalities there.

BTW, I believe this is all quite similar to the reasoning that's driving ES7 proposals around async / await as an abstraction on top of the marriage between generator and promise.

getify commented Nov 18, 2014

(perhaps we should now move the debate elsewhere?)

I don't have a particular problem with the concept of "cleanup" via finally. In fact, I've written about how I think the technique can be quite useful. I think we have to get a much, much crisper idea of the "when" behind finally, because it's far too murky thus far.

But my problem with the current formulation of finally (the soft proposals based on prior art, essentially) is symmetric with my much deeper concerns over cancel proposals. These sorts of functionalities can definitely be "useful", but I don't believe they belong directly on promise instances.

I believe an individual promise being well-defined as a singular immutable future-value container is the proper design. If you want to clean up after a single promise, you just do similar as the OP did above, in which case a finally is just thin API sugar. Any promise library can easily do that. It's not clear that we need native promises to do them.

But when you start chaining promises together, approximating async flow control, I believe you've stepped up into another layer of abstraction. The problem is that ES6 Promise is only giving us the individual promise abstraction, and when you start trying to reason about the entire chain as a single entity, there's lots of "limitations" that arise.

I believe finally and cancel and other such capabilities are reasonable only at this higher level of abstraction, where there's a single entity to refer to the entire chain. And I happen to call that abstraction a "sequence".

If we want to reason about cancel and finally use cases, I believe we should be discussing the bigger issue of putting a label and handle on the thing-that-is-currently-just-a-hidden-linked-promises-chain, and then putting those functionalities there.

BTW, I believe this is all quite similar to the reasoning that's driving ES7 proposals around async / await as an abstraction on top of the marriage between generator and promise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment