New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Node callbacks for fetch, load #20
Comments
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 It would be a little more work internally to support both interfaces - since there are a lot of promises building up internally (vs. It was also sort of a preference to keep things consistent with how Backbone.js 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.
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. |
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 |
Yep the latter works well, would save me tons of effort not having to rewrite all my code. |
So... I'd ideally like to have 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 var Bookshelf = require('bookshelf');
require('bookshelf-exec');
new Bookshelf.Model({id: 1}, {tableName: 'items'}).fetch().exec(function(err, resp) {
...
}); |
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? |
Small fix for incorrect context being used in apply.
and |
Using this
Say I do use a promise and then want to use
I think what is needed is a At the moment I am using
to write:
Not sure if you had any idea of an alternative method I could investigate? |
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) |
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 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 :) |
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 |
Ok I'll give it a whirl. Cheers! |
You can do: Bookshelf = require('bookshelf');
require('bookshelf/plugins/exec'); and then Bookshelf will have the Bookshelf.wrapExec(YourModel.prototype, 'findOrCreate'); |
Thanks. FYI, just found out that in the upcoming version 2.2.0 of when.js they are introducting
|
@tgriesser I'm getting an error with the new
This is causing the method to run twice because of this line: Not sure what the purpose of this check is? |
Perhaps from line 17 should read:
|
Yep, that's what it should be... thanks |
It appears calling
fetch().exec
is not possible like it is in Knex.Can we have this option?
The text was updated successfully, but these errors were encountered: