feat(eslint-plugin): [return-await] add option to report in error-handling scenarios only, and deprecate "never" #9364

kirkwaiblinger · 2024-06-15T20:55:48Z

PR Checklist

Addresses an existing open issue:
- fixes Enhancement: [return-await] Option for unopinionated "in-try-catch" #9030
- fixes Deprecate return-await option "never" #9433
That issue was marked as ~~accepting prs~~ "team assigned"
Steps in Contributing were taken

Overview

Just adds an option to ignore return-await concerns outside of scenarios relevant for error-handling

If anyone has suggestions on a better name for the option, please let me know!

NOTE

This is targeted against main for now, but it may make sense to target v8 instead. Not because it's a breaking change, but because its only purpose for existing is in service of #8667, which is a breaking change (if we do retarget to v8, it may also make sense to adjust the configs in the same PR) Was decided to keep this target to v7/main

typescript-eslint · 2024-06-15T20:55:51Z

Thanks for the PR, @kirkwaiblinger!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

netlify · 2024-06-15T20:56:05Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`8cd6116`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/669955902e6d9c000844475b
😎 Deploy Preview	https://deploy-preview-9364--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 94 (no change from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 90 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

nx-cloud · 2024-06-15T20:56:52Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 8cd6116. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 31 targets

Sent with 💌 from NxCloud.

packages/eslint-plugin/docs/rules/return-await.mdx

JoshuaKGoldberg

packages/eslint-plugin/src/rules/return-await.ts

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

packages/eslint-plugin/src/rules/return-await.ts

kirkwaiblinger · 2024-06-25T03:15:08Z

packages/eslint-plugin/src/rules/return-await.ts

