Rule proposal: prefer-await

[yvele]

The rule name may be named unicorn/prefer-await.

This only applies for ES6 code that have the await keyword available.

Prefer await over Promise#then(), Promise#catch() and Promise#finally()

Prefer using await operator over Promise#then(), Promise#catch() and Promise#finally(), which is easier to write and to read afterwards.

This rule is fixable.

Fail

await doSomething().then(() => {
  // …
});

doSomething().then(() => {
  // …
});

await doSomething().catch((err) => {
  // …
});

doSomething().catch((err) => {
  // …
});

Pass

 await doSomething();

try {
  await doSomething();
} catch (err) {
  // …
}

[fisker]

How about no-promise-catch.

Second thought, if we forbid .catch, why not .then? So maybe prefer-await?

[yvele]

Yeah right prefer-await fits better and will forbid using .then(), .catch() and .finally() methods.

[fisker]

There is one rule called prefer-await-to-then.

There is also proposal in eslint project, eslint/eslint#9649 and also a PR eslint/eslint#11962 😄

[yvele]

There is one rule called prefer-await-to-then .

Sure but the repo looks a bit abandoned.

There is also some issues with this rule 😅 like this one. Or that fact that it only catches .then() and not .catch() used alone. Also the rule is not fixable.

There is also proposal in eslint project, eslint/eslint#9649 and also a PR eslint/eslint#11962 😄

Looks like they abandoned this rule in to be integrated in core ESLint rule for performance control reasons 🤔

[sindresorhus]

I'm 👍 on adding a prefer-await rule here. @fisker ?

[fisker]

I like this, remember I was looking for similar rule before. We need discuss details.

1. Do we supply auto fix? I hate rewrite those promises.

2. If not inside asyc functions, how do we handle?We can't change function to async, since old function could return both Promise and other types.

3. top level await? Behind option?

[sindresorhus]

1. We could do so for the simple cases, but it's not important to me as all my code is already using only async/await and I only need this rule to enforce so for new contributions.
2. We cannot fix then, but we should still report. We could maybe add a suggestion (suggestions API) to add the async keyword and then fix, but not sure it's worth the effort.
3. Nah, we can open an issue about it and implement it when Node.js stable supports top-level await.

[fisker]

I tried a little bit to implement this rule, starting on fixing .then, really complicated... so far

[fisker]

promise.then(onFulfilled, onRejected);

Shouldn't fix to

try {
  onFulfilled(await promise);
} catch (error) {
  onRejected(error);
}

onRejected should not handle onFulfilled errors, right?

So, the following one is right?

let result;
try {
  result = await promise;
} catch (error) {
  onRejected(error);
}
onFulfilled(result);

[yvele]

@fisker maybe the rule should not fix that kind of case (that requires a new variable).. and the rule should be partially fixable.

Note that this is not an acceptable fix because we don't want to catch onFulfilled:

try {
  onFulfilled(await promise); // No good, we don't want to catch `onFullfilled`
} catch (error) {
  onRejected(error);
}

[fisker]

I have lots of legacy codes need rewrite from my work, I want a fix so much.

[yvele]

Yeah but what if a result variable already exists on the same scope? Should this scenario be the limit of what is fixable? I'm not sure generating incremental variable names is a good idea (e.g. result2 😱 )

[fregante]

This exists in eslint-plugin-promise and works well!

https://github.com/xjamundx/eslint-plugin-promise/blob/development/docs/rules/prefer-await-to-then.md

[fisker]

Reopening this, since eslint-plugin-promise don't have autofix. I'm going to give another shot on it.

[papb]

Hi @fisker, you might be interested in https://github.com/codemodsquad/asyncify by @jedwards1211 - it was used to convert the whole codebase of https://github.com/sequelize/sequelize from Bluebird promises to async-await.

[fisker]

Thanks, I'll take a look.

[jedwards1211]

It would probably be the most complicated autofix ever 🤣
Even my codemod has limitations, esp if there's a lot of returning branches inside promise handlers. And still it's tons of code

[jedwards1211]

Also I never implemented support for Flow or TS type annotations in my codemod, which would probably be a minimum requirement for an eslint autofix. Would probably take a lot more work to fully support them

[fisker]

Thanks for you information, asyncify looks great, do you think it's possible to covert your work to ESLint rule?

[jedwards1211]

Hmmm, actually type annotations aren't a huge problem, I just fixed an issue with catch error type annotations, but the argument type annotations already get transferred by asyncify during unrolling...

If you mean convert to an ESLint autofix, I'm sure it's possible, but the main obstacle is asyncify uses Babel API everywhere, which isn't the default eslint parser. I have no idea how eslint rules and autofixes are written to work with a variety of parsers. I'm happy to keep fixing issues with asyncify but I wouldn't have time to convert it to an eslint autofix...IMO it's better not to have an autofix for this and rely on an external tool like asyncify to autofix, since autofixing is a very involved and fraught process. I still recommend looking over the output of asyncify, for example sometimes it accidentally deletes comments, and it was failing entirely on catch error type annotations until I fixed that 15min ago...

Plus, people might have preferences for what the autofix does. For example, not everyone would want to unroll oneliners like return foo.catch(() => null) to five whole lines:

try {
  return await foo
} catch (err) {
  return null
}

So I'm not sure at autofix would satisfy everyone. asyncify already has an ignoreChainsShorterThan option. I guess that could be an eslint option too, but I imagine the lint check would have to support options like that as well.

[jedwards1211]

Another consideration: a lot of code has to get moved around in an autofix. I used recast to try to preserve as much formatting as possible on code that gets moved. I assume an eslint autofix would preserve less formatting, but I don't really know.

[fisker]

Thanks a lot, I think maybe we can start adding auto-fix for some simple case, we don't have to handle all cases, as it will still be reported.

[jedwards1211]

Oh and @papb thanks for the shout-out!

[papb]

@jedwards1211 And thank you for all that hard work 😁

[yvele]

For example, not everyone would want to unroll oneliners like return foo.catch(() => null) to five whole lines

I personally prefer the 5 whole lines as I want developers to catch specific errors and also add comments about why the error needs to be caught. Catching an error is something that should be done only for specific scenarios that needs to be documented and well controlled. That's why we have the catch-error-name rule.

try {
  return await foo
} catch (err) {
  if (err.name === "bar") {
    // Error is swallowed because blabla.
    return null;
  }
  throw err;
}

In my opinion code like return foo.catch(() => null) reflects more conceptual issues then styling issues.

[jedwards1211]

@yvele even if you do handling like that in a catch callback, I still prefer the catch callback sometimes, because try/catch is so ungainly to use. Especially if you have to hoist a variable declaration outside of the try block, it's pretty terrible (and gets even more cumbersome tamping down Flow errors on the hoisted variable, since it can't be const. I forget if TS is like that with mutable variables, but it complicates the type declarations...)

[jedwards1211]

With .catch you can at least keep a clean variable declaration:

const result: Foo | null = await foo.catch((err: Error) => {
  if (err.name === "bar") {
    // Error is swallowed because blabla.
    return null;
  }
  throw err;
})

[fregante]

I agree that await foo.catch(x => undefined) is a great way to ignore errors or reassign them to something non-throwing. try/catch can be noisy for that

let result: Foo;
try {
  result = await foo;
} catch(err: unknown) {
  if (err.name !== "bar") {
    // Error is swallowed because blabla.
    throw err;
  }
}