What's with the arrow effects on Promises?

[jods4]

I was reading some Aurelia source code and I really couldn't help but notice those terrible pyramids of Promise. Look for instance at dialog-controller.js for a short file where the style of coding is obvious.

One (smaller) benefit of the Promise API is that we can flatten our code compared to callbacks and put all the .then() on the same level:

return p1
  .then((result1) => {
    return p2;
  })
  .then((result2) => {
    return p3;
  })
  .then(() => { /* etc */ });

I understand that returning the first .then() rather than the nested ones has a subtly different semantic and behavior, but I can't find a reason for that in the code. Actually, it seems to me that it makes more sense to return the last promise in the chain and not the first one. EDIT: actually I think that given how the returns are nested the end result is exactly the same, but the code is less readable and less performant.

Other example:
https://github.com/aurelia/dialog/blob/master/src/dialog-service.js#L44
Also note that the return on this line (L44) is useless.

[plwalters]

The handling o the promises being resolved appear to be nested properly to show dependency on the parent from my perspective - if there is a less performant we'd welcome a pull request that demonstrates an issue / solution. A useless return statement isn't less performant as far as I know where as a conditional check with a return to prevent executing a code block would indeed be less performant, so in this regard as long as we have some tests or reasons to improve the code-base it would definitely be accepted.

[jods4]

The most important aspect of this in my opinion is that this is not idiomatic JS and it makes for really hard to read/understand code.

As a poster child example, of course the useless return on L44 has no impact on perf whether you remove it or not. But it is highly misleading. For a reader like me it implies that you are returning the promise with possible intention to chain it. But in reality, the return value is dismissed since you're not inside a .then() but inside a new Promise((resolve, reject) =>...). It hinders understanding and I'm ready to bet that the person who wrote this code didn't realize that the "returned" promise was actually lost.

The perf aspect is related to memory pressure. Consider a chain of Promises A -> B -> C -> D. Implementation (1) is to do A.then(B.then(C.then(D))) -- what the code does. Implementation (2) is A.then(B).then(C).then(D) -- idiomatic JS.

Observe that in case (1) it is only when the chain is complete (D resolves) that C.then resolves, which makes B.then resolve, which makes A.then resolve. This is what you want, but it means that no promise can be garbage collected before the last one resolves. In some pathological cases that might be a long time.

Case (2) is better because as soon as A resolves it can be GC'ed and so on.

To put it otherwise, in case (1) client code waits on A.then, in case (2) client code waits on .then(D). Very different from GC perspective.

It is also different from a polyfill point of view. A polyfill typically uses setTimeout(f, 0) to schedule promises continuations. In recent Chrome releases, there is a 4ms clamp on setTimeout if it's deeply nested. Observe that in case (1) you unwind all the promises in a long chain at the end, so you are in the "deeply nested" case. In case (2) you only resolve promises in a spread out fashion, which is not affected. In older browsers there is a 10ms clamp on all setTimeout calls, which you will pay as one big fat check at the end of (1) and spread out in (2).

[EisenbergEffect]

@jods4 We would definitely be interested in a PR to improve this. I wrote that code in haste a month or so ago and hadn't had time to think about it much since. We would appreciate your assistance in improving these issues.

Do you have a resource I can read related to the information you have above? In some of my reading I thought I read that nesting the promises was preferred in order to not lose errors that might occur...but maybe I remembered that wrong or maybe the source I had was wrong.

In any case, we are open to making improvements, especially those that improve performance and lower memory pressure. Please sign our CLA (if you haven't already) and submit a PR. We'd be happy to accept it.

[jods4]

@EisenbergEffect I have signed the CLA already. If you take PR for this I might have a look at it when I have some time, but that may not be soon as I have a lot of work right now. <_<

Errors flow through the Promise chain. So if you do A.then(B).then(C).then(D).catch(X) you will catch in X any error occuring in the chain. More precisely, if say B throws, the .then(B) promise will reject, which will automatically reject .then(C), which rejects .then(D) and so on until there is a reject handler that catch it in the chain (here X).

Note that you can continue the chain from there. Once the rejection has been handled everything resolves again, e.g.: if you do A.then(B).catch(X).then(C), C will run (that is, assuming X itself does not throw, which would of course reject the .catch(X) promise).

[EisenbergEffect]

@jods4 Thanks for taking time to write up some information. I know you're busy and thank you for being willing to engage and help us make Aurelia better.

[sethlivingston]

Hey guys, this is "Rookie Mistake #1" in Nolan Lawson's (of PouchDB) article here: http://pouchdb.com/2015/05/18/we-have-a-problem-with-promises.html

I noticed the pyramid right away, too. There is not a performance problem here, but rather a subtle but significant lack of understanding in how promises work. I was in the same boat, so I know first-hand that correcting this will help you use and debug promises more effectively.

[EisenbergEffect]

Yes, you are 100% correct. This needs to be fixed. @sethlivingston Would you be willing to create a PR to fix it? 😄

[sethlivingston]

I have it ready and will submit when #99 gets done. (And please point me somewhere if that's one I can help with, too. I'm assuming it's a general update needed for multiple Aurelia repos, but maybe I'm wrong.)

[plwalters]

#99 is all resolved now, thanks again for contributing @sethlivingston

[plwalters]

Closed by community PR.