node-express-boilerplate
node-express-boilerplate copied to clipboard
hagopj13
Style proposal: async/await vs .then()
I've seen there's a mix in the code where sometimes async/await is used and sometimes somePromise.then().catch(). I'd like to propose to homogenize the code by using always async/await, and I offer myself to make the PR should the proposal be accepted.
Also, if it's accepted, it should be added to the contribution guideline.
I think that there is already a wrapper (asyncCatch) for most functions in the controllers. Anyway I prefer async/await.
To @am2222
Why not to use
then().catch()?
async/await allows for cleaner code style.
Also, sometimes you need to use a result from previous calls, and it's cleaner to do:
// ...
try {
const aResult = await calculateA();
const bResult = await calculateB();
const finalResult = await calculateFinal(aResult, bResult);
return finalResult;
} catch (e) {
console.error(e);
// ...
}
Than:
// ...
return calculateA()
.then(aResult => calculateB()
.then(bResult => calculateFinal(aResult, bResult))
.catch(e => console.error(e));
In addition, there's a subtle thing to consider on the last snippet: if you were to use a block instead of an inline function on the anidated .then like:
// ...
.then(bResult => {
// ... some other statements
return calculateFinal(aResult, bResult);
})
// ...
You need to ensure you're returning the inner promise for the outer .catch() to catch possible errors from the inner promise. Or use a double .catch(). Thus, this style can induce you to make errors. For example if you were refactoring the one-liner into a block and forget to return the promise and put the inner .catch() you'll end up with an unnoticed bug until you get an UnhandledPromiseRejectionWarning.
Consider I gave a very simple example with 2 calls. What would the code look if you need 5 or 10 different asynchronous calls under the same function?
Quoting Douglas Crockford from memory: "If one coding style can induce you to make errors and other not, then use the later".
Aim to avoid the cognitive load always.
@fernandocanizo Thanks for the explanation. I used to use then().catch() format and have faced the issues you just mentioned.
@fernandocanizo While the scenario you describe is 100% correct, you can also have a scenario
where doSomething().then().then().catch().finally() makes sense. A sequence where actions are dependent on
the result of the previous action.
Also, having a code block in a .then() situation, wrapping in try() { } catch() { } is (should be) required.
@trasherdk My proposal was to bring consistency to the repo. To avoid having two coding styles. I don't really understand what are you trying to tell me when you say that "makes sense", I'd think that if it makes sense that way, it would also make sense using async/await
I have absolutely nothing against async/await and consistency is a good thing. Don't get me wrong.
What I'm trying to say, is that the task at hand should determine what approach is chosen. The scenario I describe, kinda calls for chaining.
@fernandocanizo I have tried to (mostly) use async/await, except for a few exceptions, which I just counted as 4 occurances. These are mainly to avoid top-level-await and Promise.all. If you have any suggestions on how to change these as well, please go ahead and make a PR.
