2

Recently I started using pg-promise with bluebird library. I have always been nesting callback and handling err in each callback. I find that the catch statement in promise looks really neat. I am not sure if it possible to turn this code to promise base?

username = username.toUpperCase();
let text = "SELECT * FROM users WHERE username = $1";
let values = [username];
database.one(text, values).then(function (userObject) {
  // ANY WAY TO TURN this nested bycrypt into promise chain?
  bcrypt.compare(password, userObject.password, function (err, same) {
    if (err) {
      return next(err, null);
    }
    if (!same) {
      return next(new Error("Password mismatched!"), null);
    }
    const serializeObject = {_id: userObject._id};
    return next(null, serializeObject);
  });
}).catch(function (err) {
  return next(err, null);
});
Zanko
  • 4,298
  • 4
  • 31
  • 54
  • bluebird has a [promisify function](http://bluebirdjs.com/docs/api/promise.promisify.html) which would promisify `bcrypt.compare` for you – Jaromanda X Feb 01 '17 at 14:41
  • @JaromandaX yea I was reading into that, so all I have to do is add "Async" keyword to the end of my function? – Zanko Feb 01 '17 at 14:42
  • yeah, I guess, sort of, once it's promisified - read the docs :p – Jaromanda X Feb 01 '17 at 14:43
  • `I am not sure if it possible to turn this code to promise base` any code can be turned into promises, and the `promisify` solution for your `bcrypt` module is the easiest one. – vitaly-t Feb 01 '17 at 15:33

2 Answers2

4

I imagine using bluebirds promisify, you would promisify bcrypt.compare like so (you don't HAVE to use the Async part of the name)

let compareAsync = Promise.promisify(bcrypt.compare);

because userObject in the first .then needs to be used in the second .then, you can't simply chain the .then's by returning compareAsync, because then the next .then wont have access to userObject

one fix, is to use a variable that will be in scope for both .then's (but ugh)

username = username.toUpperCase();
let text = "SELECT * FROM users WHERE username = $1";
let values = [username];
let uo; //hacky the outer scoped variable
database.one(text, values).then(function (userObject) {
    uo = userObject;
    return compareAsync(password, userObject.password);
}).then(function(same) {
    if (!same) {
        throw new Error("Password mismatched!");
    }
    const serializeObject = {_id: uo._id};
    return next(null, serializeObject);
}).catch(function (err) {
    return next(err, null);
});

another (in my opinion cleaner) option is a nested .then

username = username.toUpperCase();
let text = "SELECT * FROM users WHERE username = $1";
let values = [username];
database.one(text, values).then(function (userObject) {
    return compareAsync(password, userObject.password)
    // [optional] following three lines to generate a "nicer" error for compare failure
    .catch(function(err) {
        throw "bcrypt.compare failed";
    })
    // nested .then to pass on the userObject and same at the same time
    .then(function (same) {
        return { same: same, userObject: userObject };
    });
}).then(function (result) {
    let same = result.same,
        userObject = result.userObject;

    if (!same) {
        throw new Error("Password mismatched!");
    }
    let serializeObject = { _id: userObject._id };
    return next(null, serializeObject);
}).catch(function (err) {
    return next(err, null);
});

NOTE: bluebird has a promisifyAll function ... that promisifies functions in an object, and adds (by default) the Async postfix to the function name - I believe you can decide on a different postfix name, but the documentation will tell you more

when promisifying a single function, you declare the name yourself - the above could've easily been

let trumpIsBigly = Promise.promisify(bcrypt.compare);

then you would just use trumpIsBigly where the code has compareAsync

One last possibility

A hand rolled promisified compareAsync (lifted mostly from vitaly-t's answer but with additions)

function compareAsync(password1, password2, inValue) {
    return new Promise(function (resolve, reject) {
        bcrypt.compare(password1, password2, function (err, same) {
            err = err || (!same && new Error("Password mismatched!"));
            if (err) {
                reject(err);
            } else {
                resolve(inValue);
            }
        });

    });
}

Now compareAsync will resolve to the incoming value inValue only if there's no error, AND same is true

username = username.toUpperCase();
let text = "SELECT * FROM users WHERE username = $1";
let values = [username];
database.one(text, values).then(function (userObject) {
    return compareAsync(password, userObject.password, userObject)
}).then(function (userObject) {
    let serializeObject = { _id: userObject._id };
    return next(null, serializeObject);
}).catch(function (err) {
    return next(err, null);
});

Which makes the "chain" very simple!

Jaromanda X
  • 53,868
  • 5
  • 73
  • 87
  • truly answer most of my doubts! The promisify function seems really complicated. Sorry if these are absurd questions, but what if the library already have Async postfix in it? And how stable is this conversion really? Will it break for some function? – Zanko Feb 01 '17 at 14:55
  • `The promisify function seems really complicated` - really? pass in the function name ...get back a function that returns a promise ... how more simple do you need it? – Jaromanda X Feb 01 '17 at 14:59
  • sorry:P Really new to Promise. Another simple question, for `.catch(function(err))` will the `err` belongs to the first `err` that occurs? I am currently reading up on promise to understand it more : ) – Zanko Feb 01 '17 at 15:01
  • yes (and no) ... in this code, that catch will be hit by the first error, if that is in the database.one, then none of the .then codes will run – Jaromanda X Feb 01 '17 at 15:03
  • The promisify doesn't say anything about passing in a function that is already promised-based. So before I promisify any function, I have to make sure that it is a normal callback function? By reading the source code etc. – Zanko Feb 01 '17 at 15:06
  • Bluebird's `.promisify()` works with any async function that uses the node.js async calling style where there's a callback as the last argument and that callback takes two parameters so it will be called like this `callback(err, data)`. Yes, you have to make sure the function fits that criteria before using it with `.promisify()`. You shouldn't need to look at the function's code to determine this because it should be the same info that is needed just to use the function yourself so it should be in the doc or comments. – jfriend00 Feb 01 '17 at 15:24
  • @JaromandaX in your nested example, should I provide another catch for that nested promise? This way I can catch the compareAsync error? – Zanko Feb 01 '17 at 21:52
  • well, no unless you throw an error at the end of the catch, otherwise the last then will be executed, and you don't want that - the error should be processed by the last catch anyway – Jaromanda X Feb 01 '17 at 22:03
  • I've added a `.catch` for compareAsync - note WHERE I put it and that it **throws** so that neither of the following `.then` will get called – Jaromanda X Feb 01 '17 at 22:15
  • I have trouble visualizing this : ( As far as I know there are two chains? One is the main chain, another is a nested promise chain. The main chain has catch statement. How will the main chain get the error from compareAsync if they are part of different chain? – Zanko Feb 01 '17 at 22:15
  • yeah ... think of the nested chain as a promise, that resolves or rejects ... returning that in the "outer" `then` means the outer `then` will take on the result of the nested `then`, be it resolved or rejected ... – Jaromanda X Feb 01 '17 at 22:18
  • Just to clarify, So in the original example without the `catch` in the nested chain, can the outer catch get the error from compareAsync? right now my understanding is that it cant. – Zanko Feb 01 '17 at 22:20
  • as I said, the promise returned in the first `.then` will "take on: the result of the inner promise, if that is rejected (if `compareAsync` rejects) then the outer `.then` will reject with the rejection reason of `compareAsync`, which will be caught by the final `.catch` – Jaromanda X Feb 01 '17 at 22:36
  • Promise is so neat but it is so tricky when I have to depend on previous value! It is not "neat" haha – Zanko Feb 01 '17 at 23:04
  • there are other ways to handle it ... hang on, I'll add one more solution using a hand written promisified `bcrypt.compare` function :p – Jaromanda X Feb 01 '17 at 23:05
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/134656/discussion-between-zanko-and-jaromanda-x). – Zanko Feb 02 '17 at 07:08
1

This is to extend on @Jaromanda's answer, in case you use only that one function, and just want to see how to promisify it manually.

function samePassword(password1, password2) {
    return new Promise(function (resolve, reject) {
        bcrypt.compare(password1, password2, (err, same) => {
            err = err || (!same && new Error("Password mismatched!"));
            if (err) {
                reject(err);
            } else {
                resolve();
            }
        });

    });
}

db.one(text, values)
    .then(userObject => {
        return samePassword(password, userObject.password);
    })
    .catch(error => {
        return next(error, null);
    });

Other than that, the promisify approach is the way to go. But it is always good to understand what it effectively does ;)

vitaly-t
  • 24,279
  • 15
  • 116
  • 138
  • Thank you this give me more insight into promisify. In one of your documentation https://github.com/vitaly-t/pg-promise/wiki/Common-Mistakes, you mention that we should avoid Promise.all and commented with ` // doesn't settle queries;` Can you provide more explanation on why it doesnt settle? Ultimately isnt query a promise function and should work with Promise.all? – Zanko Feb 01 '17 at 16:17
  • It is not about promise function not working, it is about `Promise.all` not guaranting to settle promises, as per its protocol, i.e. `Promise.all` only guarantees to settle all promises when the method itself resolves. – vitaly-t Feb 01 '17 at 16:22
  • I see, is Promise.all and Promise chaining the same thing? I am going to read Promise in full detail but was hoping for some quick answer : ) – Zanko Feb 01 '17 at 17:08
  • Promise.all is a way to "wait" on multiple promises to resolve ... but if just one rejects, Promise.all will reject – Jaromanda X Feb 01 '17 at 22:06
  • 1
    @vitaly-t - good job, I was considering showing how to promisify that function too, but my answer was already quite verbose :p – Jaromanda X Feb 01 '17 at 22:34