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

chore: let @typescript-eslint/require-await ESLint rule pass #7123

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Aug 10, 2023

Sibling PR to @ngbrown's excellent #6325 -> #6331. I noticed @typescript-eslint/require-await wasn't in the list, and it's in the new plugin:@typescript-eslint/recommended-type-checked preset.

@typescript-eslint/require-await disallows async functions which have no await expression. This is nice because async adds an extra bit of work to the function (returns a Promise instead of a direct value).

Separately, I've also noticed a trend in the React ecosystem of APIs requiring functions return a Promise even when they wouldn't always want to. That's in direct conflict with this rule. Enabling the rule in frameworks helps ensure frameworks aren't in conflict: by either making necessary changes to the rule and/or blocking conflict-y things in the frameworks.

@changeset-bot
Copy link

changeset-bot bot commented Aug 10, 2023

⚠️ No Changeset found

Latest commit: 1b26a0d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@brophdawg11 brophdawg11 added feat:typescript package:eslint-config feat:dx Issues related to the developer experience labels Aug 10, 2023
@JoshuaKGoldberg
Copy link
Contributor Author

Err, I can't tell if the CI failures on Firefox are a result of the changes in this PR. #7133 shows similar failures. Marking as ready for review on a hunch that they're unrelated.

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review August 12, 2023 19:58
@MichaelDeBoey MichaelDeBoey force-pushed the typescript-eslint-require-await branch from d8c0e5c to f705613 Compare August 15, 2023 17:27
@brophdawg11
Copy link
Contributor

This seems ok to me but I'm going to defer to @pcattori for an approval because it touches a lot of stuff in the dev server

@brophdawg11 brophdawg11 removed their request for review August 21, 2023 15:01
@pcattori
Copy link
Contributor

pcattori commented Sep 18, 2023

Hey @JoshuaKGoldberg , thanks for your work on this! This PR went up right when we started focusing on shipping v2, so hasn't yet gotten the attention it deserves. But hoping to change that now!

One thing that I don't like about this new lint rule is that it feels like the function signature/interface is now coupled to its implementation. So let's say we find a way to make an async function not need any awaits, so we remove the async. But now that's a breaking change for users. So then I can keep async, but this lint rule will make noise about that.

I think there are also times where I want to model something as potentially being async, knowing that my implementation of it may change day-to-day.

But it is on plugin:@typescript-eslint/recommended-type-checked and I probably haven't thought through this as deeply as those folks.

tl;dr: If you want to rebase this PR on latest main, I'd be ok with merging it even though I'm not thoroughly convinced about its value add since it doesn't cost much. But happy to be convinced otherwise if there are compelling reasons here that I'm not seeing.

@JoshuaKGoldberg JoshuaKGoldberg force-pushed the typescript-eslint-require-await branch 2 times, most recently from 1e75f10 to f705613 Compare September 18, 2023 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants