Skip to content

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

Closed
wants to merge 3 commits into from

Conversation

pentateu
Copy link
Contributor

No description provided.

@@ -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) {
Copy link
Member

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.

Copy link
Contributor Author

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.

@kingcody
Copy link
Member

@pentateu not a big deal, but chai-as-promised was implemented in canary. Your user.mode.spec.js could be tweaked like so:

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;
});

@kingcody
Copy link
Member

@pentateu what are your thoughts on mongoomise?

@pentateu
Copy link
Contributor Author

@kingcody I was not aware of mongoomise when I did mongoose-bird. It looks good.
I like that with mongoomise you can choose any promise library you want.
With mongoomise you need to load your modals first and I think this is an inconvenience.
With mongoose-bird you don't have this problem, you load mongoose using mongoose-bird and everything else after that is promisified, including any model you load afterwards.
I believe that mongoomise could be tweaked to work as mongoose-bird so you don't need to load the models first.

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.

@DaftMonk
Copy link
Member

DaftMonk commented Sep 1, 2014

Looks great. Much easier to read these methods when they're using promises.

@DaftMonk
Copy link
Member

DaftMonk commented Sep 1, 2014

Could you update it against canary?

@pentateu
Copy link
Contributor Author

pentateu commented Sep 1, 2014

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 () {
Copy link
Member

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?

Copy link
Contributor Author

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.

@kingcody
Copy link
Member

kingcody commented Sep 3, 2014

@pentateu feel free to take a look at #520. I rebased your PR onto canary and adjusted a few things. If you'd like to use 520, just let me know.

Thanks

@pentateu
Copy link
Contributor Author

pentateu commented Sep 4, 2014

Hi @DaftMonk I have rebased the PR on canary ..

I had a look on PR #520 by @kingcody , but I noticed that there were a few <% if (filters.mongoose) { %> missing from the templates.

@kingcody
Copy link
Member

kingcody commented Sep 4, 2014

@pentateu I updated the line comments in #520 to indicate where and why they were moved.

@kingcody
Copy link
Member

kingcody commented Sep 4, 2014

Also @pentateu your build is failing because canary no longer uses should.js and instead uses Chai assertions. If you look in mocha.conf.js you should see where chai.should() is initialize as well as defining expect, assert, and sinon as globals for the tests.

You should be able to simply remove var should = require('should'); from the tests to correct the issue.

@pentateu
Copy link
Contributor Author

pentateu commented Sep 4, 2014

@kingcody thanks for the heads-up.

@JaKXz
Copy link
Collaborator

JaKXz commented Sep 8, 2014

This should be closed in favour of #520, yes?

@pentateu
Copy link
Contributor Author

Sorry for the delay.. I have fixed the issues with lodash not being inside the mongoose condition.

@pentateu
Copy link
Contributor Author

@JaKXz I'm happy either way.. merging this one or merging #520 will have the same result.

@DaftMonk
Copy link
Member

Closed with #520

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants