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

Mark Hammond skippy.hammond at gmail.com
Sat Aug 10 03:04:59 UTC 2013


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




More information about the firefox-dev mailing list