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
– Sujeet, Commented May 11, 2020 at 7:50

Josh Wulf · Accepted Answer · 2020-05-11 12:30:36Z

1

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

edited May 11, 2020 at 12:30

answered May 11, 2020 at 11:50

Josh Wulf

4,9122 gold badges24 silver badges35 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Bergi Over a year ago

You should put the filter before the map , so that you don't need to map to undefined.

Always Learning · Accepted Answer · 2020-05-11 11:38:29Z

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, 2020 at 11:38

answered May 11, 2020 at 7:44

Always Learning

5,6232 gold badges20 silver badges35 bronze badges

6 Comments

Sujeet Over a year ago

I don't think just returning aAttachmentPromises will actually resolve all promises inside this array, as aPromises now becomes array of array of promises.

Always Learning Over a year ago

You're right @SujeetAgrahari the return should be a Promise.all itself. I've edited my answer

viswa teja Over a year ago

After returning attachment promise its working fine. However if one the attachment promise fails then it's not getting any getBooksData of remaining books.

Always Learning Over a year ago

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

viswa teja Over a year ago

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 .

|

Collectives™ on Stack Overflow

Array of promises in a promise array

2 Answers 2

1 Comment

6 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

6 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related