Sharing promises between modules vs having multiple promises

Question

I am implementing Node.js logic with controllers and repository using Kris Kowal's Q library. I have a feeling that the way I use promises in the example below is not correct. But I can't find any guidance on proper pattern how to use promises through multiple layers or functions.

Am I doing it right? What's the proper way implementing this logic?

// Module: Repository

exports.findOne = function (query) {
    var deferred = Q.defer();

    db.query(query, function (err, data) {
        if (err) {
            deferred.reject(err);
        } else {
            deferred.resolve(data);
        }
    });

    return deferred.promise;
};

// Module: User

var isEmailAvailable = function (value) {
    var deferred = Q.defer();

    Repository.findOne({email: value})
        .then(function (user) {
            if (user) {
                if (self.id === user.id) {
                    deferred.resolve(true);
                }
                else {
                    deferred.resolve(false);
                }
            } else {
                deferred.resolve(true);
            }
        })
        .fail(function (err) {
            deferred.reject(err);
        });

    return deferred.promise;
};

this.save = function () {
    var deferred = Q.defer();

    isEmailAvailable(this.email)
        .then(function (result) {
            if (result) {
                Repository.upsert(self)
                    .then(function (user) {
                        deferred.resolve(user); //--- Yey!!!
                    }).fail(function () {
                        deferred.reject('Account save error')
                    });
            } else {
                deferred.reject('The email is already in use');
            }
        }).fail(function () {
            deferred.reject('Account validation error')
        });

    return deferred.promise;
};

score 3 · Accepted Answer · edited May 23 '17 at 12:12

I have a feeling that the way I use promises is not correct

Yes, you use the deferred antipattern a lot. Promises do chain, which means that you simply can return values and even promises from your then callback, and the .then() call will yield a promise for those return values. And when you throw, the result promise will be rejected.

Your usage of deferreds in repository module is correct, as the db.query api needs to be promisified. But the user module can be shrank heavily, you don't need to use any deferreds when you already have promises.

function isEmailAvailable(value) {
    return Repository.findOne({email: value})
    .then(function (user) {
        return !user || self.id === user.id;
    });
}

this.save = function() {
    return isEmailAvailable(this.email)
    .then(function (result) {
        if (result) {
            return Repository.upsert(self)
            .then(null, function(err) {
                throw new Error('Account save error');
            });
        } else {
            throw new Error('The email is already in use');
        }
    }, function(err) {
        throw new Error('Account validation error');
    });
};

Alternatively, as suggested by @Roamer-1888, using exceptions for control flow:

function isEmailAvailable(value) {
    return Repository.findOne({email: value})
    .then(function (user) {
        if (user && self.id !== user.id)
            throw new Error('The email is already in use');
    }, function(err) {
        throw new Error('Account validation error');
    });
}

this.save = function() {
    return isEmailAvailable(this.email)
    .then(function () {
        return Repository.upsert(self)
        .then(null, function(err) {
            throw new Error('Account save error');
        });
    });
};

If `isEmailAvailable()` was to employ the success/error paths instead of sending true/false down the success path, then I think you will find that `this.save()` also becomes dramatically simpler. — Roamer-1888, Mar 04 '15 at 04:04
@Roamer-1888: Have a look at my edit. Is that what you meant? — Bergi, Mar 04 '15 at 14:35
Bergi, yes, essentially so. I would probably leave out the 'Account validation error' throw as it would potentially mask better error messages thrown by `Repository.findOne()`. — Roamer-1888, Mar 04 '15 at 15:21
@Roamer-1888: Yeah, maybe that would be better, but I wanted to stay equivalent in behaviour to OPs function, where it is *supposed* to mask errors thrown by `findOne()`. — Bergi, Mar 04 '15 at 15:23
Yup, cool. Armed with your explanation, the OP can choose which way to go. — Roamer-1888, Mar 04 '15 at 15:27
Thank you @Roamer-1888 and @Bergi! This is exactly what I needed! That last "aha! gotcha!" piece of a puzzle! — Sergey, Mar 04 '15 at 16:45

Sharing promises between modules vs having multiple promises

1 Answers1