Hello

[petkaantonov]

I stumbled upon here and wanted to ask for feedback as one goal of bluebird is to make wrapping extremely trivial and thus utilities less necessary :).

I was wondering why the following wouldn't work for you:

var Promise = require('bluebird'),
    mongoose = require('mongoose'),
    User = Promise.promisifyAll(mongoose.model('User'));

//For promisified instance methods
Promise.promisifyAll(User.prototype);

function setNameToJohn (old_name) {
    return User.findOneAsync({ name: old_name }).then(function(user) {
        if (!user) {
            var err = new Error('User not found');
            err.status = 404; // I do this to bind HTTP response codes into errors
            throw err;
        }
        user.name = "John";
        return user.saveAsync();
    });
}

[yamadapc]

Hello!
First of all, congratulations on bluebird. :)

I'm working on a relatively big project and I'm kind of trying to avoid
repeating myself, especially when it comes to throwing errors or writing
trivial code over and over.

But yeah... That seemed as if it would work fine.
(It hadn't crossed my mind "promisifying" the prototype - thanks for that tip BTW)

This specific case doesn't seem to work properly though. I'm still trying to
figure out why, but it seems to have to do with mongoose's internals, rather
than bluebird's.

Here's a reproduction:

var Promise = require('bluebird');
var mongoose = require('mongoose');

var UserSchema = new mongoose.Schema({
  name: { type: String }
});

var User = mongoose.model('User', UserSchema);

User = Promise.promisifyAll(User);
Promise.promisifyAll(User.prototype);

This last line throws:

TypeError: Cannot read property 'scope' of undefined
    at model.Object.defineProperty.get [as __v] (node_modules/mongoose/lib/document.js:1199:58)
    at _promisify (node_modules/bluebird/js/main/promisify.js:150:32)
    at Function.Promise$PromisifyAll [as promisifyAll] (node_modules/bluebird/js/main/promisify.js:200:12)
    at Object.<anonymous> (something.js:11:9)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)

Haha... This is actually a much more interesting problem than what I started with... I actually tried promisifying other parts of the Model, but I can't seem to get it to work.

I'll look more into this later today or tomorrow morning.

[petkaantonov]

Wow very good find. I have fixed this bug in: petkaantonov/bluebird@beb26fb which is in 0.10.11

It should now work:

Promise.promisifyAll(User.prototype);
console.log(User.prototype.getAsync);
{ [Function: getAsync] __isPromisified__: true }

[inetfuture]

Seems saveAsync() won't trigger validation like save() dose, because its tricky hook mechanism. So I used this work around:

###*
 * Fix broken function after mongoose model is promisified by bluebird
 * @param  {Model} model mongoose model to fix
###
exports.fixPromisify = (model) ->
  # Somehow, saveAsync doesn't trigger validation, so run validateAsync first
  originalSaveAsync = model.prototype.saveAsync
  model.prototype.saveAsync = ->
    @validateAsync().then =>
      originalSaveAsync.call(this)

Any advice?

[petkaantonov]

I don't see how save validates something here https://github.com/LearnBoost/mongoose/blob/3.8.x/lib/model.js#L167-L216

Anyway, if there is problem it's somewhere else saveAsync() just calls save() exactly the same way you would normally call it.

[yamadapc]

Validation is run at a pre 'save' hook

But that's weird, saveAsync would call Document.prototype.validate either way.

Maybe hooks-js breaks with this, but I don't have this problem, so I don't think that's the case.

[inetfuture]

With the hack I mentioned previously, validation works, but pre and post hook still not
@yamadapc are you sure you don't have this problem?
Here is what I'm doing:

###*
 * Promisify model with bluebird
 * @param  {Model} model mongoose model to promisify
###
exports.promisifyModel = (model) ->
  Promise.promisifyAll model
  Promise.promisifyAll model.prototype

  # Somehow, saveAsync doesn't trigger validation, so run validateAsync first
  # FIXME: other hooks(pre/post) still can not be triggered
  originalSaveAsync = model.prototype.saveAsync
  model.prototype.saveAsync = ->
    @validateAsync().then =>
      originalSaveAsync.call this

[yamadapc]

@inetfuture @petkaantonov
Okay... Now I'm not sure...

I made a bunch of experiments to try to understand it and found the problem...
They are here: https://gist.github.com/9788938

The problem is hooks-js is added at the document constructor level (not at the model constructor or prototype). So if you compare Model.prototype.save with document.save you'll see they are different functions.

