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 proccessData(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?
3
Answers
The following is imo an implementation which achieves the same ends but with better practices, and solves your problem of un
await
ed promises! Note that I spoofed all the functions you referenced but didn’t include in your question, so you can actually run this code:Some practices which make this code, imo, better:
{ status: 'failed' }
! This will make your code much leaner, robust, and easier to debug!function*
) is used to perform repetitive batch selections.then
in many cases where its use made the code less unmaintainableawait
ed promisesThis new format raises a question: why are the records returned from
getAllRecords
ignored? That can’t be right – shouldn’t they factor intofirebaseDoc.updatecityDoc(...)
?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
andg
and we need to pass the result off
tog
.Wrong
Nest the
.then
handlers like in your sample code: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
withasync/await
:Why It’s Bad
It’s obviously sub-optimal compared to just marking
doTheThing
asasync
and usingawait
to get the result of callingf
in the first place.Wrong
Resolving an outer Promise when the inner Promise resolves. Say we want to return a Promise of
gResult
fromdoTheThing
: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
If you need to do more intermediate work than just call
g
with the result off
you can do it like this:This takes advantage of Promises auto-flattening: when we return a Promise from
.then
we get aPromise<T>
rather than aPromise<Promise<T>>
. We can even tack another.then
:Another benefit of this approach is that the single
.catch
at the end will catch all the Promise rejections anywhere in the chain of.then
s, which you cannot do if you nest the calls rather than chain them!Also Right
Simple. Gets the job done. Can be combined with
try/catch
for easy-peasy error handling:For an idea of what these principles applied to your code might look like, see this other answer that refactored your code.
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 thatcompletes the
mainFunc
. But it doesn’t – it returnsundefined
only from the current.then()
handler, then it executes the next.then()
handler withundefined
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: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 ofawait
, there is no reason to declare your functions asasync
.For the error handling, keep errors as rejections and only handle and transform them into a result object in the very end.
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 forgetAllRecords
, and you won’t have as much nesting, which greatly eases refactoring.