Problem understanding and implementing how promises and asynchronous things work with loops

Question

I'm learning node and I have a piece of code that queries data in the db that i'd like to pass into a callback function. But I'd like to only return unique members.

This means that I have to go through the result of the query, and push the unique members into an array, then pass the array into the cb function.

The problem is, the input is also an array that i build the query off. So there's already a loop there.

I've taken a look at some previous SO threads with promises, await, etc. But I'm having some trouble implementing them with loops like in here.

static getDestination(envName, cb){
    var ans = new Array();
    var promises;
    DataBase.initDB((db) => {
        for (var i = 0; i < envName.length; i++){
            db.collection('Environment').find({'environmentName' : envName[i]}, (err, data) => {
                if (err) {
                    console.log('error getting data in getDestination()');
                    return;
                }
                data.toArray((err, content) => {
                    if (err) {
                        console.log('error converting cursor into array');
                        return;
                    }
                    else {
                        for (const doc of content){
                            if (!ans.includes(doc.destination)){
                                ans.push(doc.destination);
                            }
                            //cb(ans); 
                        }
                    }
                }); 
            });
        }
    })
    cb(ans); 
}

Right now, the callback is just a console.log(ans), and it's printing out an empty array [].

If I call the callback in the innermost loop, it returns the proper members, but it gets called as the loop goes like:

[]
[ 'DEV' ]
[ 'DEV' ]
[ 'DEV', 'QA' ]

I'd like to get only the last line of the output. A thorough explanation would be amazing too!

EDIT: To my understanding, I should be creating an array of promises, and at every iteration of the loop, I should be creating a new promise with the operation as a parameter. Then at the end of everything, I can call

Promise.all(promises).then(cb(ans));

But I'm having trouble understanding which transaction should I create the promises on. Since if I create the promise in the innermost loop, the cb outside of the loop gets called before the first was even created.

EDIT:

exports.initDB = (cb) => {
console.log('Connecting...');
const uri = <mongo connection uri here>;
const client = new MongoClient(uri, { useNewUrlParser: true });
client.connect(err => {
    if (err) {
        console.log('eror connecting to db');
        console.log(err);
        return;
    }
    cb(client.db(process.env.DB_NAME));
});

}

Answer 1

Callbacks

Here's how you'd create your array of promises and use it to aggregate ans then invoke your cb() once. I also suggest using the Node.js callback convention of cb(error, result) rather than cb(result), since there are multiple steps that can error, and the caller of getDestination() should be notified both when the query succeeds or when it fails.

static getDestination (envName, cb) {
  DataBase.initDB(db => {
    const envCollection = db.collection('Environment');
    const promises = envName.map(value => new Promise((resolve, reject) => {
      envCollection.find({ environmentName: value }, (error, data) => {
        if (error) return reject(error);

        data.toArray((error, content) => {
          if (error) reject(error);
          else resolve(content);
        });
      });
    }));

    Promise.all(promises).then(contents => {
      const ans = contents.reduce(
        (ans, content) => content.reduce(
          (ans, doc) => ans.add(doc.destination),
          ans
        ),
        new Set()
      );

      cb(null, [...ans]);
    }).catch(error => {
      cb(error);
    });
  });
}

Notice that each new Promise() is constructed synchronously, and resolve() or reject() is called asynchronously. This is necessary because your database queries do not already return a Promise.

If you use an API that does return promises, you should avoid the explicit promise construction antipattern and chain from the existing promises instead.

I also optimized your aggregation of ans by using a Set instead of an array, so we can actually omit checking for existing values since a set only keeps one of each unique value unlike an array.

Promise chains

If you want your getDestination() function to return a promise instead of accepting a callback, you can rewrite it like this:

static getDestination (envName) {
  return new Promise(resolve => {
    DataBase.initDB(resolve);
  }).then(db => {
    const envCollection = db.collection('Environment');
    const promises = envName.map(value => new Promise((resolve, reject) => {
      envCollection.find({ environmentName: value }, (error, data) => {
        if (error) return reject(error);

        data.toArray((error, content) => {
          if (error) reject(error);
          else resolve(content);
        });
      });
    }));

    return Promise.all(promises);
  }).then(contents => {
    const ans = contents.reduce(
      (ans, content) => content.reduce(
        (ans, doc) => ans.add(doc.destination),
        ans
      ),
      new Set()
    );

    return [...ans];
  });
}

The key differences are:

• wrap DataBase.initDB() in another Promise because the result depends on db
• return the promise chain from getDestination()
• return [...ans] from .then() instead of passing it to a callback
• remove .catch() and let the caller handle any errors

Notice that because we return Promises.all(promises) from a .then(), we can get contents from the outer promise instead of chaining to the inner Promise.all(). This is one of the major benefits that promises offer to escape from callback hell.

Async / Await

Lastly, if you want to use async / await instead of .then(), you can rewrite it once more like this:

static async getDestination (envName) {
  const db = await new Promise(resolve => {
    DataBase.initDB(resolve);
  });

  const envCollection = db.collection('Environment');
  const promises = envName.map(value => new Promise((resolve, reject) => {
    envCollection.find({ environmentName: value }, (error, data) => {
      if (error) return reject(error);

      data.toArray((error, content) => {
        if (error) reject(error);
        else resolve(content);
      });
    });
  }));

  const contents = await Promise.all(promises);
  const ans = contents.reduce(
    (ans, content) => content.reduce(
      (ans, doc) => ans.add(doc.destination),
      ans
    ),
    new Set()
  );

  return [...ans];
}

The key difference here is that each

return somePromise.then(someVariable => { ... });

is replaced by

const someVariable = await somePromise;
...

Modularization

P.S. if you want to remove the constructed promises from your getDestination() function, you can refactor your code into two helper functions that can be used like this:

static getCollection (collectionName) {
  return new Promise(resolve => {
    DataBase.initDB(resolve);
  }).then(
    db => db.collection(collectionName)
  );
}

static getDocuments (collection, ...args) {
  return new Promise((resolve, reject) => {
    collection.find(...args, (error, data) => {
      if (error) return reject(error);

      data.toArray((error, content) => {
        if (error) reject(error);
        else resolve(content);
      });
    });
  });
}

static async getDestination (envName) {
  const envCollection = await this.getCollection('Environment');
  const promises = envName.map(
    environmentName => this.getDocuments(
      envCollection,
      { environmentName }
    )
  );

  const contents = await Promise.all(promises);
  const ans = contents.reduce(
    (ans, content) => content.reduce(
      (ans, doc) => ans.add(doc.destination),
      ans
    ),
    new Set()
  );

  return [...ans];
}

It may be slightly more code than before, but now you have two re-usable functions that can be called from other kinds of queries besides getDestination().