-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
There was a problem hiding this 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). |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
A semi-related question that came to mind that I'd like advice on: Other ESLint rules like |
The As such, the solution there is to just disable those horrifically foolish rules. |
9d050e8
to
4d1e087
Compare
There was a problem hiding this 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.
380e32c
to
51d342b
Compare
Makes sense to me! When the full release rolls around we can spruce up the test cases and get this merged :) |
Hi! React 19 launched 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! |
@jorgezreik yep, let's do those things :-) mark it as ready for review when you think it's so |
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.