docs: add new section: 2.12 Always await promises before returning

[Alexsey]

TL;DR: In order to have full stacktraces

1. Always prefer return await over return when returning promises
2. Any function that is returning a promise must be declared as async

Issue

[ericblade]

... isn't return await useless?

[Alexsey]

@ericblade in case of v8 they are essential in reducing the stress level :)

The reasons why it's so I've tried to explain in returningpromises.md file of the PR. I'm going to improve the language a little bit, but the text is already open for feedback. Especially considering the fact that I'm not an English native speaker

[ericblade]

"return await" however, is a bit of a problem, and this rule/suggestion would be better showcased using "const x = await whatever(); return x;" ... for when you need to deal with a stack trace problem, I guess. I don't see where there's really much of a problem, though, it still ends up going back to the right place eventually.

https://jakearchibald.com/2017/await-vs-return-vs-return-await/

also

https://eslint.org/docs/rules/no-return-await

now, i'm not the maintainer of this project, i'm just expressing my opinion on the topic.

[Alexsey]

@ericblade why return await is a problem? This proposal is about how to return promises from functions. If you split return await with an intermediate variable you will just avoid the situation of returning a promise from an async function and the rule would sound like "never return promises from functions". Yes, this is also a way to have a good async stacktrace but the price you would need to pay for the same result is higher: for example, you would need to forbid implicit return in arrow functions if the function returns a promise

the article you have mentioned is very good and absolutely correct as for 2017 - I've sent it to a lot of people when it appeared :) But in 2017 there were no async stacktraces at all. Then almost two years ago v8 team had come with there "zero-cost async stacktrace" implementation that made a difference between return and return await

As for eslint rule, first of all, it has also appeared before "zero-cost async stacktrace" in v8. In second, this thing is specific to v8 and eslint is not about v8 - it's about EcmaScript in general while Node Best Practices are only about v8. In third, in the rule, there is a line in "When Not To Use It": "If you want the functions to show up in stack traces". You can read the discussion about it here eslint/eslint#11878 - authors of eslint were actually good with adding more info to the rule description but the topic starter decided to drop the idea for his own reasons

Edit: actually, the text of the eslint rule has already been updated since I read it for the last time and the second paragraph is already saying that avoiding await after return would lead to missing frames in stacktraces

[goldbergyoni]

@Alexsey Thanks again, I'm OOO until mid next week and I'll be sure to review then

[kevynb]

Thanks for taking the time to write this BP @Alexsey, great writeup!

Only suggested a few minor edits

[goldbergyoni]

@Alexsey Just a note that I plan to chime-in very soon, just a busy period

[goldbergyoni]

@Alexsey

Thanks again for bringing this hop topic 🔥💜

I read all the related materials, couple of thoughts:

