0

I'm trying to figure out the proper way to do promisification - and more specifically using bluebird. I've come up with some code that makes use of new Promises:

function createUser(data) {
  return new Promise((resolve, reject) => {

    User.createAsync(user)
        .then((doc) => {
            resolve(doc);
        })
        .catch((err) => {
            reject(err);
        });
  });
}

function create(data) {
   return new Promise((resolve, reject) => {

    User.findOneAsync({username: data.username})
        .then((user) => {
            if (user) {
                 resolve(`Username ${data.username} is already taken`);
            } else {
                createUser(data)
                    .then((user) => {
                        resolve(user);
                    })
            }
        })
        .catch((err) => {
            reject(err);
        });
   })
}

But I feel like I'm not getting much out of bluebird this way and after browsing through the documentation it seems like this is something of an anti-pattern that should be avoided. How would I go about making for example this flow more bluebird style or better promisified in general?

Kim Lindqvist
  • 363
  • 4
  • 15
  • Avoid the [`Promise` constructor antipattern](http://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! – Bergi Mar 15 '17 at 19:56

2 Answers2

2

If User.createAsync and User.findOneAsync return promises, you don't need to wrap them in new promises. Just return the promises returned by your functions:

function createUser(data) {
  return User.createAsync(data);
}

function create(data) {
  return User.findOneAsync({username: data.username})
    .then((user) => {
      if (user) {
        throw new Error(`Username ${data.username} is already taken`);
      } else {
        return createUser(data);
      }
    });
}
SimpleJ
  • 13,812
  • 13
  • 53
  • 93
  • Note that this can be further improved with `async` functions, which are supported in the latest Node versions. You aren't using any Bluebird specific features here. – Madara's Ghost Mar 15 '17 at 19:51
  • Thank you, I didn't quite get the point of promisifyAll until now. I guess you are right, I should probably look into the async/await structure instead. – Kim Lindqvist Mar 15 '17 at 19:58
  • @MadaraUchiha That's not necessarily an improvement. It's just a different syntax for doing the same thing. I prefer a pure promise solution, as it's supported everywhere promises are available (which is anywhere if OP is using Bluebird). – SimpleJ Mar 15 '17 at 20:01
1

Adding onto SimpleJ's answer, you can do the following with async functions to get even better code readability:

function createUser(data) {
  return User.createAsync(data);
}

// async functions!
//                     destructuring!
async function create({username}) {
  //                                   short literals!
  const user = await User.findOneAsync({username});
  if (user) { throw new Error(`Username ${username} is already taken`); }
  return createUser(data);
}

This gives you a very flat, almost synchronous looking code. Note the await before User.findOneAsync() for waiting on the Promise.

The code above is equivalent to the other answer's. Async functions require node 7.6 or higher.

Madara's Ghost
  • 172,118
  • 50
  • 264
  • 308