feat($q): finally callback is called with result arguments

[shahata]

before this commit, finally callback was called with no arguments and couldn't know whether the promise was resolved or rejected and why.

Closes #9246

[dragosrususv]

@shahata : as usual, thank you very much!

[dragosrususv]

@shahata :

callbackOutput = callback(isResolved ? value : undefined, isResolved ? undefined : value);

What this basically does is to call the FINALLY method with different set of parameters:
Examples:

// (Success case)
.finally(a, b) {
  console.log(a, b); // OUTPUT: {VALUE}, undefined
}
// (Error case)
.finally(a, b) {
  console.log(a, b); // OUTPUT: undefined, {VALUE}
}

What I was suggesting is to call FINALLY with the same types of parameters (Boolean, Mixed):
Examples:

// (Success case)
.finally(a, b) {
  console.log(a, b); // OUTPUT: true, {VALUE}
}
//(Error case)
.finally(a, b) {
  console.log(a, b); // OUTPUT: false, {VALUE}
}

The code should look like:

callbackOutput = callback(isResolved, value);

Basically publish the API available inside AngularJS externally - give user the freedom to do whatever he/she wants with that API. What do you think?

[shahata]

Can go either way, let's see what ppl think and if this is even going to get merged...

[domenic]

This is a very bad idea. .finally is supposed to parallel the keyword finally, as in try/catch/finally. Since finally does not receive any args, neither should .finally.

I can guarantee that if we standardize Promise.prototype.finally in TC39 (which I hope to do), it will take no arguments.

[dragosrususv]

@domenic : as per comments in kriskowal/q#589, please stop spamming all github pages you find. I respect your opinion, but we seem to have very different aims: for you it's ok if a developer does it "in his own harden" while for me it's important to have a stable library that provides a good API. If you have any more subjective opinions about this, please use issue 589 from Q project.

Reminder here for people who do not have the patience of reading all: Domenic is proposing to use:

   .then(resolveCb, rejectCb)
   .catch(function() {})
   .then(function (response) {
      // response is the response either from "resolve()" or "reject()"
    });

    // INSTEAD OF

    .then(resolveCb, rejectCb)
    .finally(function (response) {
       // response is the response either from "resolve()" or "reject()"   
     });

His proposal seems more like a work-around than a solution and totally undocumented.

[domenic]

The second version doesn't even make any sense, since reject() gives Error instances (it is analogous to throwing) and resolve gives real values (analogous to returning).

[dragosrususv]

:) @domenic : thank you for posting more comments in Q issue.
Yes it makes sense because both callbacks receive first parameter with some Mixed type of data. That Mixed type of that can be the parameter of the finally. Period. People who need catch, will use catch.

[domenic]

(I don't take instructions from you on where to converse.) That is false; reject callbacks are always called with Errors, at least in well-behaved libraries. Doing anything else would be like doing throw 5.

[briancavalier]

I'd like to propose an option: name this operation promise.always().

I think this would be a good solution for at least a couple reasons:

1. There is already a de facto behavior for an operation named promise.finally() in many of the most widely used promise implementations, including when.js, Q, bluebird, RSVP, and others.
2. Synchronous finally does not receive any arguments. Maintaining a parallel between promise error handling and synchronous error handling is too important to ignore here, imho. It allows reasoning about them in similar ways.

So, a simple solution seems like it would be to pick a nice, but different name, like always.

[spion]

@dragosrususv - @domenic knows the pains of standardizing promises to have a common interface across all libraries and in ES6. Changing the interfaces of common methods like this could have disastrous consequences in the future - like being unable to standardize finally in ES7 thanks to a popular library like angular doing something different. Something similar to this has already happened with jQuery promises afaik (thanks to them ES6 promises are more complicated and less flexible than they could be).

@briancavalier 's proposal is the way to go here IMO - it fixes both issues

[dragosrususv]

@briancavalier , @spion - ".always()" is perfect. As a developer, the only requirement would be to write code in one place. Thank you kindly for a constructive feedback on this one.

If this proposal gets accepted in higher groups, do you think we could get some feedback here to the AngularJS sub-world to know how to proceed?

Thank you kindly in advance.

@domenic : I apologize for my insistent comments. I was not aware of the full context, as @spion described it above, but I still believe a solution should be in place (like the .always() or .anyway() or whatever name that might be).

[caitp]

I'm not too sure about the current design (unrelated to the rest of this discussion, which I don't care a whole lot about).

The "pass the fulfilled value and rejection reason" thing seems to enable you to write 2 branches of code, whereby you handle either a fulfillment or rejection --- and that doesn't seem useful or nice.

I would rather there was only one parameter, and you are meant not to care whether it's a rejection or fulfillment (otherwise you'd use proper fulfillment handlers or rejection handlers).

In short, I don't want people writing code like this:

promise.
  finally(function(goodValue, badValue) {
    if (badValue === (void) 0) {
      // deal with resolved value
    } else {
      // deal with rejection
    }
  });

Because this is messier than the api that is already provided

[briancavalier]

@caitp Personally, I agree with you about branching code being messy, and that using then and catch is less hazardous in general.

