domenic / promises-unwrapping

The ES6 promises spec, as per September 2013 TC39 meeting
1.23k stars 94 forks source link

Promise.prototype.finally #18

Closed domenic closed 10 years ago

domenic commented 10 years ago

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:


Use cases collected so far:

erights commented 10 years ago

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 commented 10 years ago

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

erights commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

jakearchibald commented 10 years ago

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

kriskowal commented 10 years ago

@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 commented 9 years ago

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

stefanpenner commented 9 years ago

@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 commented 9 years ago

oops i misread @rauschma you are correct!

domenic commented 9 years ago

Fixed the OP

stefanpenner commented 9 years ago

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

rauschma commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

@matthew-andrews :+1:

theefer commented 9 years ago

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 commented 9 years ago

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

getify commented 9 years ago

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

benjamingr commented 9 years ago

Why?

stefanpenner commented 9 years ago

@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 commented 9 years ago

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

stefanpenner commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 9 years ago

(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.