Add .catch to mpromise

[hendricha]

Hey, I noticed that mpromise does not have a .catch, like the promise library we use (bluebird). It's a nice and simple feature, and makes one's promise chains a quite readable.

Note, that I did check, that .catch is not part of the Promise/A+ specs, however it does seem to be part of the ECMA Script 6 proposed specs. ( https://people.mozilla.org/~jorendorff/es6-draft.html#sec-promisecatch ) I saw the discussion here ( #1699 ), that mpromise will eventually be just a schim until node promises arrive, and I checked out unstable node, the Promise there did have a .catch.

[jonathanong]

+1

[esco]

👍

[ivogt]

+1

[remy]

Is monogoose.js even being maintained anymore, given that such an important part of the promise spec is missing and hasn't landed yet?

[hendricha]

Obviously it is maintained and developed: https://github.com/LearnBoost/mongoose/commits/master

[vkarpov15]

Sure, this should be reasonably easy to add. Unfortunately .catch() is a non-standard feature, so I haven't quite gotten around to adding it yet. I'd consider using Bluebird itself in mongoose, but its syntax is janky compared to mpromise, and I'm not really sure of the right solution to getting rid of the "print stack trace on uncaught reject()" behavior.

[remy]

I'm going by the ES6 spec, where catch() is a standard feature.

The issue is actually mPromise's lack of support for catch.

I was able to monkey patch mongoose using the following code (and note, that Promise is promise.js):

// patch mongoose to include .catch
mongoose.Query.prototype.exec = function exec(op, callback) {
  if ('function' === typeof op) {
    callback = op;
    op = null;
  } else if ('string' === typeof op) {
    this.op = op;
  }

  var promise = new Promise(function (resolve) {
    if (!this.op) {
      return resolve();
    }

    mongoose.Query.base.exec.call(this, op, function (error, document) {
      if (error) {
        throw error;
      }
      resolve(document);
    });

  }.bind(this));

  if (callback) {
    promise.then(function (result) {
      callback(null, result);
    }).catch(function (error) {
      callback(error, null);
    });
  }

  return promise;
};

I think my concern is that I expect in some unknown future to be able to remove all promise libraries in favour of native promises, and since native promises in V8 will include .catch it means expectations around mongoose's promises are going to break (as did my code!).

I'd propose moving away from mPromise and moving to something like promise.js in favour of avoiding any extra promise features that a library like Bluebird (or RSVP for instance) would add.

It's not a big change to the library (just promise.js in mongoose) but I'm not 100% sure of the impact on tests.

I did also notice weird behaviour in my code after I changed the internals of exec to stop using mPromise, code that should have broken started breaking (in that I was rejecting then resolving, and mPromise let it through, but promise.js did not - I'll try to get a small test case to show this if I have time tomorrow).

[vkarpov15]

Is there a good guide out there to all the functionality included in V8 native promises? I'd like to look into it, but I'm somewhat hesitant to base mongoose's promise API on V8 because V8 famously breaks its API pretty regularly.

[remy]

@vkarpov15 the link I used in my comment earlier is an export of the actual spec (just as HTML). I've been using @jakearchibald's article as a pretty good guide since it boils down the API: http://www.html5rocks.com/en/tutorials/es6/promises/#toc-api

[assaf]

Bluebird's reporting uncatched rejections is actually a powerful and useful feature (coupled with long stack traces). We use this feature to fail tests that don't have proper error handling.

You can just as easily disable it:

Bluebird.onPossiblyUnhandledRejection(function() { });

[zekenie]

👍

[DFectuoso]

+1

[Jokero]

+1

[markoknez]

+1

[loxy]

+1

[xpepermint]

+1

[AHgPuK]

+1

[dozoisch]

👍 Lack of catch in mongoose is a pain. Have to wrap every call with native Promises.

[vkarpov15]

See #2688

[akolpakov]

+1 for supporting native ES6 Promises

[mavdi]

+1

[royhowie]

Probably already mentioned elsewhere, but I just use bluebird's .promisifyAll feature:

let Promise = require("bluebird")
let mongoose = Promise.promisifyAll(require('mongoose'))

coupled with .execAsync. For example, in my queries.js file:

export let userQueries = {
    // lots of other queries
    findBy: {
        email: (email) => User.findOne({ email }).execAsync()
    }
}

Then, in passport.js, I can use .catch with the Mongoose query:

function login (req, email, password, done) {
    userQueries.findBy.email(email)
    /* lots of `then`s */
    }).catch((err) => {
        return done(null, false, req.flash('message', err.message || err))
    })
}

[JeffAshton]

+1

[dgaubert]

+1

[paambaati]

+1. This is a pretty important feature, and for everyone jumping on ES6, this completely breaks immersion. I have code all over the place using .then(...).catch(...) except for Mongoose!

@royhowie @remy @assaf Is there a possible hit to performance when wrapping mongoose with Bluebird's promisifyAll?

[vkarpov15]

See #2688, 4.1.0 supports the below

require('mongoose').Promise = require('bluebird');
require('mongoose').Promise = global.Promise;
require('mongoose').Promise = require('q').Promise;

or whichever ES6 promise constructor you prefer.

[paambaati]

@vkarpov15 A-ha! Thanks a bunch. This is awesome.

[zekenie]

Hi @vkarpov15 just tried it with 4.1.0 and it doesn't seem to work for me. Throws an error that says undefined is not a function when I use catch

[paambaati]

@zekenie Works for me, actually. Here's how I do it now on 4.1.0 -

const mongoose = require('mongoose');
mongoose.Promise = require('bluebird');

const UserSchema = new mongoose.Schema({...});

function createUser(userData) {
    const newUser = new User(userData);
    return newUser.save().then(function (savedUser) {
        return savedUser;
    }).catch(function (error) {
        console.log('something terrible went wrong!');
        throw error;
    });
}

Can you show us some code to help debug your problem?

[vkarpov15]

@zekenie are you actually setting require('mongoose').Promise? To avoid backwards breaking changes, right now mongoose still uses mpromise by default, but you can set require('mongoose').Promise to whatever ES6 promise constructor you want to use, including ones that support .catch() like native ES6 promises in Node >= 0.12, bluebird, Q.Promise, etc.

[adamreisnz]

I think for the next major version, maybe switch to native promise implementation by default?

[vkarpov15]

@adambuczynski yep we won't support mpromise at all in 5.0, will use native promises by default but support switching to whichever ES2015 promise constructor lib you want: https://github.com/Automattic/mongoose/wiki/5.0-Deprecation-Warnings

[adamreisnz]

👍

[Jeff-Lewis]

@vkarpov15 without any hard stats, my gut says more and more people are using Bluebird. Strongloop is converting everything to use Bluebird in 3.0 of loopback. Do you think we can emphasize tests cases and code coverage for promises with bluebird in v5 even though v5 will technically be only supporting ES2015 promises?

[vkarpov15]

@Jeff-Lewis yeah I see a lot of people going to bluebird as well. On the other hand, with ideas like co and async/await, I haven't cared which promise library I'm using since mid-2015, and minimal promise libs like https://www.npmjs.com/package/es6-promise are seeing good adoption too.

v5 will support bluebird in the same way that >= 4.1 supports bluebird. There's really not much more we can do short of switching over to bluebird by default, and bluebird's too quirky and bloated for my tastes.

[adamreisnz]

@vkarpov15 what quirks do you think it has? I was under the impression that it was quite a decent promise library, after having a look at it last week (I was using native promises and Angular's $q before that), and I was considering using it for both backend and frontend.

But one big thing missing from native promises for example, is that there is no .finally in the spec yet. I think that's a big miss.

Also, Bluebird didn't seem too bloated, and even if it were, the performance gains over other libraries were quite decent.

[vkarpov15]

Honestly I've never been a big fan of .catch() and .finally(). It is true that syntactic sugar is more important than most devs care to admit, but IMO those two functions are too trivial to lose sleep over.

My beef with bluebird is:

1. If I had a case where I really cared about getting every last drop of CPU performance, I wouldn't use promises. Frankly, I wouldn't even use node. I've yet to see a case where one promise lib has enough of a performance win over another promise lib to register on my care-o-meter.
2. API is far too big. Smaller API is better, especially since 99% of what I want to do with promises is easily achieved with .then() and .all().
3. Weird bugs: Do not override resolve and reject functions with null mongoosejs/mquery#66

Contrast this with http://npmjs.org/package/es6-promise - nice neat minimal implementation that you can grok in 15 minutes https://github.com/jakearchibald/es6-promise/blob/master/lib/es6-promise/promise.js .

[adamreisnz]

Fair points, thanks for your feedback.

While you can pass in a second function to .then() and use that as an error handler, I do find that code is easier to read with a dedicated .catch() block. As for .finally(), that just helps with maintaining DRY principles. Often (particularily in front-end applications) after promises are returned there needs to be some clean up happening, like a button being re-enabled or a spinner turning off, and that needs to happen regardless of whether the promise has resolved or rejected. So in order to not repeat code, the .finally() block is ideal.

But I agree, in Node apps, there is much less need for a .finally().

[Jeff-Lewis]

@vkarpov15 Regarding 3) Weird bugs: mongoosejs/mquery#66, I added a comment to it.