Consider enabling the require-await rule

[rajsite]

Overview

I know, I know.. hear me out. I found that the require-await rule was disabled in nimble and think it actually might make sense to enable these days. I can't say I've fully investigated it but I found it useful enough to enable in nimble.

Why the rule is currently disabled

Reduce error code paths

Our current config comes from airbnb where the commit comments give more insight than the description in the source file.

I'd summarize the airbnb logic as they like async-await because if a dev wrote something like:

function myFunc () {
    if (woops) {throw new Error}
    return new Promise((resolve, reject) => reject());
}

They have a function that has multiple error code paths: A synchronous exception and an asynchronous rejection to handle. However if you make the function async then you only have to deal with the asynchronous rejection error path. The throw statement would always turn into an async rejection:

async function myFunc () {
    if (woops) {throw new Error}
    return new Promise((resolve, reject) => reject());
}

For a funtion that returns a promise, making that function async helps reduce the error cases to consider.

Improve performance

When the aribnb rule was made, the behavior of awaiting a promise in an async function had signficant overhead:

The following (where bar returns a promise):

async function foo() {
  const result = await bar(); // result is not a promise, it is the resolved value
  return result; // result needs to be wrapped in a promise
}

Was less performant than (where bar still returns a promise):

async function foo() {
  const result = bar(); // result is a non-resolved promise
  return result; // the promise can pass through without a wrapper
}

Why we should enable the rule

Avoid unnecessary async

The immediate use case I found was that functions in the nimble code base were being made unnecessarily async. While that example is innocuous, making functions unnecessarily async is only a detriment to both performance and code complexity. Having the rule enabled lets the linter flag unnecessarily async cases.

Consistent async patterns keep error handling consistent

At least in nimble the reviewer recommendation (not official / documented) is to avoid mixing Promise and async/await style code. What this means in practice is that the airbnb example above would be refactored to something like the following in PR:

function doSomething() {
    return new Promise((resolve, reject) => reject());
}

async function myFunc () {
    if (woops) {throw new Error}
    await doSomething();
}

The doSomething function only uses Promises and the myFunc function is only using async / await. In practice, this results in the promise functions doing minimal work outside the Promise constructor so synchrous exception code paths are unlikely and the async functions have much clearer behavior by only handling one form of async control flow at a time.

Incidental performance concerns have been mitigated

The example given, i.e.:

async function foo() {
  const result = await bar();
  return result;
}

Now has minimal overhead in modern engines (at least in V8). As discussed in the Faster async functions and promises actual spec changes were made that avoid the additional microtask from occuring with nested async-await statements and temporary promises are elided.

Compared to trying to manually write promise code to control microtask points and promise instancing, the article states: "async/await outperforms hand-written promise code now".

Function resolution is at the call-site

With the example:

async function foo() {
  const result = bar();
  return result;
}

The actual resolution of the invocation of the bar function is not in the value of result. It's conceptually and debugging wise pretty annoying. Setting a breakpoint at this location in code will only give you an unresolved promise.

However with the following:

async function foo() {
  const result = await bar();
  return result;
}

The value of result is the resolved value in the function. This makes more sense in terms of control flow and helps during debugging. With the understanding that there is not a performance cost to it anymore, then it should be preferable to await closer to the call site that creates the promise.

Summary

Assuming the style guideline that async/await and Promise styles of coding should not be mixed, then it probably makes sense today to enable the require-await rule.

[rajsite]

As a follow-up which aligns with this change, in eslint the no-return-await rule has been deprecated aligning with the reasons above (the performance concern is no longer there) which makes the require-await rule sensible to enable consistently.

Proposal: Js: 'require-await': 'error' 'no-return-await': 'off'

Ts: 'require-await': 'off', '@typescript-eslint/require-await': 'error', 'no-return-await': 'off', '@typescript-eslint/return-await': 'error'

Note: The @typescript-eslint/return-await rule is a more robust version of the eslint no-return-await that handles try-catch behavior more robustly and is worth enabling in it's default mode of in-try-catch, see: https://typescript-eslint.io/rules/return-await/#in-try-catch

[rajsite]

require-await is recommended to enable in typescript-eslint no-return-await is deprecated Didn't sound controversial but wanted a sanity check. Will try enabling in SystemLinkShared.