Support Node callbacks for fetch, load

[bookshelf]

Support Node callbacks for fetch, load

[tgriesser]

I had a longer response for this one but then I accidentally refreshed the browser and lost it...

Basically the gist was that I didn't think it made as much sense to support here because unlike in Knex where you're building up a bunch of pieces in the query chain and then the then or exec signifies that everything chained before it should be built into a query, in Bookshelf, the save, fetch, load, and destroy all have to run at the time they're called.

It would be a little more work internally to support both interfaces - since there are a lot of promises building up internally (vs. Knex where there is one, maybe two at a time in the case of schema building) there'd be a bit more needed to make sure it's taken care of correctly everywhere, and I thought it might be confusing to support both.

It was also sort of a preference to keep things consistent with how Backbone.js jqXHR return values provide promises on the frontend, so you can potentially do model.save().then(function() {... on both sides of the wire and have it just work.

I'm curious if you had specific reasons for preferring the node style callbacks over promises.

[vjpr]

I'm curious if you had specific reasons for preferring the node style callbacks over promises.

All my code is currently written with Iced Coffee Script. I find it easier to read and refactor code when theres less nesting. Just personal preference.

E.g. findOrCreate

    findOrCreate: (title, url, done) ->
      await @find(where: {url}).done defer done, link
      return done(null, link) if link?
      link = @build({title}).setUrl(url)
      await link.save().done defer()
      done null, link

Promises are great and its definitely advantageous for standard JS - and I might start using them more even with Iced Coffee.

I'd say I'm a minority using Iced Coffee, but I would argue there would be a lot of code using regular Node.js callbacks, and providing support for it would make it easier for people to transition to this lib.

At the moment I need to refactor all my code from Node.js callbacks to use Bookshelf. Would be tons easier if callbacks were supported. I think many people would be in same boat.

[tgriesser]

That makes some sense, I'll take a closer look. So which would be preferable, having an optional callback as the final parameter, or an explicit exec method similar to Knex... I think I'd prefer the latter.

[vjpr]

Yep the latter works well, would save me tons of effort not having to rewrite all my code.

[tgriesser]

So... I'd ideally like to have save, fetch, destroy, and load actually perform the database action when they're called, but adding an exec method sort of implies that you aren't actually executing the command until you call it... which wouldn't be technically correct.

So I wanted to see what you thought if there was a small plugin that shimmed in this functionality:

var Bookshelf = require('bookshelf');

// Used to optionally add `exec` support for those who prefer node-style callbacks.
var wrapExec = function(target, method) {
  var targetMethod = target[method];
  target[method] = function() {
    var args = arguments;
    return {
      then: function(onFulfilled, onRejected) {
        return targetMethod.apply(this, args).then(onFulfilled, onRejected);
      },
      exec: function(callback) {
        return targetMethod.apply(this, args).then(function(resp) {
          callback(null, resp);
        }, function(err) {
          callback(err, null);
        }).then(null, function(err) {
          setTimeout(function() { throw err; }, 0);
        });
      }
    };
  };
};

_.each(['load', 'fetch', 'save', 'destroy'], function(method) {
  wrapExec(Bookshelf.Model.prototype, method);
});

_.each(['load', 'fetch'], function(method) {
  wrapExec(Bookshelf.Collection.prototype, method);
});

module.exports = Bookshelf;

So in your application you'd just need to require it the first time you used it, and then you'd be good to have an exec call on all appropriate methods:

var Bookshelf = require('bookshelf');
require('bookshelf-exec');

new Bookshelf.Model({id: 1}, {tableName: 'items'}).fetch().exec(function(err, resp) {
  ...
});

[vjpr]

That looks great. I'll try it out now.

When you say plugin, is this going to be bundled with the lib or would I have to add it separately?

[vjpr]

Small fix for incorrect context being used in apply.

target[method] = function() {
    var that = this;
    var args = arguments;
    return {
      then: function(onFulfilled, onRejected) {
        return targetMethod.apply(that, args).then(onFulfilled, onRejected);
      },

and exec is the same.

[vjpr]

Using this findOrCreate example it looks like promises are incompatible with Iced Coffee Script.

  @findOrCreate: (where, attrs, options, done) ->
    model = new @(where)
    model.fetch(_.extend options, require: true).then null, (e) ->
      return model.set(attrs).save() if e.message is "EmptyResponse"
      throw e

Say I do use a promise and then want to use await defer:

user = User.findOrCreate(id: 1, name: 'Bob') // user is a promise

await user.exec defer e, user // doesn't work

await user.then defer r, defer e // won't move on because Iced needs both defers to be satisfied

await user.always defer r // Promise#always is deprecated though
return next(r) if _.isError r

I think what is needed is a .exec method on the when promise library. Shame the Promise.prototype is private.

At the moment I am using always and a wrapper:

# onError - function to be called when promise rejected.
# onSuccess - function to be called when promise is fulfilled.
esc = (onError, onSuccess) ->
  return (result) ->
    if _.isError result
      onError? result
    else
      onSuccess? result

to write:

app.get '/user', (req, res, done) ->
  User.find(parseInt(id)).always esc done

Not sure if you had any idea of an alternative method I could investigate?

[tgriesser]

Honestly, it might be easier for you to just use promises and not worry about ICS's defer in this case:

render = (res) -> (obj) -> res.json(object)

app.get '/user', (req, res, next) ->
  User.find(parseInt(req.params.id)).then(render(res), next)

[vjpr]

This is a contrived example.

I have tons of code with much more complicated logic. I'd have to rewrite all my code to use promises which is unappealing. Plus the framework I am working (auth logic, controllers helpers etc) is all errback based.

Sequelize offers the .done chaining function for perfect errback support on every method, but it looks like it has a much different mechanism for resolving promises.

What would need to be done to implement exec in Bookshelf? - I would be happy to have a crack at it if you provided some pointers :)

[tgriesser]

Hmm so... I think if you do this, it should take care of it:

_.each(['load', 'fetch', 'save', 'destroy', 'findOrCreate'], function(method) {
  wrapExec(Bookshelf.Model.prototype, method);
});

Basically calling the wrapExec function I provided above on whatever custom methods you add...

[vjpr]

Ok I'll give it a whirl. Cheers!

[tgriesser]

You can do:

Bookshelf = require('bookshelf');
require('bookshelf/plugins/exec');

and then Bookshelf will have the wrapExec method attached to it, so you can do

Bookshelf.wrapExec(YourModel.prototype, 'findOrCreate');

[vjpr]

Thanks. FYI, just found out that in the upcoming version 2.2.0 of when.js they are introducting when.bindCallback() allowing attachment of node-style callback: https://github.com/cujojs/when/blob/dev/node/function.js#L196

function bindCallback(promise, callback) {
        return when(promise, callback && success, callback);

        function success(value) {
            return callback(null, value);
        }
    }

[vjpr]

@tgriesser I'm getting an error with the new bookshelf/plugins/exec:

Models.User.forge(user).save().exec (e, r) ->
        return done e if e
        console.log u
        done()

      1) #findById
{ sql: 'insert into "users" ("created_at", "email", "id", "name", "password", "updated_at") values ($1, $2, $3, $4, $5, $6) returning "id"',
  bindings: 
   [ Thu Jun 20 2013 15:39:42 GMT+1000 (EST),
     '[email protected]',
     'ce09ba2a-a554-47ba-91bd-0f4d91d1db1a',
     'Vaughan',
     'test',
     Thu Jun 20 2013 15:39:42 GMT+1000 (EST),
     Thu Jun 20 2013 15:39:42 GMT+1000 (EST),
     '[email protected]',
     'ce09ba2a-a554-47ba-91bd-0f4d91d1db1a',
     'Vaughan',
     'test',
     Thu Jun 20 2013 15:39:42 GMT+1000 (EST) ],
  __cid: '__cid1' }


  ✖ 1 of 1 test failed:

  1) Models UserService #findById:
     Error: The query type has already been set to insert
      at Common._setType (xxx/node_modules/knex/knex.js:96:15)
      at _.extend.insert (xxx/node_modules/knex/knex.js:794:19)
      at _.extend.insert (xxx/node_modules/bookshelf/bookshelf.js:914:10)
      at _.extend.save.when.all.then.then.model.attributes.(anonymous function).model.(anonymous function) (xxx/node_modules/bookshelf/bookshelf.js:399:45)
      at Object.i [as then] (xxx/node_modules/when/when.js:355:15)
      at handlers (xxx/node_modules/when/when.js:253:12)
      at len (xxx/node_modules/when/when.js:417:5)
      at drainQueue (xxx/node_modules/when/when.js:735:4)
      at process.startup.processNextTick.process._tickCallback (node.js:244:9)

result is set to arguments which in the case of save() is empty.

This is causing the method to run twice because of this line: result || (result = targetMethod.apply(ctx, args));

Not sure what the purpose of this check is?

[vjpr]

Perhaps from line 17 should read:

return result.then(function(resp) {
          callback(null, resp);
        }, function(err) {
          callback(err, null);
        }).then(null, function(err) {
          setTimeout(function() { throw err; }, 0);
        });

[tgriesser]

Yep, that's what it should be... thanks