Mixing exceptions and callbacks is generally frowned upon

[daleharvey]

It means as an API consumer I need to both handle error callbacks and use try { } catch, with a callback API I think it should be far better to

callback(new Error('TokenStore:authenticate called with invalid parameters'));

than the current https://github.com/florianheinemann/passwordless-mongostore/blob/master/lib/mongostore.js#L57

[florianheinemann]

Hey,

Not necessarily. The concept is based on the (potentially misguided) approach that exceptions should be reserved for unrecoverable states. If the API is called with the wrong number of parameters then that's something that should never ever happen in production but should be handled by the application (implementation according to spec, input validation, etc.). The app is supposed to crash to show you as a developer that work is left. No try / catch needed. Callbacks are reserved for anything that can happen in production and should be recovered from (e.g. invalid token, timeouts)

There is a fantastic article on that: https://www.joyent.com/developers/node/design/errors

I'll go through it once more and check if I followed that approach consistently but I'm also happy to consider better approaches! Still one of my first major Node projects.

Florian

On Oct 18, 2014, at 2:15 PM, Dale Harvey notifications@github.com wrote:

It means as an API consumer I need to both handle error callbacks and use try { } catch, with a callback API I think it should be far better to

callback(new Error('TokenStore:authenticate called with invalid parameters')); than the current https://github.com/florianheinemann/passwordless-mongostore/blob/master/lib/mongostore.js#L57

— Reply to this email directly or view it on GitHub.

[calvinmetcalf]

bad example with the connection parameters, you are correct that those would be programming errors, here is an operational error you throw (async too) https://github.com/florianheinemann/passwordless-mongostore/blob/master/lib/mongostore.js#L188

[florianheinemann]

Yes, indeed. That could be done better! I remember that there is some funny issue with reconnects to the database that are not really well documented in the MongoClient docs. I'll have to dive into it again but if I remember right MongoClient will try to reconnect several times before timing out. I think that led me down the path of saying: is there actually any way to realistically recover from this state? Retry on the next highest level maybe? I'll leave the issue open as for now. What are your thoughts?

[calvinmetcalf]

the main problem is that you're making assumptions about what state users higher up the chain may or may not be able to recover from, for example look at abstract leveldown the only errors it throws are if callbacks or certain required arguments are missing, if you pass an invalid type in you get it returned not thrown, in other words when in doubt assume it can be recoverable, e.g. at this line while a callback not being present should definitely throw, having a falsy uid could indicate accidentally trying to pass in an empty string because there was an error somewhere else where it was trying to get the uid.