Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

💭 [lint] require await inside async functions #1118

Closed
ematipico opened this issue Aug 18, 2020 · 4 comments
Closed

💭 [lint] require await inside async functions #1118

ematipico opened this issue Aug 18, 2020 · 4 comments
Labels
A-Linter Area: linter enhancement New feature request or improvement to existing functionality L-JavaScript Langauge: JavaScript

Comments

@ematipico
Copy link
Contributor

ematipico commented Aug 18, 2020

Upstream rule: https://eslint.org/docs/rules/require-await

Basically I would like to propose a rule where we enforce the use of await if a function is marked with async.

Unfortunately with TypeScript we can bypass this check, so I think having a lint rule might be beneficial for us. I noticed this in one of the tests where the callback was marked as async (t) => but there was no await inside its implementation.

@ematipico ematipico added enhancement New feature request or improvement to existing functionality A-Linter Area: linter L-JavaScript Langauge: JavaScript labels Aug 18, 2020
@samccone
Copy link
Contributor

I would rework this rule to be a bit more nuanced, all async function calls should either be

  1. prefixed with an await
  2. consumed by another fn

for instance

ILovePromises(myAsyncFn());

That means that the following patterns would not error

await foo()
yay(foo);

but

foo()

would

@ljharb
Copy link

ljharb commented Aug 18, 2020

This eslint rule is horrifically bad and should never have existed in the first place. There are tons of benefits from using async function alone, and forcing use of await both can make code less readable and can force an unnecessary tick (via return await`, which is always an antipattern outside a try block)

Similarly, calling a sync function inside an async one is perfectly valid, and trying to force await usage on it will only serve to needlessly slow down the async function. Without type information, this is simply not something that’s safe to even attempt to lint.

@bitpshr
Copy link
Contributor

bitpshr commented Aug 18, 2020

If I'm understanding this proposal correctly, the intention of this rule is to mandate that async function bodies contain at least one await statement. This is tangentially related to - but different - than mandating how async functions are consumed, which is what I believe @samccone is referring to (which is a valid rule in my opinion, and one that we should track the creation of in another ticket.)

I agree with @ljharb and feel that a rule that mandates that async function bodies contain at least one await statement is too restrictive and could lead to unnecessarily-awaited synchronous statements. It's common to see async functions used for syntactical benefits, such as properly catching and propagating errors in an async function body to a consumer without explicit try / catch wrapping.

@ematipico
Copy link
Contributor Author

ematipico commented Aug 19, 2020

Thank you all for the constructive feedback! As suggested by @bitpshr I am going to open a new ticket to track the rule suggested by @samccone.

We can all agree that forcing an await inside an async function is a bad practice.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter enhancement New feature request or improvement to existing functionality L-JavaScript Langauge: JavaScript
Projects
None yet
Development

No branches or pull requests

4 participants