Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change @typescript-eslint/return-await to level error with 'always' config #41

Merged
merged 1 commit into from
Jan 31, 2022

Conversation

swarmiakimmo
Copy link
Contributor

@swarmiakimmo swarmiakimmo commented Jan 31, 2022

Here's the documentation for "always" setting. This change will affect our codebases largely, but fortunately it is auto-fixable. The change aims to improve two scenarios:

  1. Prevent easy-to-miss bugs with async / await in try-catch blocks. This issue describes the bug best:
  2. Improve async stack traces. The improvement is only (afaik) applicable to V8 engine, which node uses. More from this below.

The community seems to be divided on the topic. Here's a few opposing opinions about why one should not use return await convention:

My personal take on this is that better stack traces overcomes the tradeoffs, assuming we are talking about backend node processes. This PR introduces the options only in node projects just to be safe. I checked that our frontend repo had only a single place where this config change fixed something, so enabling it there too wouldn't change much either.

Want to see the stack trace improvements in practice? Just save this test.js and run node test.js to see the effect.

(function () {
  async function foo() {
    return await bar();
  }

  async function foo2() {
    return bar();
  }

  async function bar() {
    await Promise.resolve();
    throw new Error('BEEP BEEP');
  }

  foo().catch(error => console.log(error.stack));
  foo2().catch(error => console.log(error.stack));
})()

The result is that return await bar() has a better stack trace:

➜  ~ node code/rapu/test.js
Error: BEEP BEEP
    at bar (/Users/kimmo/code/rapu/test.js:14:11)
    at async foo (/Users/kimmo/code/rapu/test.js:4:12)
Error: BEEP BEEP
    at bar (/Users/kimmo/code/rapu/test.js:14:11)

For the curious, here's detailed explanation of why this happens in V8 nodejs/node#36126 (comment) and https://v8.dev/blog/fast-async

@swarmiakimmo swarmiakimmo requested a review from a team as a code owner January 31, 2022 14:04
@swarmiakimmo swarmiakimmo merged commit 60f7835 into master Jan 31, 2022
@swarmiakimmo swarmiakimmo deleted the require-await-always branch January 31, 2022 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants