Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement: [return-await] Option for unopinionated "in-try-catch" #9030

Open
4 tasks done
kirkwaiblinger opened this issue Apr 29, 2024 · 4 comments
Open
4 tasks done
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look

Comments

@kirkwaiblinger
Copy link
Member

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/return-await

Description

The current in-try-catch option handles two separate concerns

  • Correctness - requiring await where it impacts control flow (due to try-catch-finally)
  • Stylistic - forbidding await elsewhere

I would propose for there to be a rule option that handles only the correctness portion. That is to say, it should enforce await wherever in-try-catch currently does, but not report where in-try-catch forbids await.

Fail

// same as in-try-catch
async function f() {
  try {
    return returnsPromise();
  } catch { }
}

Pass

async function f() {
   return returnsPromise();
}

async function f() {
   return await returnsPromise();
}

// same as in-try-catch
async function f() {
  try {
    return await returnsPromise();
  } catch { }
}

Additional Info

I think this would be valuable in order to be able to recommend a less disruptive version of the rule in a preset, see #8667

@kirkwaiblinger kirkwaiblinger added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look enhancement: plugin rule option New rule option for an existing eslint-plugin rule labels Apr 29, 2024
@kirkwaiblinger
Copy link
Member Author

I'm not quite sure what to name this option, since, unfortunately, in-try-catch is already in use to mean something different. Maybe in-try-catch-only? 🤷

@kirkwaiblinger
Copy link
Member Author

Note that this would also make life easier for users of using while #7889 is still unresolved

@bradzacher
Copy link
Member

bradzacher commented Apr 29, 2024

I'm a -1 here.

What does one gain by allowing return await? What does the laxness enable?

All I can personally see is a big downside - allowing the inconsistency creates a confusing code standard - "Why is this function return await, but this one isn't? What's the correct code to write?"

I don't think there are any users that are evaluating this rule and this option, and skipping it because it's technically got some stylistic implications. In fact I'd say the opposite - people like that it enforces strict consistency.

Consistency is good for style and correctness because it means people think less - allowing them to get it right more often. "never use return await unless it's in a try" is simple and easy to teach. But if people start seeing code that often violates that then it muddies the water.

@kirkwaiblinger
Copy link
Member Author

I don't feel strongly about it... I would personally only ever use "always".

What does one gain by allowing return await? What does the laxness enable?

This minimizes the "big diff bad" complaint when newly enabling something in a codebase, and differentiates between signal and noise (control flow changes vs consistency changes) when reviewing the changeset. Mostly, my thought is that this is a means to an end to make it palatable for the recommended-type-checked config, where we may be recommending it to existing mixed/always/never-except-for-error-handling codebases that were not previously linted.

I don't think there are any users that are evaluating this rule and this option, and skipping it because it's technically got some stylistic implications. In fact I'd say the opposite - people like that it enforces strict consistency.

Thus far, I just have existence proof of one person in the wild (and who isn't me 😄) who asked for this as a prerequisite for adoption.

Consistency is good for style and correctness because it means people think less - allowing them to get it right more often. "never use return await unless it's in a try" is simple and easy to teach. But if people start seeing code that often violates that then it muddies the water.

Agreed! Again, for which reason I'd use always. But, tbh I'd still use the proposed option as a stepping stone to make the diff safer.

Another approach here might be restructuring the options as
"affectsControlFlow": "always" | "never" | "dontCare",
"doesNotAffectControlFlow": "always" | "never" | "dontCare"
but that seems needlessly breaking 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look
Projects
None yet
Development

No branches or pull requests

2 participants