promise finally function could send the success/fail response as parameter

[dragosrususv]

Type: feature
Reproduces on: any OS, any browser

Code:
https://github.com/angular/angular.js/blob/master/src/ng/q.js#L285
https://github.com/angular/angular.js/blob/master/src/ng/q.js#L460

Solution:
Just send the value as parameter in the callback. It could even have 2 params: success and value (as angularjs callbacks have).

Real use-case:
In our application all messages that are being queued and displayed to the user in a messages-directive are being sent by the server within each structured response (may it be success or error). Our problem is that we have to write something like:

     ajax.call($scope.shared.ajax.getXXX).then(
        function (response) {
          // ...
          msgService.notify(response);
        },
        function (response) {
          msgService.notify(response);
        }
      )

Instead of just

     ajax.call($scope.shared.ajax.getXXX).then(
        function (response) {
          // ...
        }
      ).finally(function(response) {
          msgService.notify(response); 
      });


     // OR JUST

     ajax.call($scope.shared.ajax.getXXX)
       .then(function (response) { // ... })
       .finally(msgService.notify);

CONTEXT:
I am aware that Kris's initial implementation of $q (https://github.com/kriskowal/q) does not have this, but I still believe it's a very good idea to implement. I can contact Kris directly and discuss with him the option to include this in base implementation as well.

[lgalfaso]

I think you are getting confused here, if you have a promise, then calling then creates a new promise that will get resolved with the value returned by the function used at the then. This is, to achieve what you want to do, then you have to

foo = $http(...);
foo.then(...);
foo.finally(...);

With this information, this issue is invalid

[dragosrususv]

@lgalfaso : I think you misunderstood the problem.

NOTE: In the code above I'm just linking the calls directly, without using a "foo" variable.

The problem is that the .finally() callback doesn't receive any parameter. My suggestion was for finally to receive whatever parameters would receive the 2 callbacks: error or success. Or maybe one more which says if the flow was the success or error one (as angular internal methods).

PLEASE REOPEN.

[lgalfaso]

@dragosrususv if you do

ajax.call($scope.shared.ajax.getXXX)
       .then(function (response) { /* stuff */ return response; })
       .finally(msgService.notify);

then finally will get what you want. By calling then you are creating a new promise and to this new promise is that you are calling finally to

[gkalpak]

@lgalfaso: The thing is that they need to do it from both success and error callbacks. If it was passed automatically, then their code would be DRYier by 1 line :)

[lgalfaso]

@gkalpak the fact that success and error are specific to $http and they work different does not change the fact that that is not how promises work

Just add another method to $q as

function sameSameAsThenButDifferent(callback, errback, progress back) {
  this.then(callback, errback, progress back);
  return this;
}

and use it

[gkalpak]

@lgalfaso: I wasn't referring to the success() and error() methods of $http, but to the callbacks passed to then() in order to be executed on success or on error (1st and 2nd arguments respectively). Sorry for the confusion.

What the OP is trying to do is perform some operation when a request completes (either successfully or with error) and this operation depends on the response itself. So, instead of having to perform the operation in two places or having to return the response from both the onSuccess and onError callbacks and create a new promise, they ask that the response be passed to the finally() callback as well (regardless if the request completed with success or with error).

This feature does not need to be specific to $http; it can apply to all promises.

---

Anyway, I am just trying to make sure the request is clear, so the decision (if it's going to be added or not) is made on the correct assumptions.

[lgalfaso]

@gkalpak if you have a promise p and call finally on this promise, then it cannot reference something else that is not this promise. i.e. it cannot reference the promise that this was generated from because

• there might be no other promise at all
• if a promise p is resolved to a value X then this is the value I expect to get

The fact that when you call then it creates a new promise will not change. Promise A+ works that way and so does ES6 promises

[gkalpak]

@lgalfaso: I don't think anyone wants to reference another promise.
If a promise p is resolved to a value x then the OP want to get that value x in the finally callback.
Right now the finally callback gets called without any arguments and it would be convenient if it was called with the value as an argument.

E.g.:

function myCallback(val) {
    console.log('Finally, I was called with argument:', val);
}

var deferred = $q.defer();
deferred.promise.finally(myCallback);
deferred.resolve('Hello, world !'); /* or */ deferred.reject('Hello, world !');

// With the current implementation, this will result in:
// Finally, I was called with argument: undefined

// With the feature requested by the OP, this will result in:
// Finally, I was called with argument: Hello, world !

[lgalfaso]

@gkalpak that is not what finally is for. The right way to do what you want is

function myCallback(val) {
    console.log('Finally, I was called with argument:', val);
}

var deferred = $q.defer();
deferred.promise.then(undefined, angular.identity).then(myCallback);
deferred.resolve('Hello, world !'); /* or */ deferred.reject('Hello, world !');

[gkalpak]

@lgalfaso:
It's not that I want to use that or anything (just trying to explain what the feature request is about).
Your code does not exactly does what OP wants, because there needs to be other code executed in each callback.

That said, I suppose this is a "won't fix" (and having to add the same line of code twice isn't so much of a nuisance anyway). Let's leave it at that :)

[dragosrususv]

/cc @caitp : sorry to disturb - it seems @lgalfaso doesn't get what I'm trying to say. I really think this is a cool feature and @gkalpak perfectly understood the problem. What do you think?

[dragosrususv]

@lgalfaso : please mind that on error case scenario you don't have a 2nd callback in last .then (".then(myCallback);") - this means that if the promise is rejected, your code will just not do anything on this flow.

The only solution is via finally which should receive the last returned value from either success or failure callbacks.

Again PLEASE reopen the item and leave others to share their opinion on it.

[caitp]

I don't really have a problem with making it work if someone wants to send a PR. the finally implementation is sort of confusing and hard to read, and might not be too high on the list of priorities right now. If you submit a fix for this I'm happy to review it.

[dragosrususv]

@caitp : I was going to dig into AngularJS in this weekend but @shahata already created it.
Thank you kindly @shahata !

I truely believe that a framework needs to provide style to programmers - to be able to write sexy code.

[dragosrususv]

@caitp : seems @shahata already did a PR for this - for you to reopen this issue and review.
Keeping my fingers crossed.

I added a small comment to @shahata's PR, but it's not a blocker I guess - would just make more sense if we think to the base programming languages where parameters are always the same type.

[domenic]

This is a very bad idea. finally is meant to parallel try/catch/finally.

[dragosrususv]

@domenic : can you stop spamming opened issues in multiple projects and provide solutions?
In the context of the promise finally/fin is the method regardless of the ".then"/".catch" predecessors.

I'm not keen on having the "finally" keyword used, I'm keen on having this option of executing a shared piece of code in resolved or rejected mode and stop copy-pasting between (even a 1-liner). Do you have any suggestions here?

[domenic]

Yes. This will work:

doThing().catch(() => {}).then(doOtherThing);

it is equivalent to

try {
  doThing();
} catch (e) { }
doOtherThing();

[dragosrususv]

Again, please stop spamming this issue as you posted this kriskowal/q#589 as well. I have the feeling that you don't understand the problem (see comments above), but I might be wrong of course.

[domenic]

You may not be aware, but I am a maintainer of Q, and so it is my responsibility to respond there.

[dragosrususv]

:) @domenic : I am aware ("Collaborator"). But from your comments you seem to have fall in love with your code and you find it hard to think outside/without it. I respect that, but the aim here is to reduce the code footprint. As you mentioned in a parallel threat, the ".catch().then()" might be a solution, but again, I don't need a "catch." - I'm forced to add it to achieve this. And the global point is again to reduce the code footprint.

[caitp]

lets try keep it respectful and try not to refer to comments from different people as "spam".

I'm not totally sure how this feature would affect various apps or their codebases, so the input is definitely valuable

[dragosrususv]

@caitp : the only reason I labeled @domenic 's comments as spam was because their were repetitive (3: 2 github issue pages, 1 github PR) - and in all he just posted the same content. I just had to reply everywhere, even if there was a long discussion made on reported Q issue. This usually falls into the spam category.

As for @domenic 's solution, as previously stated, seems like an workaround. Other people have added their constructive feedback here: #9287 (comment) - seems like a good place to start (another callback: ".always()").

[dragosrususv]

< ping > @caitp

[lgalfaso]

Based on kriskowal/q#589 (comment) I think that we should not add an always call

[dragosrususv]

@lgalfaso : please provide a non throw-try-catch-workaround solution though - I'm open.

As per other #9287 (comment) this can and should be done - one way or another. The only reason Domenic is pushing back is because he needs to wrap up the final spec and he cannot add something new (I don't understand that but he must have good reasons for it) on the last round. But this can be again base for a future spec.

[lgalfaso]

@dragosrususv I read the entire thread at kriskowal/q#589 and the only reference to a spec was on the change to make finally get the output parameters. Even in that case, Domenic did was was very careful and never said that he was not accepting your proposal because he was in a rush to finish the spec on ES6 Promises, and gave multiple reasons on why he thinks it is a bad idea.

Now, back to always, I find this addition unnatural. As many times before, I can be wrong and this would be a great addition, this is why on parts that angular reimplements abstractions that existed before and are stable (as it is the case of Promise), then the responsible thing to do is to look what other libraries do. In the case of Q, you opened an issue (that is the right thing) and it was rejected as Kris Kowal thinks that it would add confusion. I also went thru bluebird and was not able to find such a call. If the mosts commonly used libraries are reluctant to adding some functionality, then it is time to go one step back and reason why it is that they think it is a bad idea.

In my specific case (and for sure I am not talking on behalf of Q nor bluebird) the always method is a bad idea because it involves information erasure. The function that you would pass has no way to actually know if it is handling an error or a success (Note: the option to pass two arguments is also strange as, again, very unnatural)

Angular core is not the place to provide every solution to every problem. Third party libraries can decorate $q and provide this functionality, you can do so yourself or just add this to your own project. This is the path I recommend you to follow