-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Configs: [return-await] Change default option from in-try-catch to always #10165
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
Comments
I'm +1, but note
|
🤔 It is kind of unfortunate that the rule has two different areas of use:
I wonder if should split the rule in two, so that the logical use could go into My issue with setting |
oh and - cc @kirkwaiblinger, since you've done a bunch with async/await rules, this is probably in your area 😄 |
TBH, we say this, but I don't fully agree. It's like saying that no-floating-promises is purely stylistic as long as you don't write functions that throw exceptions. The pattern being reported is always a bug vector, whether or not you think you're going to trigger the bug. While we have the luxury of being able to syntactically detect the usages of
👎
We could already do this with the "errorHandlingCorrectnessOnly" option in recommendedTypeChecked (which is right now the case in strictTypeChecked) and "always"/"in-try-catch" in stylisticTypeChecked.
I'd argue the opposite! 😁 Having worked in codebases with lots of async code using the I've tried to be concise, but I have plenty more thoughts and justifications for these opinions that I can produce if there's desire to dive deeper. |
I personally agree that Which I think is hampered a bit from the rule's docs:
Other than reducing ticks from 2 to 1 (a negligible performance change), do we have actual runtime benefits we can demonstrate from outside the error handling context? |
The way you phrased this makes it seem like improved error-handling is not enough on its own and we need some other, separate reason. But is that true? I think the point of the MDN article I linked in the OP is that, in JavaScript/TypeScript land, you don't know if any arbitrary function that you call will throw. Thus, you are always have to worry about errors. Thus, in order to improve the (potential) stack trace, you would always want to include |
By "error handling context" I mean what the rule's docs mean: specifically code inside a async function inner() {
throw new Error("Hi.");
}
async function middle() {
try {
return /* await */ inner();
} catch (error) {
console.log("Caught in middle:", error);
}
}
async function outer() {
return /* await */ middle();
}
await outer(); Outputs from un-commenting 0, 1, or 2 of the
Notice how whether |
Huh - apparently the cryptic |
Zooming out a bit, I think it's fair to say that it's a lot easier to justify that
Let's suppose for the sake of argument that we agree that
|
Had a chat with @JoshuaKGoldberg and it sounded like he was swayed. The understood plan of action was:
|
Before You File a Proposal Please Confirm You Have Done The Following...
Description
In the documentation for the
@typescript-eslint/return-await
rule, it says that using "in-try-catch" as opposed to "always" is purely a stylistic concern.However, on this mdn page, it explains in the "Improving stack trace" section that using the "in-try-catch" style results in a worse stack trace. Thus, if true, this is not purely a stylistic concern.
Is this accurate? If so, it seems a compelling reason to use "always" instead of "in-try-catch", and I propose that:
return-await
rule should be updated to reflect this andThe text was updated successfully, but these errors were encountered: