How do i resolve promises properly?

Question

How do I make sure all promises are resolved properly? Here even after the execution of the function some statements are still getting printed(the ones where I have added comments). I feel getAllRecords is not completing promises properly. Where am I going wrong?

const getAllRecords = async (offset = 0) => { 
  return fetchRecord(offset, 10).then((resp) => { 
    console.log("Called fetchRecord") 
    if (resp.status === 'success') {
       let responseData = resp.data 
       if (responseData.length === 0 { 
         return ({ status: 'success' }) 
       } 
      return processData(responseData).then((updateResp) => { 
        if (updateResp.status === 'success') { 
           offset += 10 
           return getAllRecords(offset) 
         } else { 
           return ({ status: 'error' }) 
       }
       }).catch(err => { 
         return ({ status: 'error' }) 
       })
      } else { 
        return ({ status: 'error' }) 
      } 
  }).catch(err => { 
    return ({ status: 'error' }) 
  }) 
} 


const mainFunc = async () => { 
  console.log('Inside mainFunc') 
  return new Promise((resolve, reject) => { 
    return firestore.getAllCities() 
      .then(async (cities) => { 
        return getAllRecords().then(getAllRecordsResponse => { 
          console.log("Called getAllRecords") // 
          if (getAllRecordsResponse.status === 'success') { 
             return getAllRecordsResponse 
           } else { 
             return reject({ status: 'error' }) 
           } 
        }).then((getAllRecordsResponse) => {
             let updateArray = []
             console.log("Updating firestore db") 
             for (const city of cities) { 
                updateArray.push(firebaseDao.updatecityDoc(city)) 
             } 
             return Promise.allSettled(updateArray).then((responseArr) => { 
               let errorArr = [] 
               responseArr.map((item) => 
                 item.status === 'rejected' ? errorArr.push(item.reason) : null 
               ) 
               if (!errorArr.length > 0) { 
                 console.log("done processing") //
                 return resolve({ status: 'success' }) 
               } else { 
                 return resolve({ status: 'error' }) 
               } 
         }) 
      }).catch((err) => { 
        return resolve({ status: 'error' }) 
      }) 
    }).catch((err) => { 
      return resolve({ status: 'error' }) 
    }) 
  }) 
} 

I am trying to fetch records from some other place in fetchRecord and processing the extracted data in processData. getAllRecords is a recursion because we cant get all the data at once, only 10 records can be extracted, so I am extracting the records until I get an empty response.

Where is the promise not getting handled properly is what I am not sure about. Am I missing out something here?

Is there a way to use promises only rather than async/await?

Answer 1

The following is imo an implementation which achieves the same ends but with better practices, and solves your problem of unawaited promises! Note that I spoofed all the functions you referenced but didn't include in your question, so you can actually run this code:

const getAllRecords = async function*(offset = 0, limit = 10) {
  
  // Yields records from `fetchRecord`, one at a time
  // Records are selected in batches of size `limit`
  
  while (true) {
    // Select latest batch; halt if no items are returned
    const { data: items } = await fetchRecord(offset, limit);    
    if (!items.length) return;
    
    for (const item of items) yield item;

    // Increment the offset; will loop again
    offset += limit;
  }
  
};

const mainFunc = async () => {

  const cities = await firestore.getAllCities();
  
  // Loop through all records yielded by `getAllRecords`; each will result
  // in an "update promise" which we store in an array:
  const updatePromises = [];
  for await (const record of getAllRecords())
    for (const city of cities)
      // TODO: Ignoring `record`??
      updatePromises.push(firebaseDoc.updatecityDoc(city));
  
  // If we want to return all errors which occurred instead of just the
  // first, we need to do some manual error handling - all `updatePromises`
  // have a `.catch` applied, which stores the error
  const settled = await Promise.allSettled(updatePromises);
  
  // A final resulting error has an "errors" property attached to it,
  // exposing the full set of errors which occurred
  const errors = settled.filter(item => item.status === 'rejected');
  if (errors.length) throw Object.assign(Error('Some updates failed'), { errors });
  
  // No return value upon success

};

// Spoofed objects, to make this script runnable:
const firestore = {
  getAllCities: async () => [ 'toronto', 'new york', 'los angeles', 'tokyo' ]
};
const firebaseDoc = {
  updatecityDoc: async (...args) => console.log('Updating city doc; args:', args)
};
const spoofedRecords = 'a'.repeat(30).split('').map((v, i) => ({ recordNum: i }));
const fetchRecord = async (offset, limit) => ({
  data: spoofedRecords.slice(offset, offset + limit)
});

mainFunc()
  .then(() => console.log('Completed successfully'))
  .catch(err => console.log('Encountered fatal error', err));

Some practices which make this code, imo, better:

• Never use return values to indicate errors! Always implement functions such that if they return a value, it indicates success. Always assume that if a function returns a value, it executed successfully. If you want to signify that a function failed, throw an Error - never return something like { status: 'failed' }! This will make your code much leaner, robust, and easier to debug!
• A generator (function*) is used to perform repetitive batch selections
• I removed .then in many cases where its use made the code less unmaintainable
• Some syntax errors are fixed; "firebaseDao" typo is fixed
• Overall structure makes it easier to avoid unawaited promises

This new format raises a question: why are the records returned from getAllRecords ignored? That can't be right - shouldn't they factor into firebaseDoc.updatecityDoc(...)?

Answer 2

TL;DR

Don't nest asynchronous code. That's the rule-of-thumb for avoiding most mistakes.

Thorough Version:

It's frequently the case that you want to do something asynchronous based on the result of another asynchronous operation, and then do something else. There are two right ways to do that, and a lot of wrong ones. So lets say we have two Promise-returning functions f and g and we need to pass the result of f to g.

Wrong

Nest the .then handlers like in your sample code:

function doTheThing() {
  f().then((fResult) => {
    g(fResult).then((gResult) => {
      console.log(gResult)
    })
  })
}

Why It's Bad

Say instead of 2 steps it's 10, and each depends on the previous result. Now you're at a potentially 40 space indent before you get to the code that actually does anything useful, and you've recreated the dreaded callback hell Promises were supposed to protect us from just with much worse performance from all the wrappers and extra allocations. Also what about error handling? You can only .catch at the individual Promise level, every .then in the chain is a potential UnhandledRejectionError.

Wrong

Mix .then with async/await:

function doTheThing() {
  f().then(async (fResult) => {
    const gResult = await g(fResult)
    console.log(gResult)
  })
}

Why It's Bad

It's obviously sub-optimal compared to just marking doTheThing as async and using await to get the result of calling f in the first place.

Wrong

Resolving an outer Promise when the inner Promise resolves. Say we want to return a Promise of gResult from doTheThing:

function doTheThing() {
  return new Promise((resolve) => {
    f().then((fResult) => {
      g(fResult).then((gResult) => {
        resolve(gResult)
      })
    })
  })
}

Why It's Bad

This is the fabled Promise constructor anti-pattern and in addition to the problems outlined in that Q&A you can no longer defer error handling to the caller.

Right

function doTheThing() {
  return f().then(g).catch(console.error)
}

If you need to do more intermediate work than just call g with the result of f you can do it like this:

function doTheThing() {
  return f()
    .then((fResult) => {
      // do intermediate work
      return g(fResult)
    })
    .catch(console.error)
}

This takes advantage of Promises auto-flattening: when we return a Promise from .then we get a Promise<T> rather than a Promise<Promise<T>>. We can even tack another .then:

function doTheThing() {
  return f()
    .then((fResult) => {
      // do intermediate work
      return g(fResult)
    })
    .then((gResult) => {
      // do something
    })
    .catch(console.error)
}

Another benefit of this approach is that the single .catch at the end will catch all the Promise rejections anywhere in the chain of .thens, which you cannot do if you nest the calls rather than chain them!

Also Right

async doTheThing() {
  const fResult = await f()
  const gResult = await g(fResult)
  // do stuff
  return gResult
}

Simple. Gets the job done. Can be combined with try/catch for easy-peasy error handling:

async doTheThing() {
  try {
    const fResult = await f()
    const gResult = await g(fResult)
    // do stuff
    return gResult
  } catch (err) {
    console.error(err)
  }
}

For an idea of what these principles applied to your code might look like, see this other answer that refactored your code.

Answer 3

It's a bit hard to spot, but the root of your problem is the use of the Promise constructor antipattern. While you are trying to handle all errors, you made the mistake to think that

         return reject({ status: 'error' }) 

completes the mainFunc. But it doesn't - it returns undefined only from the current .then() handler, then it executes the next .then() handler with undefined as the argument, continuing execution even after your promise is already rejected.

You can fix the mistake by not chaining .then(…).then(…) here, instead using a single function:

…
    return getAllRecords().then(getAllRecordsResponse => { 
      console.log("Called getAllRecords")
      if (getAllRecordsResponse.status !== 'success') { 
        return reject({ status: 'error' }) 
      } else {
//    ^^^^^^^^
        let updateArray = []
        console.log("Updating firestore db") 
        for (const city of cities) { 
          updateArray.push(firebaseDao.updatecityDoc(city)) 
        } 
        return Promise.allSettled(updateArray).then(…);
      }
    });
…

---

However, that doesn't improve your code by much. Your error handling is all over the place, with lots of duplicated code, and you should get rid of the new Promise. Also if you want to use .then() instead of await, there is no reason to declare your functions as async.

For the error handling, keep errors as rejections and only handle and transform them into a result object in the very end.

function getAllRecords(offset = 0) { 
  return fetchRecord(offset, 10).then(resp => { 
    const responseData = resp.data;
    if (responseData.length === 0 { 
      return;
    } 
    return processData(responseData).then(updateResp => {
      offset += 10;
      return getAllRecords(offset);
    });
  });
} 


function mainFunc() {
  console.log('Inside mainFunc') 
  return firestore.getAllCities().then(cities => { 
    return getAllRecords().then(getAllRecordsResponse => { 
      console.log("Called getAllRecords")
      const updateArray = []
      console.log("Updating firestore db") 
      for (const city of cities) { 
        updateArray.push(firebaseDao.updatecityDoc(city)) 
      } 
      return Promise.allSettled(updateArray);
    });
  }).then(responseArr => { 
    const errorArr = responseArr.filter(item =>
      item.status === 'rejected'
    ).map(item =>
      item.reason
    );
    if (!errorArr.length) {
      console.log("done processing");
      return { status: 'success' };
    } else { 
      return { status: 'error' };
    }
  }, err => { 
    return { status: 'error' };
  });
}

While this code is already much simpler, I would recommend to use async/await syntax as demonstrated in the other two answers. You no longer need to use recursion for getAllRecords, and you won't have as much nesting, which greatly eases refactoring.