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

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

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

jorgezreik
Copy link

@jorgezreik jorgezreik commented Apr 8, 2024

Adds a new rule to require that a server actions (functions with the 'use server' directive) be async as specified by the server 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!

Copy link

codecov bot commented Apr 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ Current head 7f9a2bd differs from pull request most recent head 9d050e8. Consider uploading reports for the commit 9d050e8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3729      +/-   ##
==========================================
- Coverage   97.71%   97.59%   -0.13%     
==========================================
  Files         133      134       +1     
  Lines        9470     9479       +9     
  Branches     3469     3468       -1     
==========================================
- Hits         9254     9251       -3     
- Misses        216      228      +12     

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

Copy link
Member

@ljharb ljharb 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)

CHANGELOG.md Outdated Show resolved Hide resolved
configs/recommended.js Outdated Show resolved Hide resolved

<!-- 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.

@ljharb ljharb marked this pull request as draft April 8, 2024 19:25
@jorgezreik
Copy link
Author

jorgezreik commented Apr 8, 2024

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.

@jorgezreik jorgezreik requested a review from ljharb April 8, 2024 20:17
@jorgezreik jorgezreik marked this pull request as ready for review April 8, 2024 20:17
lib/rules/async-server-action.js Outdated Show resolved Hide resolved
lib/rules/async-server-action.js Outdated Show resolved Hide resolved
jorgezreik and others added 3 commits April 8, 2024 23:06
@jorgezreik jorgezreik requested a review from ljharb April 9, 2024 15:16
@jorgezreik jorgezreik requested a review from ljharb April 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 like eslint/require-await and typescript-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.

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

Successfully merging this pull request may close these issues.

None yet

2 participants