Async findOne() operation inside forEach Loop

Question

I am having a hard time understanding JavaScript Promises. I am searching on of my Mongoose models for objects that meet a certain condition and if they exist, I want to make the object into a plain JS object and add a property onto it.

Unfortunately, I am unable to wrap my head around how I can ensure my forEach loop will run completely before my promise ends up resolving. Please see my code.

// Called to check whether a user has participated in a given list of challenges
participationSchema.statics.getParticipation = function(user, challenges) {
  return new Promise((resolve, reject) => {
    challengesArray = [];
    challenges.forEach((challenge) => {
      // Model#findOne() is Async--how to ensure all these complete before promise is resolved?
      Participation.findOne({user, challenge})
      .then((res) => {
        if (res) {
          var leanObj = challenge.toObject();
          leanObj.participation = true;
          challengesArray.push(leanObj);
        }
      })
      .catch(e => reject(e));
    })
    console.log("CHALLENGES ARRAY", challengesArray); // Challenges Array empty :(
    resolve(challengesArray);
  });
}

I've looked through similar questions, but am unable to get to an answer. Appreciate the help.

Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it), and don't use `forEach`! You are looking for `Promise.all` — Bergi, Jun 02 '17 at 03:39
Well, as asynchronous code is asynchronous, you are clearly resolving `challengesArray` before the asynchronous results are available — Jaromanda X, Jun 02 '17 at 03:51

nem035 · Accepted Answer · 2018-12-08T17:23:02.363

2

So, what is happening when you call getParticipation is that the forEach loop runs all the way and all individual promises for Participation.findOne are created but not yet resolved. The execution doesn't wait for them to resolve and continues after the forEach, resolving the top-level promise challengesArray, which is still empty at that point. Sometime after, the promises created in the forEach start resolving but their results are now lost.

Also, as Bergi mentioned in the comments, nesting promises is considered an anti-pattern; promises should be chained, not nested.

What you want is to use something like Promise.all to wait for all of your promises to finish first, then you filter out all non-existing results and finally return the array.

participationSchema.statics.getParticipation = function(user, challenges) {
  return Promise.all(challenges.map(challenge => {
    return Participation.findOne({user, challenge}).then(result => {
      if (result) {
        var leanObj = challenge.toObject();
        leanObj.participation = true;
        return leanObj;
      }
    });
  })
  // at this point, results contains an array of `leanObject` and `undefined` depending if the `findOne` call returned anything and the code withing the `if` above was run
  .then((results) => {
    return results.filter(result => !!result) // filter out `undefined` results so we only end up with lean objects
  });
}

edited Dec 08 '18 at 17:23

answered Jun 02 '17 at 03:45

nem035

34,790
6
87
99

Ah, this is so interesting! So in this design, you prepare all the promises in an array and `promises.all()` ensures they all complete, correct? Please let me try this out real quick and get back to you. – Govind Rai Jun 02 '17 at 03:53
1

Not sure what you mean by prepare. What is happening is that first, the array of challenges is converted to an array of promises, which represent the calls to `Participation.findOne` and all the requests are made, basically in parallel. Then those requests start completing in some arbitrary order and the promises start resolving accordingly, which runs the `then` clause for each of them and checks if results exist, converting them to `leanObj` if it does. Once all of these promises are resolved, `Promise.all` resolves with an array of the results obtained from each promise. – nem035 Jun 02 '17 at 03:57
Ah, yes, that is what I meant by prepare, as in creating a bunch of promises. Also could you shed some light on ***results contains an array of `leanObject` and `undefined` depending if the `findOne` call returned anything and the code withing the `if` above was run`*** – Govind Rai Jun 02 '17 at 04:09
1

Sure. `Promise.all` will resolve with an array of results for each promise chain in the array passed into it (look at MDN for more info). In this example, we're passing an array of promises into `Promise.all`, where each promise is a promise chain consisting of a call to `findOne` and then a return of either `leanObj`, if `result` is truthy, or nothing, which is by default `undefined`, if `result` is falsy. That means that any of these individual promise chains will resolve with the `leanObj` or `undefined`. Therefore, the array of results for each promise will contain one of those 2 values. – nem035 Jun 02 '17 at 04:14
got an error : `TypeError: keys.map(...).then is not a function`, and resolved by taking `then` outside. – 151291 Feb 02 '19 at 07:50

Async findOne() operation inside forEach Loop

1 Answers1