Skip to content

Enhancement: [return-await] change rule option to object #9546

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

Closed
4 tasks done
phaux opened this issue Jul 12, 2024 · 5 comments
Closed
4 tasks done

Enhancement: [return-await] change rule option to object #9546

phaux opened this issue Jul 12, 2024 · 5 comments
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on

Comments

@phaux
Copy link
Contributor

phaux commented Jul 12, 2024

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

From #9030 (comment)

Another approach here might be restructuring the options as

"affectsControlFlow": "always" | "never" | "dontCare",  
"doesNotAffectControlFlow": "always" | "never" | "dontCare"  

but that seems needlessly breaking 🤷

and #9364 (comment)

Maybe change the option so that instead of a string it's an object:

{
  default: "always" | "never" | "ignore", 
  inTryCatch?: "always" | "never" | "ignore", // undefined means "same as default" 
}

And make the string options aliases for backward compatibility:

  • "always" - { default: "always" }
  • "in-try-catch" - { default: "never", inTryCatch: "always" }
  • "never" - { default: "never" }

The new option would be { default: "ignore", inTryCatch: "always" }

In the future, there could be more conditions added. For example, someone might want to use "always" but make an exception for single-line arrow functions to make them less noisy:

/* eslint @typescript-eslint/promise-function-async: "error" */
/* eslint @typescript-eslint/return-await: ["error", { default: "always", inArrowFunction: "ignore" }] */

promise.then(async (v) => doSomethingAsync(v)) // allowed

Fail

// no changes

Pass

// no changes

Additional Info

No response

@phaux phaux added 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 team members to take a look labels Jul 12, 2024
@phaux
Copy link
Contributor Author

phaux commented Jul 12, 2024

Reply to @Josh-Cena

I thought about the object API (which I mentioned in the linked issue), but I now don't like it. It creates a lot of "dubious combinations", for example you would never want { default: "always", inTryCatch: "never" }.

Then remove the possibilities that don't make sense from the types (assuming "never" is deprecated):

type Options = "always" | {
  default: "always" | "ignore" | "never",
  // inTryCatch: "always", // not even necessary because you can't disable it
  inArrowFunctionShorthand?: "ignore" | "never",
}

In the future, there could be more conditions added. For example, someone might want to use "always" but make an exception for single-line arrow functions to make them less noisy:

You should either not mark it as async (which is even less noisy), or add the await. I don't think we should accommodate that style as that's not really the spirit of this rule.

This was just an example of how it would look if the rule had to be extended again.

But if we assume we never want another modifier then all we are left with is just:

type Options = 
  | "always"
  | { default: "ignore", /* inTryCatch: "always" */ }
  | { default: "never", /* inTryCatch: "always" */ }

Which could be simplified to just "always" | "ignore-or-in-try-catch" | "never-or-in-try-catch"
or just "always" | "prefer-ignore | "prefer-never".

@kirkwaiblinger maybe use one of these names in your PR? And even make an alias for the current "in-try-catch"

@kirkwaiblinger
Copy link
Member

TLDR; IMO we should not change the rule structure unless/until there are other concrete options added to the rule.


The way I, personally, see the rule for now is that, after #9364, it should always enforce return await in error handling scenarios.

We have two stylistic choices

  • prefer return await promise ("always")
  • prefer return promise ("in-try-catch")

As for the other options, we have

  • a stylistically unopinionated config which is a concession in order to be able to add the rule to a preset ("error-handling-correctness-only"), but not something we actually want people to use.
  • A deprecated, IMO harmful option, which we can omit from this conversation ("never")

So, that gives us three options states that we need to think about. I do not believe that there is a benefit to restructuring to an options object to support just those options.

If a concrete proposal for an option is created and accepted, I think that switching to an object-based options structure (with aliases to avoid breaking changes) may well be a viable solution. But IMO that needs to exist first, before it's worth considering this issue.

@kirkwaiblinger
Copy link
Member

(I acknowledge that my current position may appear at odds with some of my previous comments referenced in the issue report. The context around why I considered that idea at the time has changed, and so has my opinion. I don't mean to pull the rug out from under @phaux 🙂)

@Josh-Cena
Copy link
Member

I agree. The current three choices: always, in-try-catch, in-try-catch-only (?) correspond to default: "always" | "never" | "ignore", but the whole premise of this proposal is that people want to handle concise arrows differently, and that needs to be proposed separately, as I don't think that's a nice convention to enforce.

@kirkwaiblinger
Copy link
Member

Closing for now, but I would invite this to be reopened/refiled if a concrete option proposal is created as discussed.

@kirkwaiblinger kirkwaiblinger closed this as not planned Won't fix, can't repro, duplicate, stale Jul 12, 2024
@kirkwaiblinger kirkwaiblinger added wontfix This will not be worked on and removed triage Waiting for team members to take a look labels Jul 12, 2024
@github-actions github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Jul 20, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants