feat(finally): finally should receive as parameter the state of the promise

[dragosrususv]

As per angular/angular.js#9246 , it would be nice and more developer friendly to be able to receive the state of the promise inside the finally call.

My suggestion is for .fin() to receive 2 parameters:

1. a boolean -- if the promise was resolved (TRUE) or rejected (FALSE)
2. a mixed -- value sent to .resolve() or .reject()

@kriskowal : what do you think?

I'll post here the real use-case again.
We have a SPA and we are using AngularJS as a framework. I know Q service is a bit different than AngularJS $q, but the logic seems to be very similar.

Our use-case is when all the ajax requests (error or success) may contain a set of messages - those messages should be automatically pushed to the messages component and be shown.
The problem is that we have to write:

.then(
  function (data) {
    msgService.notify(data);
  },
  function (data) {
    msgService.notify(data);
  }
);


My suggestion was to have something like:
.then(...)
.finally(function (isResolved, data) {
    msgService.notify(data);
});

We can even adapt our notify and just put ".finally(msgService.notify)".

[domenic]

.finally is supposed to parallel the keyword finally, as in try/catch/finally. Since finally does not receive any args, neither will .finally.

Your above example seems like you want to notify the message service no matter if there are errors or not. In synchronous code, we already have a pattern for that: try { ... } catch (e) {}. So I would suggest

doThing().catch(() => {}).then(() => msgService.notify(data));

[dragosrususv]

@domenic : I am aware of current functionality. This is a feature proposal, not a bug.

As for your solution: how does that catch work in a success scenario?

[domenic]

Yes. I am telling you why the current functionality is designed that way and will not be changing.

The code I wrote is exactly equivalent to the code you wrote, so it will work in the same way.

[dragosrususv]

I don't get it.
You just have a catch in between. Your last "then" from above will only cover the SUCCESS/RESOLVED case scenario. In case that promise will be rejected, the msgService will not be called. Please tell me I'm wrong. And please provide full code samples and not logical arrows.

[domenic]

That's false ... because I have caught the error, the subsequent .then will execute. That is how promises work... Try it in your browser console:

Promise.reject(new Error("boo!"))
  .catch(function () { })
  .then(function () { console.log("this is reached"); });

[dragosrususv]

Ok... and if the promise is resolved, am I forced to add that catch?

[domenic]

How would you know ahead of time whether the promise is fulfilled or rejected?

[domenic]

Regardless, if the promise is fulfilled, the .catch will have no effect. (Just like the try { doStuff() } catch (e) { } has no effect if doStuff() does not throw an error.)

[dragosrususv]

I'm not 100% sure of the differences between @kriskowal 's Q and AngularJS $q so this might be the root of this issue.

In your code you added just 1 method in the last ".then" - there is no 2nd function added there. Do you confirm that method is called regardless of resolve/reject?

[domenic]

Yes, because the promise returned by .catch(() => {}) is always fulfilled (the entire purpose of .catch(() => {}) is to convert rejections into fulfillments, just like try { } catch (e) { } converts thrown errors into normal flow).

All of this applies to $q as well, by the way. I encourage you to just try this in your browser, whether with Q, $q, or window.Promise.

[dragosrususv]

Ok. So what you are basically saying is that I MUST have a "catch", and then a ".then" and stop using the "finally". I'll test and get back.

Still, I am being forced to add a "catch" that I don't need as an empty function - the aim here is to reduce the lines of written code, not to add more...

[domenic]

What I am saying, is just write your code as if it was synchronous, then convert any catchs into .catchs, and finallys into .finallys :). The amount of code you have to write will be parallel in both cases, because promises error flow is parallel to sync control flow in this regard.

[dragosrususv]

:) You have your way with words.
I understood what you tried to "pixelcopy" (so to say) the default mechanism, but you actually have another step in this flow, the THEN (not part of try-catch-finally) - this is the root of the idea of a promise I presume.

And what you are doing is to insert this "then" everywhere in this flow. Fine with me if that works.
But with the use-case above, as a developer, I am forced to insert some lines of code in my app that I do not use to achieve my goal - with this, I do not agree. There should be a way to achieve this without this extra line (not saying today, but in future, for the sake of writing nice code).

[domenic]

Well, you can always write a function. Similar to how you might not like to write try { x(); } catch (e) { } y(); and prefer to write foo(x, y), you could write a promise-based function that does the same thing:

function foo(x, y) {
  return x().catch(() => {}).then(y);
}

[dragosrususv]

@domenic : I can rewrite Q if I want. But that is not the goal here.
The goal and the main reason I insist is for other developers to be able to use a out-of-the-box library behavior - easy and with as less code as possible.
On my side I will probably go on with the catch solution (will get back if doesn't cover all cases).

But again, the point is to write clean code - everybody should have this freedom without hi-jacking existing libraries/frameworks and doing their own versions. Do you agree?

[domenic]

I disagree that what you are proposing is a feature that encourages clean code. I think clean code for promises is code that makes the structure of error flow clear. Hiding that behind a function is unclean code, whereas making it explicit by only using those structures that have clear analogies in synchronous code is clean.

[dragosrususv]

:) We'll just have to have a 3rd opinion I guess.
I disagree with the idea of adding code that I do not use. Period. This is a hack that I wouldn't have found myself if I didn't had the opportunity to speak with you. But how about the rest? Do they have to dig into source code, understand their logic and find work-arounds?

I still believe adding a parameter to the finally call is a good idea and it's also a good idea for the finally call to be called EVEN IF catch is not added - which happens today!!! I'm just saying: "add a parameter, so I know how to handle my finally"!

[domenic]

You should be OK creating your own abstractions on top of solid base ones.

.finally as an abstraction will not change in promises (I say this as a Q collaborator and the ES6 promises spec editor), because it is important it parallel synchronous finally.

If you want your own abstractions you can try to convince library authors to make them for you, or you can make them yourself. I advise the latter as it helps get work done faster.

[domenic]

And I take issue with the description of making your error flow clear as a hack. If you understand promises, or more simply just understand that promise error flow is exactly the same as try/catch/finally error flow, is it entirely natural and non-hacky, and anyone who sees it with that same shared understanding will understand it. I assume you have read http://domenic.me/2012/10/14/youre-missing-the-point-of-promises/ ?

[dragosrususv]

I totally respect your opinion. Still, you often think in a "doing it for myself" way, while I'm suggesting a more common approach - let others write clean code via general libraries - why not?

At this moment I don't have the background to evaluate your 2nd statement with parallel sync - you might just be right. And still, solutions can be found I'm sure.

And no, your blog is not a reference so I haven't read that. I will.

And as a final word: "finally" is called anyway - if you are so keen on having the "try-catch-finally" blocks, why are you calling the finally when no catch is there?

[domenic]

And as a final word: "finally" is called anyway - if you are so keen on having the "try-catch-finally" blocks, why are you calling the finally when no catch is there?

Finally is used when you don't catch the error, or when you rethrow. E.g.

try {
  doThing();
} catch (e) {
  logToServer(e);
  throw e;
} finally {
  cleanUpResources();
}

// this code doesn't run since the error is still thrown

<=>

doThing()
  .catch(e => { logToServer(e); throw e; })
  .finally(cleanUpResources)
  .then(() => { /* this code doesn't run since the promise is still rejected */ });

[dragosrususv]

@domenic : So this would be once difference between this library Q and AngularJS. In AngularJS finally is ALWAYS called - even if there's no error and even if there's no catch.

[dragosrususv]

@kriskowal : what's your opinion here, if the finally is called anyway, is there a real coomon-sense reason why that should not receive a parameter?

As per previous comments (and this will be my last), I do not agree with @domenic - I respect him from all points of view, but he's just keen on leaving as is, because it's easier. But it's NOT easier for people who use this library.

[domenic]

Finally is always called in Q as well (try it in your browser). It's just not useful if you don't expect there to be an error.

[dragosrususv]

:) I rest my case.

[kriskowal]

Existing code depends on the ability to pass a function with optional or variadic arguments to promise.finally, e.g., promise.finally(stream.close) and promise.finally(server.close). Passing an argument would interfere with these cases, in the same way that array.forEach(console.log) displays the index and array on the same line. Early versions of finally experimented with passing the error or value but I found this to be less useful that the ability to pass variadic functions. It is possible to check the state of a promise within the finally block, e.g., promise.finally(() => promise.inspect()).

[dragosrususv]

@kriskowal : how about an extra ".always()" that would have this parameter?
(see http://api.jquery.com/deferred.always/ )

I'm not keen on the name - it wast he only name I knew it would be called at the end, but that can be different.

[kriskowal]

Having always and finally side by side would do more to confuse than illuminate. I’m of the opinion that Q gives you all the tools you need as-is and that this sugar would not improve enough people’s lives to pay for itself.

[dragosrususv]

@kriskowal : so in your opinion adding a fake ".catch(function () {})" and another ".then(function (value) {})" makes perfect sense to handle the case when parameter is needed in some method that should be called at the end? (finally/always/whatever)

For me, as previously stated to @domenic , that is just a work-around - nobody will document such a thing because it's hilarious.

[domenic]

Anyone who understands how promise code parallels synchronous try/catch/finally---which should be everyone using promises---will find such a pattern natural and obvious because that is how you would do it in sync code. So indeed no need to document, not because it's hilarious, but because it's obvious.

[thatshailesh]

@domenic
How would you implement something like

Promise.reject(new Error("boo!"))
  .catch(function () { console.log('it should not go to next promise') })
  .then(function () { console.log("I should be called only if there is no error"); });

Below code doesn't work since .finally() doesn't take any arguments

.then((something) => {
  return () => { something.log(...) }
})
.catch(next)
.finally(cb => {
  //check if cb === function
  cb.call(this)
})

One way to do that
is

Promise.reject(new Error("boo!"))
  .catch(function (err) { next(err); 
     return {notify: false}
 })
.then(res => {
  if (res.notify) { console.log("I should be called only if there is no error"); }}
})

Any other cleaner way to achieve the same?

[benjamingr]

That train has sailed by now - finally is part of JavaScript.

Also, you should just do:

Promise.reject(new Error("boo!"))
.then(function () { console.log("I should be called only if there is no error"); }, function () { console.log('it should not go to next promise') });

Or, for this behavior do what synchronous code would (and rethrow the error with a throw).

Also, it is considered bad etiquette to bump issues from 2014