Understanding explicit promise construction anti pattern

Question

CertainPerformance highlighted in my previous post advised me to avoid the explicit Promise construction antipattern with reference to to following question in stackoverflow

Frankly, Speaking, I am new to JS and node and I haven't used promise a lot. I went and read those article but either I was unable to comprehend or unable to relate or maybe somewhere my understanding of promises have been vague/wrong all together

So I decided to ask this question in a new thread and seek for help.

So what am I doing and why am I doing it

I am creating helper/common function which I could use to keep my code tidy and if in case I want to change anything inside function at anytime, I don't have to manually change every function.

So these are the functions I have made

//Find user by email Address 
const findUserByEmail = (emailAddress) => {
    return new Promise((resolve, reject) => {
     User.findOne({email: emailAddress}).then(response => {
        resolve(res)
      }).catch(error => {
        reject("Error in findUserByEmail", error);
      })
    })
}

//Create User 
const createNewUser = (newUserDetails) => {
    return new Promise((resolve, reject) => {
      new User({
         fullName: newUserDetails.fullName,
         email: newUserDetails.email,
         image: newUserDetails.image,
         gender: newUserDetails.gender,
         age: newUserDetails.age
      }).save().then((response) => {
          resolve(response)
      }).catch((error) => {
          reject("Problem in Creating New User", error)
      })
    })
}

Question 1

Now, I am assuming CertainPerformance said the excessive use of promises because I am creating new promise return new Promise((resolve, reject) => { when I am already using promises with mongoose User.findOne({email: emailAddress}).then(response => { ?

But the reason for me to create those promise was, when I call these helper function from anywhere in my app after importing

const { findUserByEmail } = require("./my_db_query");

I would probably want it return a response or throw an error in case of error

findUserByEmail("test@example.com").then(/*...*/).catch(/*...*/);

If I change my above code snippet without adding new promise

 function findUserByEmail  (email) {
       return User.findOne({email: email}).then(currentUser => currentUser).catch(error => error)
    } 

Question 2

Then I won't probably be able to .then and .catch in findUserByEmail("test@example.com")?

And In API route of App, where I would be calling the findUserByEmail("test@example.com") function, I would want to do something else if there is an error (which would be different for different case and hence I cannot use it in my helper function).

Question 3

Does, it make sense now for doing return new Promise((resolve, reject) => { instead of doing just one return User.findOne( or am I missing something?

Answer 1

Because .findOne already returns a Promise, there's no need to construct a new one with new Promise - instead, just chain onto the existing Promise chain with .then and .catch as needed. Such Promise chains can have any number of .thens and .catchs - just because you consume a Promise with one .then doesn't prevent you from using the same resolve value elsewhere. To illustrate:

makePromise()
  .then((result) => {
    console.log(result);
    // Returning inside a `.then` will pass along the value to the next `.then`:
    return result;
  })
  .then((result) => {
    // this `result` will be the same as the one above
  });

In other words - there's no need to construct a new Promise every time you want to be able to use another .then. So:

Then I won't probably be able to .then and .catch in findUserByEmail("test@example.com")

isn't correct - you can indeed chain onto the end of an existing Promise with as many .thens and .catches as you want.

Note that a .then which only returns its parameter and does nothing else (such as .then(currentUser => currentUser)) is superfluous - it won't do anything at all. Also note that a .catch will catch Promise rejections and resolve to a resolved Promise. So if you do

function findUserByEmail(email) {
  return User.findOne({email: email})
    .then(currentUser => currentUser)
    .catch(error => error)
}

that catch means that callers of findUserByEmail will not be able to catch errors, because any possible errors were caught in findUserByEmail's catch. Usually, it's a good idea to allow errors to percolate up to the caller of the function, that way you could, for example:

someFunctionThatReturnsPromise('foobar')
  .then((result) => {
    // everything is normal, send the result
    res.send(result);
  })
  .catch((err) => {
    // there was an error, set response status code to 500:
    res.status(500).send('there was an error');
  })

So, unless your findUserByEmail or createNewUser helper functions need to do something specific when there's an error, it would probably be best just to return the Promise alone:

const findUserByEmail = email => User.findOne(email);
const createNewUser = newUserDetails => new User(newUserDetails).save();

If your helper functions do need to do something when there's an error, then to make sure that the error gets passed along properly to the caller of the function, I'd recommend either throwing the error inside the catch:

const findUserByEmail = email => User.findOne(email)
  .catch((err) => {
    // error handling - save error text somewhere, do a console.log, etc
    throw err;
  });

so that you can catch when something else calls findUserByEmail. Otherwise, if you do something like

const findUserByEmail = email => User.findOne(email)
  .catch((err) => {
    // do something with err
    return err;
  });

then the caller of findUserByEmail will have to check inside the .then if the result is actually an error, which is weird:

findUserByEmail('foo@bar.com')
  .then((result) => {
    if (result instanceof Error) {
      // do something
    } else {
      // No errors
    }
  });

Better to throw the error in findUserByEmail's catch, so that the consumer of findUserByEmail can also .catch.

Answer 2

It never makes sense to create a promise with promise constructor when there's existing promise, that's why it's called promise construction antipattern.

This is a mistake, reject("Error in findUserByEmail", error). reject accepts only 1 argument, which is rejection reason. error will be ignored. It's conventionanl for an error to be Error object and not a string.

The function may be refactored to:

   const findUserByEmail = (emailAddress) => {
     return User.findOne({email: emailAddress})
     .then(response => response) // noop
     .catch(error => {
        const readableError = new Error('Error in findUserByEmail');
        readableError.originalError = error;
        throw readableError;
      });
    })
  }

etc.

Antipatterns don't necessary result in bad performance but they result in code smell. They make the code harder to read, maintain and test, also show that a developer may have a poor understanding of the subject.

Promise constructor has some insignificant performance impact. It introduces another level of nesting and contributes to callback hell - promises are supposed to help avoiding it.

If I change my above code snippet without adding new promise <...> Then I won't probably be able to .then and .catch in findUserByEmail("test@example.com")?

No, a promise can be chained with then(...) and catch(...) (which is syntactic sugar for then(null, ...)) as many times as needed, that's the strong side of the pattern. Notice that catch(err => { return err }) and catch(err => { throw err }) is not the same thing, the former catches an error, the latter rethrows it.