What Is the Explicit Promise Construction Antipattern and How to Avoid It

What is the explicit promise construction antipattern and how do I avoid it?

The deferred antipattern (now explicit-construction anti-pattern) coined by Esailija is a common anti-pattern people who are new to promises make, I've made it myself when I first used promises. The problem with the above code is that is fails to utilize the fact that promises chain.

Promises can chain with .then and you can return promises directly. Your code in getStuffDone can be rewritten as:

function getStuffDone(param){
    return myPromiseFn(param+1); // much nicer, right?
}

Promises are all about making asynchronous code more readable and behave like synchronous code without hiding that fact. Promises represent an abstraction over a value of one time operation, they abstract the notion of a statement or expression in a programming language.

You should only use deferred objects when you are converting an API to promises and can't do it automatically, or when you're writing aggregation functions that are easier expressed this way.

Quoting Esailija:

This is the most common anti-pattern. It is easy to fall into this when you don't really understand promises and think of them as glorified event emitters or callback utility. Let's recap: promises are about making asynchronous code retain most of the lost properties of synchronous code such as flat indentation and one exception channel.

Understanding explicit promise construction anti pattern

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.

How to avoid the explicit Promise construction antipattern for this concurrent timer that should reset when the promise completes?

See What is the explicit promise construction antipattern and how do I avoid it? for details. Generally speaking, you (mostly) do not need new Promise when you already have a promise. You can just reuse the existing one (and chain it if necessary).

Your code can be simplied.

Remove the unnecessary new Promise
Reuse and return the existing promise
Remove code duplication and use Promise#finally instead

{
  waiting: false,

  handleWaiting(promise, timeout) {
    let loadingStarted = false;

    const timeoutInstance = setTimeout(() => {
      loadingStarted = true;
      this.waiting = true;
    }, timeout);

    return promise.finally(() => {
      if (loadingStarted) {
        this.waiting = false;
      }
      clearTimeout(timeoutInstance);
    });
  },

  async searchForTerm(term) {
    this.results = await this.handleWaiting(this.$wire.query(term), 500);
    // do something with the results...
  },
}

And you can probably get rid of loadingStarted as well. Is there a reason why you have two state variables for that? You never reset loadingStarted anyway.

{
  waiting: false,

  handleWaiting(promise, timeout) {
    const timeoutInstance = setTimeout(() => {
      this.waiting = true;
    }, timeout);
    return promise.finally(() => {
      this.waiting = false;
      clearTimeout(timeoutInstance);
    });
  },

  async searchForTerm(term) {
    this.results = await this.handleWaiting(this.$wire.query(term), 500);
    // do something with the results...
  },
}

how to avoid the explicit-construction anti-pattern and still keep indentation/callbacks low

  downloadAndAssemble = async () =>  {
    if (!(await checkAvailableDiskSpace())) {
      return false;
    }

     try {
       // Download all
       let files  = await this.downloadAll();

       // Assemble File from downloaded snippets.
       const assembledFile = await buildFile(files);

      // Save assembled file.
      return true;
    } finally {
       const dirExists = await fs.exists(tmpFolderPath);
       if (dirExists) await fs.unlink(tmpFolderPath);
    }
};

If you call an async function, it will implicitly create a promise under the hood for you, that resolves if you return and rejects if you throw, so there is no need to create and manage a promise "manually".

.catch(error => reject(error)) makes little sense as awaiting a promise automatically lets the errors bubble up. In your code this bypasses the try { ... } catch { ... } which is probably not wanted.

The same applies to } catch (err) { reject(err); }, await is all you need.

Avoid Promise constructor antipattern when logging

Just return or rethrow the original value in your logging callbacks, and then() will give you back a promise of the same thing.

The whole point of promises is that they're easy to chain like that.

return myPromiseFn(param+1)
     .then(function(val) {
         console.log("getStuffDone executing");
         return val;
     }).catch(function(err) {
         console.log("getStuffDone error");
         throw err;
     });

Handle all promise rejections in nested promises?

After ggorlen's excellent advice, I refactored to this much cleaner (and test-passing) code:

async function getTransformedData(url,page) {

    try {

        const response = await fetch(url);

        if (response.ok) {

            const records = await response.json();

            const transformedData = transformData(records,page);

            return(transformedData);

        } else {

            throw new Error("failed");    

        }

    }

    catch(error) {

        console.log(error);

    }
    
    // getTransformedData
}


function retrieve({page = 1, colors = ["red", "brown", "blue", "yellow", "green"]} = {}) { 

    // limit set to 11 so we can determine nextPage
    const limit = "11";

    const offset = "" + ((page * 10) - 10);

    let colorArgs = "";
    colors.forEach((color, index) => {
        colorArgs = colorArgs + "&color[]=" + color; 
    });

    const requestQuery = `limit=${limit}&offset=${offset}${colorArgs}`;
    
    const requestURL = new URI(window.path);
    requestURL.query(requestQuery);

    return getTransformedData(requestURL.toString(),page);

};

Reason to return promise from another promise?

This whole thing can be replaced with:

function someFunc() {
    return someModule.someFunction(input);
}

It is considered an anti-pattern to create your own promise which you then resolve and reject when you could just use a promise that is already there.

It is considered an anti-pattern to have do-nothing success and error handlers that just do basically what the default behavior would already be.

Really, the only thing that your code snippet does differently than just returning the promise directly is that it makes sure the returned promise is a Q promise, not whatever kind of promise .someFunction() returns. But, if that is a requirement, then it could be cast to a Q promise more directly without all the extra code.

Here's a discussion of promise anti-patterns to be avoided because there's a better way.

How do I convert this function to not use the Promise constructor antipattern

If the db.users.count() function is an asynchronous operation, then what you've written is not an anti pattern. The "anti pattern" comes in when the method is synchronous, and a promise is returned for the sake of using the Promise Pattern.

The key here is from a quote from another Stack Overflow question linked to in the question you reference:

... promises are about making asynchronous code retain most of the lost properties of synchronous code such as flat indentation and one exception channel.

(emphasis, mine)

If the code is not asynchronous, then returning a promise is an anti pattern.

As for returning new Promise(...) being an anti pattern? If that's the case then returning new Anything() is an anti pattern. Using the Promise constructor is fine.