@@ -271,92 +314,69 @@ export default createRule({
      const type = checker.getTypeAtLocation(child);
      const isThenable = tsutils.isThenableType(checker, expression, type);


github's diffing gets real confused about what actually changed here if you don't use the "ignore whitespace" here. Be sure to use that.

JoshuaKGoldberg

Thinking on the docs 🤔

packages/eslint-plugin/docs/rules/return-await.mdx

JoshuaKGoldberg · 2024-06-25T04:15:48Z

packages/eslint-plugin/docs/rules/return-await.mdx

+|             always              |            `return await promise;`            |                     `return await promise;`                     |                          ✅ Yes!                           |
+|          in-try-catch           |               `return promise;`               |                     `return await promise;`                     |                          ✅ Yes!                           |
+| error-handling-correctness-only |                 don't care 🤷                 |                     `return await promise;`                     | 🟡 Okay to use, but the above options would be preferable. |
+|              never              |               `return promise;`               |                        `return promise;`                        |                           ❌ No!                           |


[Docs]

❌ No!

Well, if the option should never be used... why would we want to include it at all? It's not clear from this table or the ## `never` section why we have that. Maybe a solution to this would be to mention that never is only included for compatibility with the base rule?

Documenting a private conversation for visibility:

never is only included for compatibility with the base rule

There is no evidence that this is or ever was the case; see #994.

Plan to resolve this comment: tentative plan to deprecate 'never' as part of this PR. Getting that process started imminently.

filed #9433

Incorporated #9433 into this PR

JoshuaKGoldberg · 2024-06-25T04:19:22Z

packages/eslint-plugin/docs/rules/return-await.mdx

+### Options Summary
+
+|             Option              | Ordinary Context <br/> (stylistic preference) | Error Handling Context <br/> (catches subtle control flow bugs) |                       Should I use?                        |


[Docs] I think this table is pretty useful. Proposal: how about moving this to within the ## Options early on, to prime folks with it before they dive into the options?

I also think it would be useful to mention in each option's section the pros & cons of that option. Right now, the always section just says "Requires that all returned promises are awaited.". It doesn't mention the option being preferred, or why it's preferred.

Proposal: how about moving this to within the ## Options early on, to prime folks with it before they dive into the options?

Yeah, that was actually my first approach, but i was waffling on how to go about defining an error-handling context since that's required before the table... and I went with this approach out of laziness. I agree that putting it first would be better, though, so I'll give it another stab 🙂

I also think it would be useful to mention in each option's section the pros & cons of that option. Right now, the always section just says "Requires that all returned promises are awaited.". It doesn't mention the option being preferred, or why it's preferred.

Feels like we're venturing into scope creep territory, but at the same time, we probably don't want this option to go out without good supporting docs 🤔 Will try to incorporate this 👍

I made changes in this area. I definitely think the result is much better! Thanks for making the suggestions.

packages/eslint-plugin/docs/rules/return-await.mdx

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

Josh-Cena · 2024-06-27T19:48:16Z

packages/eslint-plugin/docs/rules/return-await.mdx

+Same as `in-try-catch`, except _only_ enforces Promises be awaited within the
+error handling contexts explained under that option. Outside of error-handling
+contexts, returned Promises are ignored, whether awaited or not.


Don't wrap lines by line width

Huh. not sure how that happened. In any case this has been fixed/superseded 👍

phaux · 2024-07-06T12:04:25Z

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

packages/eslint-plugin/docs/rules/return-await.mdx

codecov · 2024-07-08T08:21:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (dd965a4) to head (8cd6116).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9364      +/-   ##
==========================================
- Coverage   88.40%   88.40%   -0.01%     
==========================================
  Files         422      422              
  Lines       14652    14650       -2     
  Branches     4287     4285       -2     
==========================================
- Hits        12953    12951       -2     
  Misses       1374     1374              
  Partials      325      325

Flag	Coverage Δ
unittest	`88.40% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
packages/eslint-plugin/src/rules/return-await.ts	`97.39% <100.00%> (-0.05%)`	⬇️

Josh-Cena · 2024-07-12T00:34:12Z

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" }.

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.

packages/eslint-plugin/docs/rules/return-await.mdx

JoshuaKGoldberg

Whoohoo! Happy to finally have this thing modernized! 👏 🔥

packages/eslint-plugin/docs/rules/return-await.mdx

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

Josh-Cena

Two optional nits; otherwise 🛳

packages/eslint-plugin/docs/rules/return-await.mdx

packages/eslint-plugin/src/rules/return-await.ts

first stab

1384bf5

kirkwaiblinger changed the title ~~enhancement(eslint-plugin): [return-await] add option to report in error-handling scenarios only~~ feat(eslint-plugin): [return-await] add option to report in error-handling scenarios only Jun 15, 2024

fix CI stuff

bc8310f

JoshuaKGoldberg reviewed Jun 17, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/return-await.mdx Show resolved Hide resolved

JoshuaKGoldberg previously approved these changes Jun 18, 2024

View reviewed changes

packages/eslint-plugin/src/rules/return-await.ts Outdated Show resolved Hide resolved

Update packages/eslint-plugin/src/rules/return-await.ts

7c5340f

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

kirkwaiblinger dismissed JoshuaKGoldberg’s stale review via 7c5340f June 18, 2024 02:43

kirkwaiblinger added 2 commits June 17, 2024 21:06

change comments and stuff

624eeee

Merge branch 'main' into return-await-unopinionated

68d8c5b

kirkwaiblinger commented Jun 18, 2024

View reviewed changes

packages/eslint-plugin/src/rules/return-await.ts Show resolved Hide resolved

kirkwaiblinger added 3 commits June 24, 2024 20:14

Merge branch 'main' into return-await-unopinionated

845812e

add table to docs

93c66be

dry it up

59192fc

kirkwaiblinger requested a review from JoshuaKGoldberg June 25, 2024 03:12

kirkwaiblinger commented Jun 25, 2024

View reviewed changes

revert useAutoFix

c2bac5c

JoshuaKGoldberg requested changes Jun 25, 2024

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jun 25, 2024

kirkwaiblinger and others added 2 commits June 24, 2024 23:04

Update packages/eslint-plugin/docs/rules/return-await.mdx

5150439

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

to awaiting

d050fe4

kirkwaiblinger mentioned this pull request Jun 25, 2024

Deprecate return-await option "never" #9433

Closed

Josh-Cena reviewed Jun 27, 2024

View reviewed changes

kirkwaiblinger commented Jul 8, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/return-await.mdx Show resolved Hide resolved

kirkwaiblinger added 2 commits July 8, 2024 01:42

changes and deprecation

c12dfeb

mention motivation for each option in its section

5ea1b81

kirkwaiblinger mentioned this pull request Jul 8, 2024

Docs: treat return-await as a standalone rule, not an extension #9517

Closed

2 tasks

subjunctive

584aad9

kirkwaiblinger requested a review from JoshuaKGoldberg July 8, 2024 08:06

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Jul 8, 2024

phaux mentioned this pull request Jul 12, 2024

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

Closed

4 tasks

kirkwaiblinger changed the title ~~feat(eslint-plugin): [return-await] add option to report in error-handling scenarios only~~ feat(eslint-plugin): [return-await] add option to report in error-handling scenarios only, and deprecate "never" Jul 14, 2024

kirkwaiblinger added this to the 8.0.0 milestone Jul 15, 2024

JoshuaKGoldberg reviewed Jul 16, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/return-await.mdx Show resolved Hide resolved

JoshuaKGoldberg previously approved these changes Jul 16, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/return-await.mdx Outdated Show resolved Hide resolved

kirkwaiblinger dismissed JoshuaKGoldberg’s stale review via 3892d64 July 16, 2024 21:25

Update packages/eslint-plugin/docs/rules/return-await.mdx

b27b673

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

kirkwaiblinger force-pushed the return-await-unopinionated branch from 3892d64 to b27b673 Compare July 16, 2024 21:27

Merge branch 'main' into return-await-unopinionated

14f801b

kirkwaiblinger requested review from JoshuaKGoldberg and a team July 16, 2024 21:29

Merge branch 'main' into return-await-unopinionated

9309769

JoshuaKGoldberg previously approved these changes Jul 17, 2024

View reviewed changes

Josh-Cena previously approved these changes Jul 18, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/return-await.mdx Show resolved Hide resolved

packages/eslint-plugin/src/rules/return-await.ts Outdated Show resolved Hide resolved

kirkwaiblinger added 2 commits July 18, 2024 11:48

move things out of closure

f5a4060

Merge branch 'main' into return-await-unopinionated

8cd6116

kirkwaiblinger dismissed stale reviews from Josh-Cena and JoshuaKGoldberg via 8cd6116 July 18, 2024 17:49

kirkwaiblinger requested review from Josh-Cena and JoshuaKGoldberg July 18, 2024 17:49

Josh-Cena approved these changes Jul 18, 2024

View reviewed changes

kirkwaiblinger merged commit 970f3f1 into typescript-eslint:main Jul 18, 2024
65 checks passed

kirkwaiblinger deleted the return-await-unopinionated branch July 18, 2024 18:16

github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): [return-await] add option to report in error-handling scenarios only, and deprecate "never" #9364

feat(eslint-plugin): [return-await] add option to report in error-handling scenarios only, and deprecate "never" #9364

kirkwaiblinger commented Jun 15, 2024 •

edited

Loading

typescript-eslint bot commented Jun 15, 2024

netlify bot commented Jun 15, 2024 •

edited

Loading

nx-cloud bot commented Jun 15, 2024 •

edited

Loading

JoshuaKGoldberg left a comment

kirkwaiblinger Jun 25, 2024 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg Jun 25, 2024

kirkwaiblinger Jun 25, 2024 •

edited

Loading

kirkwaiblinger Jun 25, 2024

kirkwaiblinger Jul 8, 2024

JoshuaKGoldberg Jun 25, 2024

kirkwaiblinger Jun 25, 2024

kirkwaiblinger Jul 8, 2024

Josh-Cena Jun 27, 2024

kirkwaiblinger Jul 8, 2024

phaux commented Jul 6, 2024 •

edited

Loading

codecov bot commented Jul 8, 2024 •

edited

Loading

Josh-Cena commented Jul 12, 2024

JoshuaKGoldberg left a comment

Josh-Cena left a comment

		@@ -271,92 +314,69 @@ export default createRule({
		const type = checker.getTypeAtLocation(child);
		const isThenable = tsutils.isThenableType(checker, expression, type);

		### Options Summary

		\| Option \| Ordinary Context <br/> (stylistic preference) \| Error Handling Context <br/> (catches subtle control flow bugs) \| Should I use? \|

feat(eslint-plugin): [return-await] add option to report in error-handling scenarios only, and deprecate "never" #9364

feat(eslint-plugin): [return-await] add option to report in error-handling scenarios only, and deprecate "never" #9364

Conversation

kirkwaiblinger commented Jun 15, 2024 • edited Loading

PR Checklist

Overview

NOTE

typescript-eslint bot commented Jun 15, 2024

netlify bot commented Jun 15, 2024 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Jun 15, 2024 • edited Loading

☁️ Nx Cloud Report

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

kirkwaiblinger Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirkwaiblinger Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phaux commented Jul 6, 2024 • edited Loading

codecov bot commented Jul 8, 2024 • edited Loading

Codecov Report

Josh-Cena commented Jul 12, 2024

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Josh-Cena left a comment

Choose a reason for hiding this comment

kirkwaiblinger commented Jun 15, 2024 •

edited

Loading

netlify bot commented Jun 15, 2024 •

edited

Loading

nx-cloud bot commented Jun 15, 2024 •

edited

Loading

kirkwaiblinger Jun 25, 2024 •

edited

Loading

kirkwaiblinger Jun 25, 2024 •

edited

Loading

phaux commented Jul 6, 2024 •

edited

Loading

codecov bot commented Jul 8, 2024 •

edited

Loading