-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat (mongoose): use mongoose-bird to promisify the mongoose API #503
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
Conversation
@@ -5,19 +5,37 @@ var passport = require('passport'); | |||
var config = require('../../config/environment'); | |||
var jwt = require('jsonwebtoken'); | |||
|
|||
var validationError = function(res, err) { | |||
return res.json(422, err); | |||
var validationError = function(res) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this line to: var validationError = function(res, code) {
should resolve your build error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool.thanks for the heads up.
@pentateu not a big deal, but it('should fail when saving a duplicate user', function() {
return user.saveAsync()
.then(function() {
var userDup = new User(user);
return userDup.saveAsync();
})
.should.be.rejected;
});
it('should fail when saving without an email', function() {
user.email = '';
return user.saveAsync().should.be.rejected;
}); |
@pentateu what are your thoughts on mongoomise? |
@kingcody I was not aware of mongoomise when I did mongoose-bird. It looks good. Anyway having options is good, but is comes with a cost of complexity which in the case of generator is a relevant topic. mongoomise benchmark also points to blubird as having the best performance. That is why I have chosen to use blubird. In summary I would stick with mongoose-bird for now. |
Looks great. Much easier to read these methods when they're using promises. |
Could you update it against canary? |
Yeah.. I'll find some time this weak to rebase on Canary and also do the unit test updates suggested using chai-as-promise |
function saveUpdates(updates){ | ||
return function(entity){ | ||
var updated = _.merge(entity, updates); | ||
return updated.saveAsync(function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pentateu please correct me if I'm wrong, but shouldn't this be:
return updated.saveAsync()
.then(function() {
return updated;
});
I actually had problems with passing a callback. Perhaps I was doing something wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes :-) I have it fixed now.
Also @pentateu your build is failing because canary no longer uses You should be able to simply remove |
@kingcody thanks for the heads-up. |
This should be closed in favour of #520, yes? |
Sorry for the delay.. I have fixed the issues with lodash not being inside the mongoose condition. |
Closed with #520 |
No description provided.