callback signatures

[Trott]

I'm experimenting with using Freebase.js in a Node.js project.

The Node.js convention for callbacks is to pass either null (if no error) or an Error object for the first parameter, and the data/result of that the user wants as the second parameter. Freebase.js doesn't do this. And in fact, the way it sends back errors appears to be different in different situations. (Or maybe it's not and I'm just confused. Regardless, I want to stick to convention.)

So, there are a few potential solutions for me. The absolute best would be for Freebase.js to switch to the convention. Would you be open to that? It appears that the Node.js callbacks happen in different places mostly than the web/client-side callbacks, so I think it's manageable. I could submit a pull request updating the code and inserting a line in the README saying "For Node.js, blah blah blah instead." But I don't want to do that if you'd be hesitant.

[spencermountain]

hey rich, I get what you're saying. It shouldn't eat errors the way it does. for what it's worth, mqlread and paginate methods (and those that use them) always return the error in callback(result){result.error}. so you could gork validation yourself somehow. That's a freebase convention, not mine. Methods that use freebase.search() have been built to accept an array, so no metadata there, and that's a harder change to make.

honestly, I always forget about the error-first parameter, and have cursed it/myself a number of times. maybe theres a lazy way to return an error? cheeers

[Trott]

For now, I'm just running all my Node-style callbacks through a function like this to get a Freebase-style callback. (cleanup is just a function I pass that does any data massaging on the result before the callback is invoked.)

var callbackify = function (callback, cleanup) {
  return function (data) {
    if (!data) {
      callback(new Error("unknown error"));
    }
    if (data.result) {
      var result = cleanup(data.result);
      callback(null, result);
    } else {
      callback(data);
    }
  };
};

Seems like maybe I should tweak that last line to be more like callback(data.error || data). But you get the idea.

Mostly putting this here in case someone else finds it useful in the future.

[Trott]

So if I go and make the changes for Node (so that I don't have to use the callbackify() function above) and submit a pull request, that is:

• [ ] cool by you
• [x] probably not going to be merged because lots of good reasons, doesn't really matter what they are for purposes of this discussion
• [ ] cool by you but I should change the web-based/client-side callback signatures too
• [ ] no idea, I'm just going to have to do it if I want you to look at it and make a decision one way or the other

[spencermountain]

ha ;) yeah, unless it was somehow optional, or lazy, and backwards compatible, it would be a breaking change for a lot of projects

[spencermountain]

maybe it could be a supported flag in the option object? like {blow_up:true} ?

[spencermountain]

actually, I kinda like that- would that work for you?

or, maybe we could start throwing (result.error || null) into the second paramater of the callback? then you could just swap them aposteriori?

[Trott]

Could tie it to an option, I think.

[spencermountain]

let a rip! <3

[Trott]

Here it is for just mqlread(). If that looks good, I'll go and do it elsewhere. (I'll probably create a helper function so if you decide you dislike nodeCallback and want the option to be node_callback, you only have to change it in one place.) But basically, is that along the lines you're thinking and I should go ahead? https://github.com/Trott/Freebase.js/compare/spencermountain:06abc5e1caa8a87c101302409869699a7880b06d...91cac7194bcdb255fbab61905f6132bf13c16601

[spencermountain]

looks good to me.