0

I try to update on group members value for each group , and after that i want to return new groups updated .i do this by adding each group to result array i declared before .

but i always got the empty result in result array . Anyone know how i can to make execution sequentially,do not goes to return statement first

This is my code :

searchGroups: async (groupIdsList, accountId) => {
  var result = [];
  return Group.find({
    _id: { $in: groupIdsList },
    accountId: mongoose.Types.ObjectId(accountId),
  }).then((groupList) => {
    groupList.forEach(function (group) {
      groupService
        .getMembersUserDetails(
          group.members,
          mongoose.Types.ObjectId(accountId)
        )
        .then((members) => {
          group.members = members;
          result.push(group);
        });
    });

    return result;
  });
};
Nicholas Tower
  • 72,740
  • 7
  • 86
  • 98
Ammar Mousa
  • 48
  • 10
  • need something like Promise.All() take a look here https://stackoverflow.com/questions/53798589/await-loop-vs-promise-all –  Apr 12 '20 at 19:30
  • The await keyword The real advantage of async functions becomes apparent when you combine it with the await keyword. This can be put in front of any async promise-based function to pause your code on that line until the promise fulfills, then return the resulting value. In the meantime, other code that may be waiting for a chance to execute gets to do so. You can use await when calling any function that returns a Promise, including web API functions. https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Asynchronous/Async_await – Arturas Apr 12 '20 at 19:35
  • Does this answer your question? [How to return many Promises in a loop and wait for them all to do other stuff](https://stackoverflow.com/questions/31426740/how-to-return-many-promises-in-a-loop-and-wait-for-them-all-to-do-other-stuff) – Dan O Apr 12 '20 at 19:40
  • there are many, many Stack Overflow questions regarding Javascript promises generated inside a loop. which of them have you researched and why do they not solve your specific problem? – Dan O Apr 12 '20 at 19:40

2 Answers2

3

Since you are using async, you could leverage await and simplify the code like this.

Also note that forEach doesn't support promises so you need to use either for..of or Promise.all with .map if you could do things oin parallel.

Something like this.

searchGroups: async (groupIdsList, accountId) => {
  const groupList = await  Group.find({"_id": {"$in": groupIdsList}, "accountId": mongoose.Types.ObjectId(accountId)});
  return Promise.all(groupList.map(async group => {
    group.members = await groupService.getMembersUserDetails(group.members, mongoose.Types.ObjectId(accountId));
    return group;
  }));
};
Patrick Roberts
  • 49,224
  • 10
  • 102
  • 153
Ashish Modi
  • 7,529
  • 2
  • 20
  • 35
  • Thanks @Ashish Modi for your answer , its returned updated group member , but the main property for group does not returned , for example group name ,id ..etc does i need to make map for all group properties ? – Ammar Mousa Apr 12 '20 at 19:58
  • @AmmarMousa in that case you need to do a then and add members in group and return that instead. updated my answer. You could also use `async/await` – Ashish Modi Apr 12 '20 at 20:02
  • @ Ashish Modi Thank you very much , it toke too much time from me , thanks again :)) – Ammar Mousa Apr 12 '20 at 20:06
  • 1
    @AshishModi I made a stylistic improvement to your answer, hope you don't mind. – Patrick Roberts Apr 12 '20 at 20:16
0

First, you'll have to convert your forEach into a for... of or any flat loop, while adding an await to your getMembersUserDetails:

    searchGroups: async (groupIdsList, accountId) => {
      var result = [];

      return Group.find({
        _id: { $in: groupIdsList },
        accountId: mongoose.Types.ObjectI(accountId)
      }).then(async groupList => {
        for (var group of groupList) {
          await groupService.getMembersUserDetails(group.members,mongoose.Types.ObjectId(accountId))
            .then(members => {
              group.members = members;
              result.push(group);
            });
        }
        return result;
      });
    };

Then, use the await operator when calling your function:

await searchGroups(groupIdsList, accountId);
Elias Faraone
  • 266
  • 2
  • 7
  • This has the same problem. – jfriend00 Apr 12 '20 at 19:42
  • 1
    This will probably work now with the edit (if you may the function `async` that the `await` is in), but it serializes the database requests, when they don't need to be serialized which may be much slower. You're also mixing `await` and `.then()` which usually isn't preferred. See Ashish's answer for a better way. – jfriend00 Apr 12 '20 at 19:44
  • Fare point, perhaps one can make subsequent calls without awaiting them and then use a Join operator to await them all together. – Elias Faraone Apr 12 '20 at 19:50
  • Never had a problem with awaiting a .then as it's clear that you are awaiting its return value. – Elias Faraone Apr 12 '20 at 19:51
  • @EliasFaraone awaiting a `.then()` isn't problematic, it's stylistically unnecessary since you can convert the `.then()` into another `await` statement and avoid nesting closures. – Patrick Roberts Apr 12 '20 at 19:55
  • Along with what Patrick said, that `await groupService.getMembersUserDetails(...)` will throw a syntaxError because it's not in an `async` function. That's the other part of my comment. I will remove my downvote if you at least add the necessary `async` to make it code that would work. – jfriend00 Apr 12 '20 at 20:02
  • @jfriend00 I have added the async, missed that you're right. – Elias Faraone Apr 12 '20 at 20:09
  • 1
    OK, now this is an answer that should work. I don't think it's optimal from a performance point of view because you've serialize each of the database requests and I don't think it's optimal from a style point of view because of the mixing of `await` and `.then()`. I removed my downvote, but my upvote goes to Ashish's answer as my preferred implementation. – jfriend00 Apr 12 '20 at 20:10