Use ".then(null, Cu.reportError);" with Promise.jsm

[Joe Walker]

Promise chains often look like your examples, but with an extra feature:

    something.then(result => {
       *return*  step1.then(result => {
         *return*  step2.then(result => {
           *return*  42;
         });
       });
    }).then(console.log, console.error);


I added ".then(console.log, console.error);" to make it show what 
happens and some "returns" to make it more obvious what was going on.

The result of the above code would be console.log(42); because
* the result of a call to "then" is a promise of the return value of the 
function
* and promises internally resolve a promise of a promise of a thing, to 
be a promise of a thing

Clearly this can be (and often is) split up into several parts:

    something.then(result => {
       *return*  step1.then(doStuff);
    }).then(console.log, console.error);

    function doStuff(result) {
       *return*  step2.then(doMoreStuff);
    }

    function doMoreStuff(result) {
       *return*  42;
    }


Looking at doStuff and doMoreStuff saying "Where is the error 
handling?", misses that the error handling is in the thing that called 
doStuff.

The lack of a 'return' statement doesn't change this, because without an 
explicit return, the "then" function just returns a promise of 
undefined, which works like "Tell me when you're done".

The ratio of promise uses to error handling is surprisingly lopsided. I 
believe the GCLI code does error handling properly and the ratio there 
is 23 places where errors are handled, and 124 places where 'then' is 
called, and not 1:1. I just did a very quick scan through the devtools 
code, and I could believe that the ratio was similar there too.

Joe.

On 10/08/2013 04:04, Mark Hammond wrote:
> On 9/08/2013 8:32 PM, David Rajchenbach-Teller wrote:
>> Furthermore, I *strongly* suggest that we r- any promise chain that does
>> not report errors, unless there is a comment explaining why we do not.
>
> This concept of a "promise chain" escapes me.  I understand that 
> conceptually, promise code should look like:
>
>   something.then(step1)
>            .then(step2)
>            .then(step3)
>            .then(null, Cu.reportError)
>
> One nice promise chain and the final .then() looks reasonable. But 
> every time I try and use promises, they actually end up looking like:
>
>   something.then(result => {
>     step1.then(result => {
>       step2.then(result => {
>         step3.then(result => {
>         });
>       });
>     });
>   })
>
> in that case we have 4 "promise chains".  Thus, we need 4 extra 
> .then(null, Cu.reportError) calls, making the code much larger and 
> harder to follow.
>
> At first I thought it was just me, so I started digging around in our 
> code.  I looked at non-test code in browser/base and toolkit/ (but 
> stopped at devtools), and examined over 30 calls to .then() - and 
> couldn't find a *single* case of a promise chain.  The closest I could 
> find was a few places where .then(null, Cu.reportError) was correctly 
> at the end, giving a promise chain of length 2.
>
> So, in practice in our code, *every single* .then() call must have 
> .then(null, Cu.reportError) tacked onto the end.  Given so few of our 
> existing ones do this, fixing this code would require nearly doubling 
> the number of .then() calls in the code.
>
> While most don't have any error reporting, the ones that do attempt to 
> handle errors may or may not be correct, depending on your subjective 
> viewpoint.  For example, code like:
>
>    .then(null, error => {
>       this.foo.bar = "error";
>     });
>
> is still poor form IMO - in exceptional circumstances, this.foo might 
> be null, so a final .then() should still be used even if the error 
> looks extremely unlikely.  This is a subjective call, and they are 
> hard to get right - is the final .then() really necessary there? Would 
> the "unless there is a comment" clause get us out of this?
>
> So - I couldn't find a single "promise chain", and the vast majority 
> of promises fail to handle errors correctly.  Of the ones that *do* 
> handle errors correctly, overwhelmingly they do the trivial 
> .then(null, Cu.reportError).  I could only find a few cases that used 
> more sophisticated error handling.  Or to put it another way - if 
> promises magically reported errors, there would only be a few 
> onRejected handlers left in the code I examined and the vast majority 
> of our code would suddenly become correct without any further changes.
>
> IMO, something is very wrong with this picture.  Please understand I'm 
> not looking to blame anyone or anything.  Promise-based code, at first 
> glance, looks like a huge improvement over what we were doing before. 
> It just concerns me that to do it right means bloating the code with 
> onRejected handlers that just do what should be done automatically 
> anyway - and that clearly we've failed to do this in the past, so I'm 
> not confident we will consistently do it correctly in the future.
>
> I'm relatively new to promises so apologize if I got any of the 
> details above wrong.  Also, I didn't do a thorough examination of all 
> promises in the tree, so I may have overlooked significant chunks of 
> code that don't suffer the problems I've mentioned above (but if I 
> have, please point it out to me so I can learn from it!)  This 
> conversation is purely motivated by the fact that I like compact code 
> so my brain doesn't overflow, and that I personally screw things up so 
> often that without good, reliable error reporting I fail to achieve 
> anything at all :)
>
> Cheers,
>
> Mark
>
> _______________________________________________
> firefox-dev mailing list
> firefox-dev at mozilla.org
> https://mail.mozilla.org/listinfo/firefox-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.mozilla.org/pipermail/firefox-dev/attachments/20130810/c30fcaa9/attachment.html>