This wouldn't be a problem if bluebird wrapped the Model.prototype.save function directly, but it actually wraps it with a different key appending "__beforePromisified__" to the method it calls.

Removing this line fixes the problem, but I understand why it's there.

The solution would be to make a custom promisification function which calls the original methods rather then the "beforePromisified" versions.

[inetfuture]

@yamadapc Sorry, I can't totally get what you're saying, have you figure out a fix for this?

[petkaantonov]

If a class adds methods in their constructor then you need to promisifyAll the created object after the constructor is called.

[inetfuture]

Ok, now I understand this part

The problem is hooks-js is added at the document constructor level (not at the model constructor or prototype). So if you compare Model.prototype.save with document.save you'll see they are different functions.

Still looking into this:

This wouldn't be a problem if bluebird wrapped the Model.prototype.save function directly, but it actually wraps it with a different key appending "beforePromisified" to the method it calls.

[inetfuture]

@petkaantonov promisifyAll the created object worked, but it's tedious...

[petkaantonov]

The reason for the key suffix is because an object might contain methods that end in Async but are not promisified. See https://gist.github.com/spion/a535623f4c1a779339b9

I suppose promisifyAll weakness could be changed from "not working with methods that are overriden in constructor" to "not working with objects that have methods ending in Async"

[yamadapc]

@petkaantonov I'd say this is a decent tradeoff. Perhaps adding a modified version of promisifyAll which called the original method inside the promisified one, or an optional argument to do that.

If you think that's a good idea, I'm wiling to write and PR it.

[yamadapc]

@inetfuture

This wouldn't be a problem if bluebird wrapped the Model.prototype.save function directly, but it actually wraps it with a different key appending "beforePromisified" to the method it calls.

What I meant was that the pre, post etc. hooks depend on the document.save method being called, rather than document.save__$before_promisified (I'm not sure of the suffix, but there's one - EDIT: the suffix is beforePromisified). Thus, when you call document.saveAsync, the hooks don't run because inside this function the method called was document.save__$before_promisified (which is equal to Model.prototype.save) rather than document.save.

Get it? I'm sorry... I know this is confusing and my english probably doesn't help much.

[petkaantonov]

@yamadapc I don't think the edge case is worth any additional code and especially API surface like optional param. I mean you need a class with methods named 'x' and 'xAsync' to cause a problem, and I really don't see that as realistic.

I will provide a fix tonight - this actually requires quite a bit of modifications in the ugliest part of the code base :P

[inetfuture]

@yamadapc Now I get it, thanks!
@petkaantonov I tried your fix, it works now, thanks!

[wprater]

@petkaantonov did you commit a change to fix this, or should we promisify on the instance as suggested ?

[yamadapc]

@wprater petkaantonov/bluebird@70f90c3 this should work now, as expected.

[SiZapPaaiGwat]

nice discussion, i have been working on this all day, and finally got a solution
@petkaantonov @yamadapc @inetfuture @wprater
see https://github.com/simongfxu/mongoomise/blob/master/src/main.js

[jeduan]

@simongfxu The latest bluebird improved this functionality and now all you have to do is Promise.promisifyAll(require('mongoose'))

https://github.com/petkaantonov/bluebird/blob/2.0/API.md#promisification

[yamadapc]

@jeduan As far as I'm aware, to promisify mongoose.Model "instance" methods you still have to promisify each model by hand.

This is due to the fact that the mongoose.model('Something', SomethingSchema); overwrites the prototypical save and remove instances. I haven't looked on what you improved on bluebird yet and need to do so, but one thing that does come to my mind about mongoose is that it doesn't (on 3.8, as of today, at least) export the Aggregate class like it exports its Query class, so you still have to monkey patch that with:

var Aggregate = require('mongoose/lib/aggregate');
var Promise = require('bluebird');
Promise.promisifyAll(Aggregate.prototype);

// etc.
SomeModel.aggregate(/* etc. */).execAsync();

If you're not manipulating instances of Aggregate that's irrelevant, though, since you could just use SomeModel.aggregateAsync. The hooks problem, however, remains, doesn't it? All models will have at least a validation hook, which is attached by mongoose automatically.

[SiZapPaaiGwat]

@jeduan thanks,that is so nice.

[SiZapPaaiGwat]

@jeduan i found that bluebird do not support promisify custom static methods,so i have improved my code to support it. what's more, it supports any other promise, current support list is bluebird,rsvp,q,when.
@yamadapc Aggregate is still a problem, hope mongoose would fix that.