0

I have the next method

async findOne(id: string) {
  const coffee = await this.coffeeModel.findById(id);
  if (!coffee) throw new NotFoundException('coffee not found');
  return coffee;
}

But I want to check if in an array of coffees all exist so I am doing the following:

const coffeesPromises = []
arrayOfCoffees.forEach((element) => {
  coffeesPromises.push(this.findOne(element))
})

await Promise.all(coffeesPromises) 
//In case 1 coffee does not exist the API return
{
  "statusCode": 404,
  "message": "coffee not found",
  "error": "Not Found"
}

But when I try to wait for the coffee and make a condition with each coffee the error is thrown on the console (in case 1 coffee does not exist) and not as a JSON.

One way I tried to solved it was:

Promise.all(promises).then((results) =>
  results.forEach((result) => if (!result.sugar) throw new BadRequestException('coffee has no sugar'))
);

The way Im doing it is the next, but is slow, so that's why Im trying to improve it.

for (const coffee of arrayOfCoffees) {
  const coffeeDocuments = await this.findOne(coffee);
  if (!coffeeDocuments.sugar)
    throw new BadRequestException(
      `coffee has no sugar`,
    );
}

Which is the best way to handle this?

Legion
  • 129
  • 6

1 Answers1

2

Array.forEach technically ignores Promise(async/await). If you want to use Promise.all with array you'd better to use Array.map. The code would be something like this.

async findOne(id: string) {
  const coffee = await this.coffeeModel.findById(id);
  if (!coffee) throw new NotFoundException(`coffee ${id} not found`);
  return coffee;
}

const promises = arrayOfCoffees.map(async element => await this.findOne(element))

await Promise.all(promises).catch(e => /* error handling goes here */)

// or shortly
await Promise.all(arrayOfCoffees.map(async element => await this.findOne(element)))
             .catch(e => /* error handling goes here */)

As you can see the last line, I put an error handler to Promise.all. Because if Promise.all meets an error while running all the other promises it emits an error.

Kai
  • 101
  • 3
  • I am storing all the promises in an array, and then resolving them with the Promie.all, that's why Im not using an asynchronous foreach – Legion Dec 24 '22 at 17:16
  • Yes, You can use it as you prefer, I just gave you my personal opinion in terms of easy maintenance. Because for the other developers it would be easy to understand that **this.findOne()** method is a Promise if **with await keyword**. By the way, the point is that you can manage Promise.all with one error handler by adding .catch at the end. – Kai Dec 24 '22 at 17:30
  • @Legion Technically, **for...of** and **forEach** are slower than map (check this [link](https://leanylabs.com/blog/js-forEach-map-reduce-vs-for-for_of/)) If you are considering to find many rows by passing many ids, you might consider to use findMany() instead. – Kai Dec 24 '22 at 17:38
  • @Legion If you consider to use findMany with given ids., check this [link](https://stackoverflow.com/questions/69857000/prisma-how-can-i-find-all-elements-that-match-an-id-list). – Kai Dec 24 '22 at 17:48
  • @Kai It's unclear when the article you link was written, but [`for … of` and `forEach` are optimised equally well in js engines](https://youtu.be/m9cTaYI95Zc?t=951) since a few years already – Bergi Dec 24 '22 at 23:50
  • @Bergi fair enough to compare between forEach and for..of in the link, however I was talking the comparison between for-loop and Array.map, not for..of and forEach. I might need to dig it a little bit more. By the way thanks for sharing the link. – Kai Dec 25 '22 at 10:42