Skip to content
This repository has been archived by the owner on Feb 19, 2019. It is now read-only.

Better error reporting / handling #20

Open
sole opened this issue Jan 30, 2015 · 4 comments
Open

Better error reporting / handling #20

sole opened this issue Jan 30, 2015 · 4 comments

Comments

@sole
Copy link
Contributor

sole commented Jan 30, 2015

While Promises are cool etc they also capture errors in a sneaky way and sometimes it's impossible to find where your error is unless you delve into the code and start console.logging like silly.

For example I was passing a wrong parameter to connect and the promise was capturing the error in getWebAppsActor call, which overall remained silent. We should review the flow of each function and make sure errors can bubble up (perhaps intentionally building errors!)

This is probably due to my own lack of experience with promises so I accept all fault already :-)

@tofumatt
Copy link
Contributor

If you’re finding the Promises are swallowing up too many errors, a common fix is to change:

callingAPromiseFromLibrary().then(function() {
  // doSomething
}, function(err) {
  // handle the error
});

to:

callingAPromiseFromLibrary().then(function() {
  // doSomething
}).catch(function(err) {
  // handle the error
});

We do this in localForage and it prevents promise errors inside library code (which is what I assume you're hitting) from being "swallowed" by the Promises lib.

@tofumatt
Copy link
Contributor

Related: this is probably good practice to convert to anyway, and maybe I can inspect linting code to look for this type of Promise implementation in our code. Users shouldn't have to write promises this way, but our code should.

It's mildly annoying at first, when dealing with the otherwise pleasant Promises API, to run into this 😒

@sole
Copy link
Contributor Author

sole commented Jan 30, 2015

Yeah, I am using catch in places but the errors are not bubbling up often - because probably the chain of promises is not being returned. I am not sure that linting is going to solve this, it requires looking at the code by a human :-P

Also, the second argument to then is not an exception but a rejection which should not throw.

@tofumatt
Copy link
Contributor

Right, but if I understand the chain correctly that means we aren't doing .catch() all the way up our Promises chain so somewhere the error is hidden...

https://github.com/mozilla/node-firefox-connect/blob/master/index.js#L10

That would mean having to wrap the client.connect bit in a try/catch block to prevent the Promise lib from catching the error, if I follow correctly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants