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

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

Promise.prototype.finally #18

domenic opened this issue Sep 3, 2013 · 27 comments

Comments

@domenic
Copy link
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
Copy link
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
Copy link
Owner Author

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
Copy link
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//issues/18#issuecomment-23708973
.

@annevk
Copy link
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
Copy link
Owner Author

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
Copy link

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
Copy link
Owner Author

domenic commented Sep 4, 2013

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

@jakearchibald
Copy link

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

@kriskowal
Copy link

@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
Copy link

rauschma commented Oct 3, 2014

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

@stefanpenner
Copy link

@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
Copy link

oops i misread @rauschma you are correct!

@domenic
Copy link
Owner Author

domenic commented Oct 3, 2014

Fixed the OP

@stefanpenner
Copy link

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

@rauschma
Copy link

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
Copy link

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
Copy link

@matthew-andrews 👍

@theefer
Copy link

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
Copy link
Collaborator

annevk commented Nov 18, 2014

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

@getify
Copy link

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
Copy link

Why?

@stefanpenner
Copy link

@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
Copy link

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

@stefanpenner
Copy link

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
Copy link
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
Copy link

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
Copy link

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
Projects
None yet
Development

No branches or pull requests