Unfortunately, I don't think a single-parameter version of always is necessarily "better" (as always, subjective). It would still require branching, but since JS allows throwing anything, it would be impossible to determine whether the single argument represents success or failure:

promise.always(function(x) {
    // There's no foolproof test to know if x represents success or failure
}

The only truly safe thing to do in that case is ignore x entirely, which leads back to the zero-arg version of finally.

[caitp]

I think if you're wanting to run the same code regardless of whether it's a success or failure, you probably don't care if it's a success or failure (just my 2c)

[shahata]

If you don't care if it is success or failure, you probably don't care about the result at all. Maybe we should be consistent with Q and drop this...

[caitp]

In the case of finally, I think we've established that you don't care about the value --- however in the case of always, I think it makes sense to get the fulfilled or rejected value, but not provide a mechanism for branching

[shahata]

how about having the fulfilled or reject value in the first param and a boolean indicating if the promise was resolved or rejected in the second param as was previously suggested? this way you are not forced to branch, but you also have the boolean if you really need it for something.

[dragosrususv]

@shahata , @caitp : For my own scope I do not need this information - the value as first parameter is sufficient. Still, maybe other developers would need the isResolved in future - I would vote for adding that (although I do agree with @caitp for the unnecessary IF's).

[caitp]

I'm not keen on it

[shahata]

okay, let's start without. another issue - regarding the semantics of this always... what do you think it should return? I think it should behave like then/catch and not like finally (reminder - finally can't change the resolved value, but Angular's implementation is a bit different then Q's, see #9291).

[caitp]

I'm not sure the return value should affect the resolution of the handler's promise, it should probably just resolve/reject to whatever the value is. I'd have to look at some other promise libraries to see what the general pattern of done/always is

[dragosrususv]

Presuming there will be a 1-to-1 relationship between a promise and an .always() method, I don't see any reason one would need to forward the returned value to the next always/then. As the name implies, "always" or "finally" - it's a one time show. IMHO.

The jQuery guys though seem to have done this as @shahata said: "Since deferred.always() returns the Deferred object, other methods of the Deferred object can be chained to this one, including additional .always() methods" ( http://api.jquery.com/deferred.always/ )

[caitp]

I think you'll find it more difficult to make it work as a "one time show" than it's really worth. That said, I will investigate strategies for always/done in other frameworks

[dragosrususv]

If it's easier to just extend the defer API with a method in the command-chain object, then that's just great. From standard perspective, this choice would be even better.

[caitp]

I don't really buy the premise that angular or jQuery's own deferred APIs will have that much of an impact on TC39's promise apis. We're already playing catchup with them, they certainly haven't gone down a path like jQuery.Deferred or angular $q yet.

[dragosrususv]

That's too bad. I believe that most popular frameworks have faced a lot of problems they had to resolve for their own communities - I guess they just opt to throw to garbage this experience. And most of the time this experience brings you the real scenarios and use-cases and not fictional ones.

[caitp]

that's not to say that promise libraries haven't been very involved in those discussions, especially if Domenic is maintaining Q

[shahata]

I've updated this PR with an initial implementation of always just so we have something to discuss and see if we want this thing.

[shahata]

1. an alternative implementation can be return this.then(callback, callback) but then the callback can control the reject/resolve chain, which is strange since the callback doesn't even know if this is a reject or resolve.
2. we could allow this callback to return a promise in order to delay the resolve/reject of the chained promise, but still not effect the final result, similar to what finally does.

[dragosrususv]

(1) - it would make no sense to add 2 callbacks, it becomes "then"
(2) - I might misunderstand the code above, but that seems to be different from jQuery's implementation where the return of an always call is pushed to the next one in the queue (.then,
.always, etc) - in this case the return value is ignored and the chain continues with last known value; but I presume that is what you intended to do.

[shahata]

so you are saying that you prefer the alternative implementation where:

always: function (callback) {
  return this.then(callback, callback);
});

I don't really care, but as I said, this would be a bit strange since the callback doesn't even know if this is a reject or resolve. I could do that just as easily, obviously, let's just try to agree on the implementation that makes sense.

[caitp]

it would be ideal to behave the way other promise implementations do, I guess (in other words, I'll investigate the behaviour of other popular libraries tomorrow and leave comments based on that, unless someone else beats me to it)

[shahata]

I'll have time to look into it tonight, so I guess you can do it too if you don't want to wait. From initial (and very superficial) look it doesn't seem like other promise implementations are doing something like this except for maybe jQuery.

[dragosrususv]

Yeah. Tried to search a bit but didn't find this "always" anywhere else.
Personally for me it works both ways. I was just saying that jQuery implemented "always()" in the same manner as "then" (stack-able via command-chain - even multiple always, as they say).

[dragosrususv]

This seems like it got on a closed rail - any news?
I doubt there are more known libraries that implement the all except jQuery and that has already been mentioned above.

[lgalfaso]

Given that Angular 2 is moving to ES6 Promise, then I would not add anything new to $q. I am going to close this as even if there is enough traction, this can be part of a third party library