Array of promises in a promise array

Question

I have an array which contains books data. I have to loop in array and make an service call to fetch details of each book and each book data has id's of attachments associated to book and make a service calls to fetch associated attachments for each book.

Here issue is promise.all not waiting for aAttachmentPromises to get resolved

  function ExportbooksData(books) {
  return new Promise((resolve, reject) => {
    if (books && books.length > 0) {
      let aPromises = [];
      for (let i = 0; i < books.length; i++) {
        const id = books[i].id;
        const name = books[i].name;
        aPromises.push(this.getBooksData(name, id, null).then(results => {
          let aAttachmentPromises = [];
          Object.entries(results).forEach(([key, value]) => {
            let fieldName = key;
            if (value.constructor === Array && value.length > 0) {
              aAttachmentPromises.push(this.getAttachments(fieldName).then(fileContent => {
              }))
            }
          });
        }));
      }
      // Resolve when all are done!
      Promise.all(aPromises)
        .then(results => resolve(results))
        .catch(error => reject(error));
    }
  })
}

Don't need to wrap inside a `new Promise()`, `Promise.all()` itself returns a promise. — sujeet, May 11 '20 at 07:50
Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! — Bergi, May 11 '20 at 08:09

Josh Wulf · Answer 1 · 2020-05-11T12:30:36.870

I refactored this live in the BrisJS meetup in Brisbane, Australia tonight. Here is the video where I do it and explain the changes: https://youtu.be/rLzljZmdBNM?t=3075.

Here is a repo with both your version and the refactor, with mocked services: GitHub repo

function getAttachmentsForBookWithMetadataArray(bookdata) {
  return Object.entries(bookdata)
    .filter(([_, value]) => value.constructor === Array && value.length > 0)
    .map(([fieldname, _]) => getAttachments(fieldname));
}

function getAttachmentsForBook(book) {
  return getBookData(book).then(getAttachmentsForBookWithMetadataArray);
}

function ExportbooksData(books) {
  return !books || !books.length > 0
    ? Promise.reject(new Error("Did not get an array with 1 or more elements"))
    : Promise.all(books.map(getAttachmentsForBook));
}

For a discussion on dealing with failures, see this article: Handling Failure and Success in an Array of Asynchronous Tasks

You should put the `filter` before the `map` , so that you don't need to map to `undefined`. — Bergi, May 11 '20 at 12:20

Always Learning · Accepted Answer · 2020-05-11T11:38:29.913

0

You build up aAttachmentPromises but you don't return it, so your aPromises.push always pushes a Promise that resolves immediately with undefined into the array that you're waiting on. Change your code like this:

aPromises.push(
  this.getBooksData(name, id, null).then(results => {
    let aAttachmentPromises = [];
    Object.entries(results).forEach(([key, value]) => {
      let fieldName = key;
      if (value.constructor === Array && value.length > 0) {
        aAttachmentPromises.push(this.getAttachments(fieldName)
          .then(fileContent => {})
          .catch(err => {
            if (err == "minor") console.log("Ignoring error",err);
            else throw err;
          })
        );
      }
    });
    return Promise.all(aAttachmentPromises); // <--- here!
  })
);

But in addition to that you can simplify the function also. You don't need to wrap everything in a new Promise object, and using a variable to hold a value that's used only once is not helpful. This simplified version is (imho) easier to read/maintain:

function ExportbooksData(books) {
  let aPromises = [];
  for (let i = 0; books && i < books.length; i++) {
    aPromises.push(
      this.getBooksData(books[i].name, books[i].id, null).then(results => {
        let aAttachmentPromises = [];
        Object.entries(results).forEach(([key, value]) => {
          if (value.constructor === Array && value.length > 0) {
            aAttachmentPromises.push(this.getAttachments(key).then(fileContent => {}));
          }
        });
        return Promise.all(aAttachmentPromises);
      })
    );
  }
  return Promise.all(aPromises);
}

edited May 11 '20 at 11:38

answered May 11 '20 at 07:44

Always Learning

5,510
2
17
34

1

I don't think just returning `aAttachmentPromises` will actually resolve all promises inside this array, as `aPromises` now becomes `array of array of promises`. – sujeet May 11 '20 at 07:59
1

You're right @SujeetAgrahari the return should be a `Promise.all` itself. I've edited my answer – Always Learning May 11 '20 at 08:03
After returning attachment promise its working fine. However if one the attachment promise fails then it's not getting any getBooksData of remaining books. – viswa teja May 11 '20 at 11:17
yes, if you want to ignore individual failures you can add `.catch` statements and determine whether the failure should be re-thrown on a case-by case basis. But what you describe is how `Promise.all` works – Always Learning May 11 '20 at 11:26
I tried adding catch like below return Promise.all(aAttachmentPromises).then(results => resolve(results)) .catch(error => reject(error)); and same for aPromises. After this change I am not getting attachments for success one as well . – viswa teja May 11 '20 at 11:34
I updated the first solution in my answer to have a catch statement. Let me know if that makes sense to you. – Always Learning May 11 '20 at 11:39

Array of promises in a promise array

2 Answers2