1. No clear cut here - I'm not sure that this is a case for an explicit recommendation as one might favor performance over observability (see bullet 3' 👇), and going against an ESLint rule (partially) is something to consider. Unless, we can clarify that these guides are completely wrong (see bullet 3' 👇). In any case, almost all the text and great examples that you brought are still relevant, we would just need to specify the trade-off. Probably tweak and add few sentences

2. Formatting - You have great English writing skills for non-native speaker, still the texts and formatting should be revised. By the way, I'm also not a native English speaker, like you, I struggle also in my writing here, @BrunoScheufler @js-kyle helps me a lot in sharpening my writing (+200 PR that fixed TYPOs and grammar issues 😅). I'll share more detailed thoughts on this soon, once we conclude our main advice here

3. Tech question - The ESLint rule states that:

Keeps the current function in the call stack until the Promise that is being awaited has resolved, at the cost of an extra microtask before resolving the outer Promise

You wrote that:

had come with there "zero-cost async stacktrace" implementation that made a difference between return and return await

Isn't the modern asyn stacktrace keep the function in the call stack until the outer promise is resolved?

[Alexsey]

@goldbergyoni thank you for the review!

You are correct - we need to add some notes about performance tradeoff and I already know what to write

As for your question, I don't see any conflict between ESLint description any what I've written: after "zero-cost async stacktrace" has been introduced return awaitwould keep the current function in the call stack until the Promise that is being awaited would be resolved while return would not keep the current function in the call stack. Not all modern async stacktraces works this way: Firefox doesn't require return await for having good async stacktraces. Please, tell me if you still see some collisions

[Alexsey]

Added Tradeoff section to returningpromises.md with notes on performance penalty, changed Otherwise sentence as suggested by @goldbergyoni, and improved Russian translation

The PR is ready for the review!

[goldbergyoni]

[I mistakenly approved instead of 'Request changes', please treat this accordingly]

@Alexsey Great progress, I believe we can merge this in days and it will be valuable even to experienced developers

I left a few comments, here is a summary:

1. Mandatory: The 'one paragraph' should start with a simpler explanation. The advanced stuff are great but should come later. I proposed a skeleton for the simple text in my comment

2. Mandatory: Revise the English grammar, this should be done after the text is done and can be relatively short

@BrunoScheufler @js-kyle Would you be able to help with this?

3. Optional: Other comments which I left inside, take whatever makes sense to you

[goldbergyoni]

Consider including 'the why' in the title:

'Always await promises before returning to avoid a partial stacktrace'

[goldbergyoni]

Consider including the reason:

Always return await when returning a promise to benefit full error stacktrace

[goldbergyoni]

Suggestion - Explain which function and why is it important:

The function that returns a promise without awaiting won't appear in the stacktrace although it's part of the flow and might even be the cause for the abnormal behavior

[goldbergyoni]

Minor suggestion: Add happy and sad emojis to the relevant line, of the words "Good", "Issue" to flag easily which path is troubled

[goldbergyoni]

Important - The 'one paragraph' should start by telling the simple story. Advanced theories are great but should come at the end or within another section 'More info' below

Simply put, just put 2-3 sentences that tell the basic call for action, the rest should be put below or in another bullet, for example:

When an async function is calling another async function or a promise, prefer awaiting on those before returning (e.g. return await someAsyncFun), otherwise the caller function won't appear in the stacktrace in case of error which might confuse the developer who tries to understand the flow. This is happening because of the new Node.js mechanism of async stacktraces that omits synchronous functions from the stack and under the hood works in a different way that a casual synchronous stack (see more details below)

Also, I would include here some short notice about the performance impact - While this increases the error visibility, it carries a minor, some may say negligible, performance penalty

@Alexsey

[goldbergyoni]

Minor suggestion - Separate into two examples, good and bad, it will make those shorter with clear title (e.g. Code example anti pattern: Return without await confuses the stacktrace)

[goldbergyoni]

@Alexsey I'm just recaping here the current status, LMK if we/I can help somehow with pushing this:

I mistakenly approved instead of 'Request changes', requested edits:

1. Mandatory: The 'one paragraph' should start with a simpler explanation. The advanced stuff are great but should come later. I proposed a skeleton for the simple text in my comment

2. Mandatory: Revise the English grammar, this should be done after the text is done and can be relatively short

3. Optional: Other comments which I left inside, take whatever makes sense to you

[Alexsey]

@goldbergyoni I'm going to go over all of your comments and also add some small things I have think about, but right now I'm our of time :( I expect to get back to this next weekend. The PR is not forbidden - I'm having some thoughts about it almost every day :)

[goldbergyoni]

@goldbergyoni I'm going to go over all of your comments and also add some small things I have think about, but right now I'm our of time :( I expect to get back to this next weekend. The PR is not forbidden - I'm having some thoughts about it almost every day :)

@Alexsey This is the OSS land, there are no clocks, no time, no gravity, no apologies, only good will 🙂. In other words, take as much time as needed

[Alexsey]

The PR is ready for the next iteration of review: I went over all comments, added a simplified one paragraph explanation, split all code examples into "Anti-pattern" + "the right way to go" and added one new anti-pattern when usage of async callback is leading to missing frames in stackteaces

[goldbergyoni]

@Alexsey Thanks for making this even better and for understanding our great care for the content quality. The home page text is great and the advanced explanations are really outstanding.

We're 10 min of work from approving this PR, 4 fundamental asks:

1. One paragraph should start with a simple explanation - There should be a simpler and actionable explanation for newbie average developers. See an example of how simple it should be below

2. History is out of scope - The one paragraph contains history of changes in Node, our future reader in 2022 won't care about this, it neither exists in other bullets. You may exclude this or move to the advanced explanation

3. Advanced should be at the bottom - Our format is always 'One paragraph' and then code examples. There are no other bullets with advanced explanations like your amazing drill down. To make this consistent, at least let's place the advanced text at the bottom including everything that is not 'one paragraph' or 'code examples'

4. Code examples should follow our format - See attached an image that exemplify the formatting of code examples, they all start with the same text. It's not mandatory to put the JS/TS expander, but the title should be consistent

**Dead-simple explanation for the opening of the one paragraph:
'"When function A calls async function B, make A also async and await on B's response, otherwise A won't appear in the stacktrace in case of error. This might confuse the developer who tries to understand the flow..."

[goldbergyoni]

Hey @Alexsey , Just checking whether you need anything from me or good to make the slight adjustments. This is by no mean aim to rush you, take your time

[Alexsey]

Hey @goldbergyoni, thank you for reminding - you have guessed a good moment :) I finally have some spare time to finish with this one - 90% will make the PR during the next 3 days, hope even today's evening.

[Alexsey]

Simplified "One paragraph", added collapsable blocks to code snippets, moved history to a separate paragraph called "Why return await was considered as anti-pattern in the past" and move it with "Advanced explanation" below code examples

I didn't remove history because there is a lot of people who consider return await as anti-pattern, and I think it's important to explain that this information is not wrong but obsolete

[goldbergyoni]

LGTM

[goldbergyoni]

@Alexsey That's an epic PR, great idea and execution 🏆

Let's promote this:

1. I'm gonna tweet about. Do you happen to have a Twitter handle?
2. Put it in our news section
3. Add you to the contributors list - @all-contributors please add @Alexsey for content

[allcontributors]

@goldbergyoni

I could not determine your intention.

Basic usage: @all-contributors please add @Someone for code, doc and infra

For other usages see the documentation

[goldbergyoni]

@all-contributors please add @Alexsey for content

[allcontributors]

@goldbergyoni

I've put up a pull request to add @Alexsey! 🎉

[Alexsey]

@goldbergyoni Cool, really glad we have finally done this, and thank you for your intention to promote!

Unfortunately, I don't have a Twitter account :(

[goldbergyoni]

@Alexsey That's fine, I'll tweet anyway and promote this great content!

[goldbergyoni]

@Alexsey Would you like to chime-in here?

https://www.reddit.com/r/node/comments/k1x8p1/a_new_best_practice_212_await_before_returning/

[Alexsey]

@goldbergyoni now I will probably even register on Reddit =) Thank you for the link!