Proposal to disable no-return-await

[Jerska]

I have two issues with no-return-await.

When there are multiple awaits

no-return-await tells you to change

async function myFunc() {
  await anotherFunc();
  return await anotherFunc2();
}

into

async function myFunc() {
  await anotherFunc();
  return anotherFunc2();
}

This is OK, until you start doing a try...catch here:

async function myFunc() {
  try {
    await anotherFunc();
    return anotherFunc2();
  } catch (e) {
    // Catches rejections from `anotherFunc`, but not from `anotherFunc2`
  }
}

Here if you add await after the return again, eslint will not complain anymore.
But you've most of the time already removed it, and it doesn't jump to my eyes that anotherFunc2 errors won't be catched.

When there's only one await

For me, things are even worse with only one await.
no-return-await tells you to change

async function myFunc() {
  return await anotherFunc();
}

into

async function myFunc() {
  return anotherFunc();
}

which require-await tells you to change to:

function myFunc() {
  return anotherFunc();
}

Suddenly, you have the same issue than the previous one, plus this function isn't marked async anymore, but you still need to await it if you call it.
Now if you add a try...catch but don't know if anotherFunc is async, it's now even worse, because you have no indication whatsoever that there's async stuff going on there:

function myFunc() {
  try {
    return anotherFunc();
  } catch (e) {
    // Looks 100% legit, not even an `async` keyword to tip us off
  }
}

An alternative solution

There's another pattern where eslint won't complain:

async function myFunc() {
  const res = await anotherFunc();
  return res;
}

Now that I know about it, I'm thinking about using it everywhere I have such returns of non-explicit promises.

Drawback

I assume that:

async function myFunc() {
  return await anotherFunc();
}

will actually create a wrapper promise on top of the other one, which could be concerning performance-wise.
I also believe that this is a case that should be easily optimizable by the compiler though.
Finally, I believe that the better clarity outweighs clearly the creation of a useless Promise instance.

cc @vvo @Haroenv who I pinged on Slack :)

[Haroenv]

I totally agree, and very happy for this explanations, feel free to make aPR

[vvo]

Thanks for the detailed explanations!

I do use the

async function myFunc() {
  const res = await anotherFunc();
  return res;
}

pattern myself.

About:

async function myFunc() {
  try {
    await anotherFunc();
    return anotherFunc2();
  } catch (e) {
    // Catches rejections from `anotherFunc`, but not from `anotherFunc2`
  }
}

Would it be better to catch those exceptions at the call level of myFunc rather than inside myFunc? It ca be applied everywhere I guess but this may be what you want to do.

Finally, I believe that the better clarity outweighs clearly the creation of a useless Promise instance.

Do you mean that you would prefer to enforce the res = await.. pattern rather than removing those rules?

One of the goal of no-return-await I believe is to force people to really understand what's going on by doing return await.

Again, I have no strong opinions, you are the biggest await users here.

[Haroenv]

The annoying thing that disabling this rule would fix is that if you return await, there can only be promise rejections, without it the function can throw an error and reject. Allowing return await prevents that, so I'm in favour of removing the rule.

[vvo]

The annoying thing that disabling this rule would fix is that if you return await, there can only be promise rejections, without it the function can throw an error and reject. Allowing return await prevents that, so I'm in favour of removing the rule.

Can you precise what you mean

[Jerska]

I do use the

async function myFunc() {
  const res = await anotherFunc();
  return res;
}

pattern myself.

This pattern is definitely ok, and I would start using it more if we keep the rule.
But is this less clear for you?

async function myFunc() {
  return await anotherFunc();
}

I don't feel like we're losing any information here, while we do lose the async with:

function myFunc() {
  return anotherFunc();
}

This pattern is the one no-return-await encourages, because when I read that, I used to simply remove the await, then I would remove the async because require-await would now tell me it is useless.

---

Would it be better to catch those exceptions at the call level of myFunc rather than inside myFunc? It ca be applied everywhere I guess but this may be what you want to do.

My real-world code where this comes from is a wrapper which is supposed to send an error to sentry but not stop the execution if this fails.
The try...catch could indeed be put in the main function which calls the wrapper, but the advantage of the wrapper was that it was isolating some code relative to a specific feature, so this would a bit defeat the purpose.
I could however make a wrapper on top of the wrapper which role would be to only try...catch I guess. Maybe that's the best option.

---

The annoying thing that disabling this rule would fix is that if you return await, there can only be promise rejections, without it the function can throw an error and reject. Allowing return await prevents that, so I'm in favour of removing the rule.

Can you precise what you mean

I believe this is what @Haroenv meant:

let i = 0;

async function myAsyncFunction() {
  throw new Error('Error from async');
}

function myNonAsyncCallingAsyncFunction() {
  // Throws on its first call
  if (i++ === 0) throw new Error('Error from non async');
  return myAsyncFunction();
}

for (let j = 0; j < 2; ++j) {
  try {
    // Notice how it is not awaited
    myNonAsyncCallingAsyncFunction();
  } catch (e) {
    console.log(`Catch ${j} : `, e.message);
  }
}

process.on('unhandledRejection', e => console.log('Unhandled:', e.message));

Output:

Catch 0 :  Error from non async
Unhandled: Error from async

Basically, this try...catch will catch some exceptions but not others, because myNonAsyncCallingAsyncFunction is not async. Part of the errors are underlying rejections, others are actual errors.

If we add an await, both errors will be caught, but the errors are still of two completely different types. Whereas if myNonAsyncCallingAsyncFunction was also marked async all the errors it could output would be rejections.

---

Finally, I believe that the better clarity outweighs clearly the creation of a useless Promise instance.

Do you mean that you would prefer to enforce the res = await.. pattern rather than removing those rules?

What I would like even more would be the ability to do the opposite : prevent return myFunc() if myFunc() is a promise, and force return await myFunc();.
Basically, I'd like to enforce using await anywhere a promise is called, except if explicitely disabled because you know what you're doing.
This would make every single function which calls a promise have await keywords in them, forcing the user to also mark them async, so suddenly you would know exactly, in your whole codebase, which functions are async or not.

Unfortunately, this is not possible, since a linter cannot know whether or not myFunc is async / a promise. But removing no-return-await has the advantage of letting you do it yourself if you want to without having to use a specific pattern for it.

---

[Jerska]

Related:

• Add information to no-return-await docs about potential pitfalls. eslint/eslint#9695
• Rule Proposal: require-await eslint/eslint#6820 (comment)