Change pattern for callbacks to node-style error first

[jviereck]

The callbacks to getItem(...) etc. contain the result value as first argument. If the method call fails, there is no way to get hold of this information from just using the callback.

Therefore, I propose to change the signature of the callbacks to be:

  callback(error, value)

similar to the style used in node.js.

[tofumatt]

I’ve always disliked that node.js (error, value) -> style, at the very least I’d like error to follow value, but what’s the convention in browser-land?

What method calls are we talking about failing?

[sole]

I think we've had this discussion before... when Jen suggested using node style callbacks in order to check for error values. The conclusion was that it broke the expected return value.

But given that we're not using this in the same way that localStorage is used (we don't just use the return value, but send in a callback), we might be able to change that too (and @ednapiranha would be happier).

That said, the version number should be bumped or make a giant CHANGELOG item that warns people that the methods have changed their signature.

[jviereck]

What method calls are we talking about failing?

All of the available API functions (setItem, getItem, ...). E.g. the
WebSQL driver might fail on any API function being used as there is an
error with the underlaying WebSQL database.

but what’s the convention in browser-land?

I do not know. Putting the error first is a little bit annoying when
writing the callback, but on the other hand, if the developer ignores
possible errors to be handled in his application, they are doing
something wrong. So by putting the error value as first argument it's a
way to make developers think about error handling, which I think is a
positive point :)

On 14/02/14 00:24, Matthew Riley MacPherson wrote:

I’ve always disliked that node.js (error, value) -> style, at the
very least I’d like error to follow value, but what’s the convention
in browser-land?

What method calls are we talking about failing?

—
Reply to this email directly or view it on GitHub
#55 (comment).

[operatorjen]

If it's localStorage-specific, always return a null error on callback? Otherwise, return an error if it fails on async storage.

I don't think putting the error first is annoying - it's a standard practice that is done these days.

[tofumatt]

I actually think what I'd like to see is an error callback and a failure callback, particularly because that maps to promises nicer than a single callback with an error and value argument. The node-style means that every single callback function then needs to deal with errors internally instead of having a generic error handler (which most of the time would be to do with a storage limit exceeded error). I'd rather an extra callback than every function starting with if (error) which strikes me as boilerplate code and less reusable.

So let's say we should implement an error callback as the second argument, and use that same error callback as the promise rejection callback.

[magalhas]

Libraries that implement promises on the browser like Q expect a node style callback. I like the approach and also works as expected by other libraries. Thumbs up for the change.

[tofumatt]

localForage uses ES6-style, Promises-spec promises. They don’t expect an error value in their success callbacks, they just offer a .then(success, failure) setup.

[magalhas]

Having callback(error, value) doesn't break that interface. Check Q documentation.

[alekseykulikov]

FYI: I created a wrapper around localForage with support of node-style callbacks https://github.com/ask11/storage