Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[New]async-server-action: Add rule to require that server actions be async#3729

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
jorgezreik wants to merge1 commit intojsx-eslint:master
base:master
Choose a base branch
Loading
fromjorgezreik:async-server-action

Conversation

jorgezreik
Copy link

@jorgezreikjorgezreik commentedApr 8, 2024•
edited
Loading

Adds a new rule to require that a server actions (functions with the'use server' directive) be async as specified by theserver actions spec. Suggests fixes for server actions that aren't async by adding the async keyword.

Note: It is my first time contributing so let me know if I missed anything!

@codecovCodecov
Copy link

codecovbot commentedApr 8, 2024•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.59%. Comparing base(a944aa5) to head(7f9a2bd).

❗Current head7f9a2bd differs from pull request most recent head4d1e087

Pleaseupload reports for the commit4d1e087 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@##           master    #3729      +/-   ##==========================================+ Coverage   94.47%   97.59%   +3.11%==========================================  Files         134      134                Lines        9613     9479     -134       Branches     3486     3468      -18     ==========================================+ Hits         9082     9251     +169+ Misses        531      228     -303

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

Copy link
Member

@ljharbljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I see some examples using single and double quotes - great! - but none using backticks. Can we add some? (whether it's supported or not)


<!-- end auto-generated rule header -->

Require Server Actions (functions with the `use server` directive) to be async, as mandated by the `use server` [spec](https://react.dev/reference/react/use-server).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

is this actually released yet?

despite vercel's usage of it prior to it being fully released, i don't think we should ship a rule until that's the case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

So the feature is still in canary. I understand not wanting to ship a rule for a canary feature, and if that's the final policy then I'm happy to wait until it's out of canary to merge this in.

That being said, Server Components are definitely seeing widespread use, mainly through Vercel, with less stable implementations elsewhere. Additionally, the React team confirmed that they're officially shipping the feature with React 19 (when that comes out is anyone's guess).

Because of this, I think adding this rule (as an optional rule outside of the recommended config) would be very helpful to users using server components, while not impacting users who are not yet using them.

@ljharbljharb marked this pull request as draftApril 8, 2024 19:25
@jorgezreik
Copy link
Author

jorgezreik commentedApr 8, 2024•
edited
Loading

I see some examples using single and double quotes - great! - but none using backticks. Can we add some? (whether it's supported or not)

I knew I'd miss a few things 😅 Thanks for the feedback, really appreciate it!

Backticks don't work with use server, but great idea to test them. Adding now.

ljharb reacted with thumbs up emoji

@jorgezreikjorgezreik requested a review fromljharbApril 8, 2024 20:17
@jorgezreikjorgezreik marked this pull request as ready for reviewApril 8, 2024 20:17
@jorgezreikjorgezreik requested a review fromljharbApril 9, 2024 15:16
@jorgezreikjorgezreik requested a review fromljharbApril 10, 2024 23:11
@jorgezreik
Copy link
Author

A semi-related question that came to mind that I'd like advice on:

Other ESLint rules likeeslint/require-await andtypescript-eslint/require-await report async functions that do not have an await operator as incorrect. Any ideas on how we can make rules like these not report on"use server"? Adding options to these rules to ignore functions with certain directives would do the trick, but an option like that is React-specific enough that it's unlikely to get merged.

@ljharb
Copy link
Member

Therequire-await rules are harmful and bad and should never have existed in the first place. Anasync function without anyawaits is perfectly acceptable (unlike a generator without ayield).

As such, the solution there is to just disable those horrifically foolish rules.

nchevsky reacted with thumbs down emoji

@ljharbljharbforce-pushed theasync-server-action branch from9d050e8 to4d1e087CompareJune 1, 2024 01:12
Copy link
Member

@ljharbljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Rebased. I'd love to see lots more test cases, and I'm not going to merge this until the feature is available in a non-canary full release, so I'll mark it as a draft until then.

@ljharbljharb marked this pull request as draftJune 1, 2024 01:13
@ljharbljharb removed the question labelJun 1, 2024
@ljharbljharbforce-pushed themaster branch 2 times, most recently from380e32c to51d342bCompareJuly 4, 2024 15:25
@jorgezreik
Copy link
Author

Rebased. I'd love to see lots more test cases, and I'm not going to merge this until the feature is available in a non-canary full release, so I'll mark it as a draft until then.

Makes sense to me! When the full release rolls around we can spruce up the test cases and get this merged :)

@jorgezreik
Copy link
Author

Hi! React 19launched as stable today.

Are we still interested in getting this merged? If so we can fix merge conflicts and spruce up tests to do so!

@ljharb
Copy link
Member

@jorgezreik yep, let's do those things :-) mark it as ready for review when you think it's so

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ljharbljharbljharb approved these changes

Assignees
No one assigned
Labels
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@jorgezreik@ljharb
[8]ページ先頭
©2009-2025 Movatter.jp