Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

New Rule: no async without await #3937

Closed
eranshabi opened this issue Jun 3, 2018 · 15 comments · Fixed by #3945
Closed

New Rule: no async without await #3937

eranshabi opened this issue Jun 3, 2018 · 15 comments · Fixed by #3945

Comments

@eranshabi
Copy link
Contributor

eranshabi commented Jun 3, 2018

We wrote a rule which seems useful enough to go into TSLint.
The rule disallows async functions from not having await inside them, as from our experience the async modifier is almost always useless and the function could be a synchronous one. It will also cause TypeScript to compile the function into something much longer than a simple function.

If this rule seems useful enough, we would like to PR it.

passing example:

async function myFunc() {
    await fetch('...');
}

failing example:

async function myFunc(a) {
    console.log(a);
}
@Shinigami92
Copy link
Contributor

I think it's ok for warning the developer, but I want to leave this here:

"An async function can contain an await expression that pauses the execution of the async function and waits for the passed Promise's resolution, and then resumes the async function's execution and returns the resolved value."

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function

https://stackoverflow.com/a/45594870/6897682

@eranshabi
Copy link
Contributor Author

If you couldn't use async without await it was the compiler job to disallow this. In this case you will have an unhandled promise that will be resolves to undefined that you ignore in your code;

@giladgray
Copy link

this seems like a fair linter rule as async without await is technically possible but may be stylistically undesirable.

@eranshabi go ahead with a PR!

@iluuu1994
Copy link

iluuu1994 commented Dec 3, 2018

Only issue I see with this:

async takesFnReturningPromise(closure: () => Promise<void>) {
    // ...
    await closure();
    // ...
}

await takesFnReturningPromise(async () => {
    // no await
});

The closure should still be async because takesFnReturningPromise expects it to return a promise.

@willklein
Copy link

I ran into the scenario @iluuu1994 pointed out. This rule would still be valuable but I wonder if there's a way to handle this exception, if so I think it would requiring typing to be enabled.

If it can't be handled, there is a workaround to use this rule and get rid of async when you need to return a promise: the function can return Promise.resolve() whatever value it is otherwise returning. In my code base I opted to leave the async in many cases because it works fine as a way to promisify the return value of a function.

@gms1
Copy link

gms1 commented Jan 5, 2019

The 'promise-function-async' rule requires that any function or method that returns a promise be marked as 'async', even if it is guaranteed that the function/method will always return a promise.

e.g.

async bar (): Promise<void>;

// tslint: disable-next-line: promise-function-async
function foo (): Promise <void> {return bar (); }

or

// tslint: disable-next-line: promise-function-async
function foo (): Promise <void> {return new Promise((resolve, reject) => {....}); }

and the new rule 'no-async-without-await' requires any function or method without 'await' to be synchronous (non-async), even if there is no guarantee that the function/method will always return a promise.

e.g.

// tslint: disable-next-line: no async-without-await
async function foo (): Promise <void> {
  return callSomethingThatCanThrow ();
}

Hopefully these two rules will eventually be combined into one useful rule in the future

@hen-x
Copy link

hen-x commented Mar 13, 2019

There's a valid reason to use async even when the function never uses await. If an error gets thrown inside an async function, the function will automatically return a rejecting promise, rather than throwing the error to its consumer. This is more consistent, since there is only one error path for the consumer to handle, rather than two.

@a7madgamal
Copy link

isn't this what you guys are looking for?
https://palantir.github.io/tslint/rules/no-floating-promises/

@gms1
Copy link

gms1 commented Mar 19, 2019

@a7madgamal
not really
This is about two rules regarding 'async' that contradict each other and have no relation to reality

@Tehnix
Copy link

Tehnix commented Mar 26, 2019

@eranshabi do you have this rule anywhere? We'd really like to use it, and if it's not gonna be merged in here, we'd just run it from a fork.

@eranshabi
Copy link
Contributor Author

@eranshabi do you have this rule anywhere? We'd really like to use it, and if it's not gonna be merged in here, we'd just run it from a fork.

You can use it from here: https://github.com/wix-incubator/wix-tslint-custom-rules

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Mar 26, 2019

Hey folks - this issue is marked as Accepting PRs. If someone wants to implement the rule in this repository, we'd be happy to take it in! @eranshabi would you be willing to send a PR? 😄

A couple of notes on the discussion:

  • This is a different piece of logic than promise-function-async, though the two are related. It's generally better to have two small rules than one medium-sized one, so folks can more easily enable, disable, or reconfigure them.
  • Yes, there are valid reasons to use async without await. If your code base is littered with them, this rule is not for you.

@eranshabi
Copy link
Contributor Author

eranshabi commented Mar 26, 2019

I already did a PR 6 months ago: #3945
would be great if someone can merge it.

@JoshuaKGoldberg
Copy link
Contributor

👍 posted some comments on the PR @eranshabi

@JoshuaKGoldberg
Copy link
Contributor

FYI folks - this will be available in the next TSLint release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants