Error handling philosophy, when should you cb(err), cb(err, object) or cb(null, object) for non-success conditions
Owen Allen
Aria Stewart
There's a lot of blog posts around the internet talking about the mechanics of handling errors, be it domains or process.on("uncaughtException"). I can't find much definitive talk about the philosophy of error handling though. Specifically when should a function return an error cb(err), or return an error state such as cb(null, { success : false, message : "Some useful message", resp : httpResponse }).In example, lets imagine we're creating a social media NPM package which has an async function which queries a social media API via an HTTP call using the popular request module and returns some posts. Our function takes 3 arguments a userid and a count and a callback. There are a variety of exit scenarios for this function:
So I've responded with what I think.
1. Malformed arguments sent to the function such as a missing cb or a null/undefined/incorrectly formatted userid.
Throw -- use a library like aproba or otherwise validate args synchronously.
2. Unable to reach the service via HTTP.
Call back with error.
3. Service returned a non-200 status code for some reason, maybe we're being rate-limited.
If a library that exposes HTTP semantics, call back with no error but object with status.
If a library that hides HTTP semantics, call back with error.
It will really depend on what layer you're trying to expose; mixing them usually leads to confusing APIs. Clarifying what layer you are targeting can really help clarify how the API should work.
4. Reached the service, but the call failed at the service due to invalid parameters, some services return a 200 status code for this type of failure other's return a non-200 status code.
Services that report errors with 200 are awful. Fix that up as if the service behaves.
If it's a protocol concept that's exposed, call back with status object. If you're wrapping the protocol so the details aren't exposed, call back with error.
5. Reached the service, the call succeeded but returned an empty array of posts.
Empty success is still success. Call back with the empty array.
6. Reached the service, the call succeeded and returned data.
Call back with it.
7. The function throws an unhandled error due to programmer mistake.
Bummer. Catch that internally and call back with it. An async throw due to that kind of mistake in a callback is awful and crashes the whole process unrecoverably. This is the worst side-effect of error handling in node.
Static analysis to find cases, remove the catch when you get it right.
Usually my philosophy for handling errors in this case would be:1. cb(new Error("Invalid parameters, must pass a callback"));2. cb(err); // the error that was returned by the http request3. cb(null, { success : false, message : "Service returned a " + resp.statusCode + " status code", resp : resp });4. cb(null, { success : false, message : serviceResponse.message, resp : resp });5. cb(null, { success : true, data : [] });6. cb(null, { success : true, data : posts });7. Whoops, let error bubble up to the application employing our package and it will handle it or crash.
In general I find the success true/false to be a bad pattern: Either expose the protocol _and all the details_ for it, or don't, and hide them completely into the error path as debugging data, but don't expect the users of your API to act on that information.
I attach code properties to errors where I can. Use the VError class to attach context to inner exceptions, and to make formatting error messages easier with format strings.
That's just one way to approach the problem. It could be argued that cases 1 - 4 should all cb an error and that the error object should be packed with the additional data to help debug or be a custom error type. Yet at the same time some people argue it's a bad idea to pack additional non-standard keys on to Error objects because it results in inconsistency between Error objects and modules. If every npm module packed it's Error objects with custom keys wouldn't we end up in a pretty chaotic situation? In addition, if that error is thrown or console.error() those extra keys will not show up because of the native behavior of Error.prototype.toString().
Use the code property for consistent, machine-matchable tokens on errors. Custom subclasses are just annoying to define and give little useful information that a code property won't. Use the message for human readable message.
Yet another approach would be for 1 - 4 to return an error AND the additional data. Such as cb(new Error(serviceResp.message), { success : false, message : serviceResp.message, resp : resp }). This way downstream modules have the useful debugging information available, but we still end up treading down the error pathway of control flow structures such as async and promises. In example, in async if an error is returned it short-circuits all the way to the end.
I rarely see that go well -- sometimes it's clever and nice, but generally, you don't need it, and it's only confusing. And it's not particularly composable, in that not all error handlers do this, and passing two parameters through a complex chain of functions is far more difficult to get right. You end up throwing away the extra data if it's not attached to the error object, so just do that instead.
Lots of this is subjective, or just plain "better if people tend to do it the same way", but I find this approach works for me.
Aria
