fix: Add stricter check for "use server" exports #62821

shuding · 2024-03-04T15:57:11Z

As mentioned in the new-added error messages, and the linked resources:

Because the underlying network calls are always asynchronous, 'use server' can only be used on async functions.
https://react.dev/reference/react/use-server

It's a requirement that only async functions are allowed to be exported and annotated with 'use server'. Currently, we already have compiler check so this will already error:

'use server'

export function foo () {} // missing async

However, since exported values can be very dynamic the compiler can't catch all mistakes like that. We also have a runtime check for all exports in a 'use server' function, but it only covers typeof value === 'function'.

This PR adds a stricter check for "use server" annotated values to also make sure they're async functions (value.constructor.name === 'AsyncFunction').

That said, there are still cases like synchronously returning a promise to make a function "async", but it's still very different by definition. For example:

const f = async () => { throw 1; return 1 }
const g = () => { throw 1; return Promise.resolve(1) }

Where g() can be synchronously caught (try { g() } catch {}) but f() can't even if they have the same types. If we allow g to be a Server Action, this behavior is no longer always true but depending on where it's called (server or client).

Closes #62727.

ijjk · 2024-03-04T16:02:59Z

Tests Passed

ijjk · 2024-03-04T16:05:48Z

Stats from current PR

Default Build

General Overall increase ⚠️

	vercel/next.js canary	vercel/next.js shu/092d	Change
buildDuration	13.9s	13.8s	N/A
buildDurationCached	8.3s	6.8s	N/A
nodeModulesSize	197 MB	197 MB	⚠️ +3.9 kB
nextStartRea..uration (ms)	415ms	415ms	✓

Client Bundles (main, webpack)

	vercel/next.js canary	vercel/next.js shu/092d	Change
2453-HASH.js gzip	30.2 kB	30.2 kB	N/A
3304.HASH.js gzip	181 B	181 B	✓
3f784ff6-HASH.js gzip	53.7 kB	53.7 kB	N/A
8299-HASH.js gzip	5.04 kB	5.04 kB	N/A
framework-HASH.js gzip	45.2 kB	45.2 kB	✓
main-app-HASH.js gzip	242 B	242 B	✓
main-HASH.js gzip	32.2 kB	32.2 kB	N/A
webpack-HASH.js gzip	1.68 kB	1.68 kB	N/A
Overall change	45.6 kB	45.6 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	vercel/next.js shu/092d	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	vercel/next.js shu/092d	Change
_app-HASH.js gzip	196 B	197 B	N/A
_error-HASH.js gzip	184 B	184 B	✓
amp-HASH.js gzip	505 B	505 B	✓
css-HASH.js gzip	324 B	325 B	N/A
dynamic-HASH.js gzip	2.5 kB	2.5 kB	N/A
edge-ssr-HASH.js gzip	258 B	258 B	✓
head-HASH.js gzip	352 B	352 B	✓
hooks-HASH.js gzip	370 B	371 B	N/A
image-HASH.js gzip	4.2 kB	4.2 kB	✓
index-HASH.js gzip	259 B	259 B	✓
link-HASH.js gzip	2.67 kB	2.67 kB	N/A
routerDirect..HASH.js gzip	314 B	312 B	N/A
script-HASH.js gzip	386 B	386 B	✓
withRouter-HASH.js gzip	309 B	309 B	✓
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	6.56 kB	6.56 kB	✓

Client Build Manifests

	vercel/next.js canary	vercel/next.js shu/092d	Change
_buildManifest.js gzip	483 B	485 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	vercel/next.js shu/092d	Change
index.html gzip	529 B	528 B	N/A
link.html gzip	542 B	541 B	N/A
withRouter.html gzip	524 B	523 B	N/A
Overall change	0 B	0 B	✓

Edge SSR bundle Size

	vercel/next.js canary	vercel/next.js shu/092d	Change
edge-ssr.js gzip	95 kB	95 kB	N/A
page.js gzip	3.08 kB	3.08 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	vercel/next.js shu/092d	Change
middleware-b..fest.js gzip	624 B	626 B	N/A
middleware-r..fest.js gzip	151 B	151 B	✓
middleware.js gzip	25.5 kB	25.5 kB	N/A
edge-runtime..pack.js gzip	839 B	839 B	✓
Overall change	990 B	990 B	✓

Next Runtimes

	vercel/next.js canary	vercel/next.js shu/092d	Change
app-page-exp...dev.js gzip	171 kB	171 kB	✓
app-page-exp..prod.js gzip	96.7 kB	96.7 kB	✓
app-page-tur..prod.js gzip	98.5 kB	98.5 kB	✓
app-page-tur..prod.js gzip	92.9 kB	92.9 kB	✓
app-page.run...dev.js gzip	150 kB	150 kB	✓
app-page.run..prod.js gzip	91.4 kB	91.4 kB	✓
app-route-ex...dev.js gzip	21.3 kB	21.3 kB	✓
app-route-ex..prod.js gzip	15 kB	15 kB	✓
app-route-tu..prod.js gzip	15 kB	15 kB	✓
app-route-tu..prod.js gzip	14.8 kB	14.8 kB	✓
app-route.ru...dev.js gzip	20.9 kB	20.9 kB	✓
app-route.ru..prod.js gzip	14.7 kB	14.7 kB	✓
pages-api-tu..prod.js gzip	9.51 kB	9.51 kB	✓
pages-api.ru...dev.js gzip	9.79 kB	9.79 kB	✓
pages-api.ru..prod.js gzip	9.51 kB	9.51 kB	✓
pages-turbo...prod.js gzip	22.3 kB	22.3 kB	✓
pages.runtim...dev.js gzip	23 kB	23 kB	✓
pages.runtim..prod.js gzip	22.3 kB	22.3 kB	✓
server.runti..prod.js gzip	50.6 kB	50.6 kB	✓
Overall change	950 kB	950 kB	✓

build cache

	vercel/next.js canary	vercel/next.js shu/092d	Change
0.pack gzip	1.56 MB	1.56 MB	N/A
index.pack gzip	105 kB	105 kB	N/A
Overall change	0 B	0 B	✓

Diff details

Diff for middleware.js

Diff too large to display

Commit: 7f7943b

As mentioned in the new-added error messages, and the [linked resources](https://react.dev/reference/react/use-server#:~:text=Because%20the%20underlying%20network%20calls%20are%20always%20asynchronous%2C%20%27use%20server%27%20can%20only%20be%20used%20on%20async%20functions.): > Because the underlying network calls are always asynchronous, 'use server' can only be used on async functions. > https://react.dev/reference/react/use-server It's a requirement that only async functions are allowed to be exported and annotated with `'use server'`. Currently, we already have compiler check so this will already error: ```js 'use server' export function foo () {} // missing async ``` However, since exported values can be very dynamic the compiler can't catch all mistakes like that. We also have a runtime check for all exports in a `'use server'` function, but it only covers `typeof value === 'function'`. This PR adds a stricter check for "use server" annotated values to also make sure they're async functions (`value.constructor.name === 'AsyncFunction'`). That said, there are still cases like synchronously returning a promise to make a function "async", but it's still very different by definition. For example: ```js const f = async () => { throw 1; return 1 } const g = () => { throw 1; return Promise.resolve(1) } ``` Where `g()` can be synchronously caught (`try { g() } catch {}`) but `f()` can't even if they have the same types. If we allow `g` to be a Server Action, this behavior is no longer always true but depending on where it's called (server or client). Closes #62727.

sannajammeh · 2024-03-04T22:12:04Z

This appears to break cache function from React.
https://stackblitz.com/edit/stackblitz-starters-a4zmeu?file=app%2Fpage.tsx

"use server"
import { cache } from "react"
export const test = cache(async () => {
 return "test"
});

The following will now crash.

import { test } from "./actions"
const CachedComp = async () => {
  const val = await test()
  return <>{val}</>
}

export default function Page() {
  return <>
    <CachedComp />
    <CachedComp />
  </>
}

Everything works on latest canary 14.1.2-canary.5

danieltott · 2024-03-05T18:07:15Z

After updating to 14.1.2 we got surprised by an issue - we have an async server action function in a file marked with use server. That function was calling another function in the same file that was not async, but was exported (turns out for no reason, not using it anywhere else). I didn't get any warnings from nextjs on this, but it absolutely broke the original function since the other fn was now returning a promise instead of it's original value. Removing export from the function definition fixed that - but this was certainly unexpected.

The new fix seems to be automatically converting all exported functions in a use server file to async with no warning. Is this like...a bug? As in, should I log it somewhere? It certainly feels like a breaking change (since it did break our production build), but I'm not sure what to do with that information. If "do nothing" is the answer that's fine, I just wanted to bring it up.

mertafor · 2024-03-06T11:43:09Z

After updating to 14.1.2 we got surprised by an issue - we have an async server action function in a file marked with use server. That function was calling another function in the same file that was not async, but was exported (turns out for no reason, not using it anywhere else). I didn't get any warnings from nextjs on this, but it absolutely broke the original function since the other fn was now returning a promise instead of it's original value. Removing export from the function definition fixed that - but this was certainly unexpected.

The new fix seems to be automatically converting all exported functions in a use server file to async with no warning. Is this like...a bug? As in, should I log it somewhere? It certainly feels like a breaking change (since it did break our production build), but I'm not sure what to do with that information. If "do nothing" is the answer that's fine, I just wanted to bring it up.

Same here. I noticed the issue by an unexpected returned value, otherwise there was no errors or warnings. I was able to debug the issue by logging the result returned by server action and realize that it actually returns a promise even though the function is synchronous. Apparently all functions placed in a "use server" file are automatically turned into async functions, even though definitions are sync. In my opinion it was the missing piece for use server files and definitely a great improvement.

But we need an Eslint check/error for sync functions placed in use server files.

vercel/next.js#62821

chungweileong94 · 2024-03-07T12:50:06Z

packages/next/src/build/webpack/loaders/next-flight-loader/action-validate.ts

+    if (action.constructor.name !== 'AsyncFunction') {
+      const actionName: string = action.name || ''
+
+      // Note: if it's from library code with `async` being transpiled to returning a promise,


This bit is actually quite annoying for library authors tho, any plan where this will get improved in the future?

Agree. It also breaks the use of Zod for validating the input of Server Actions (this worked before):

"use server"; import { db } from "./db"; import { z } from "zod"; export const getUserName = z .function() .args(z.object({ userId: z.string() })) .implement(async ({ userId }) => { const user = await db.users.query({ where: { id: userId } }); return user.name; });

shuding · 2024-03-10T14:40:31Z

Hey everyone, thanks for all your feedback!

Tl;DR

"use server" requires the functions to be async. If they are not, they will be converted to async functions.

So, instead of silently converting your sync functions to be async (breaks type checking), here we're directly showing errors and ask you to make them async in the first place.

Details

it breaks React.cache

Please give #62860 (comment) a read - React.cache will not do anything when it's called inside a Server Action.

but it absolutely broke the original function since the other fn was now returning a promise instead of it's original value

As per this PR description, async functions and sync functions that return promises are fundamentally different things:

const f = async () => { throw 1; return 1 }
const g = () => { throw 1; return Promise.resolve(1) }

The new fix seems to be automatically converting all exported functions in a use server file to async with no warning. Is this like...a bug?

Again, as mentioned in the PR description, "Because the underlying network calls are always asynchronous, 'use server' can only be used on async functions." (https://react.dev/reference/react/use-server) that's a requirement. Previously that's not ensured which was a bug and this PR fixes it.

But we need an Eslint check/error for sync functions placed in use server files.

This would be great, but it's difficult to ensure the type statically. Also in TS async () => {} and () => Promise are considered the same type but they're actually different things.

Agree. It also breaks the use of Zod for validating the input of Server Actions (this worked before)

Agreed that this is annoying. I'll see if there are any other work arounds.

Addresses some feedback in #62821. This will re-allow implementations like: ```js 'use server' export const f = wrapper(async () => {}) ``` Where `wrapper` creates a sync function that returns a promise. Although it will still be silently converted to an async function under the hood. Closes NEXT-2790

add stricter check for 'use server' exports

6d1476f

shuding requested review from timneutkens, ijjk, huozhi, ztanner, feedthejim, a team and wyattjoh as code owners March 4, 2024 15:57

shuding requested review from manovotny and StephDietz and removed request for a team March 4, 2024 15:57

ijjk added area: documentation created-by: Next.js team PRs by the Next.js team. type: next labels Mar 4, 2024

shuding mentioned this pull request Mar 4, 2024

14.1.1 version introducing breaking change for cookies() #62727

Closed

huozhi approved these changes Mar 4, 2024

View reviewed changes

ztanner approved these changes Mar 4, 2024

View reviewed changes

Merge branch 'canary' into shu/092d

7f7943b

ijjk approved these changes Mar 4, 2024

View reviewed changes

shuding merged commit 7b2b982 into canary Mar 4, 2024
67 checks passed

shuding deleted the shu/092d branch March 4, 2024 17:50

yannxaver mentioned this pull request Mar 5, 2024

Docs: How to use react cache with server actions if "use server" files only allow async exports? #62860

Closed

chungweileong94 added a commit to chungweileong94/server-act that referenced this pull request Mar 7, 2024

test if action is AsyncFunction

94d1e3a

vercel/next.js#62821

chungweileong94 reviewed Mar 7, 2024

View reviewed changes

shuding mentioned this pull request Mar 10, 2024

A "use server" file can only export async functions' error when exporting non-async/cache functions #62926

Closed

1 task

shuding mentioned this pull request Mar 12, 2024

fix: Loose Server Actions runtime check #63200

Merged

github-actions bot added the locked label Mar 25, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add stricter check for "use server" exports #62821

fix: Add stricter check for "use server" exports #62821

shuding commented Mar 4, 2024 •

edited

Loading

ijjk commented Mar 4, 2024 •

edited

Loading

ijjk commented Mar 4, 2024 •

edited

Loading

sannajammeh commented Mar 4, 2024 •

edited

Loading

danieltott commented Mar 5, 2024

mertafor commented Mar 6, 2024 •

edited

Loading

chungweileong94 Mar 7, 2024

sam3d Mar 9, 2024

shuding commented Mar 10, 2024 •

edited

Loading

fix: Add stricter check for "use server" exports #62821

fix: Add stricter check for "use server" exports #62821

Conversation

shuding commented Mar 4, 2024 • edited Loading

ijjk commented Mar 4, 2024 • edited Loading

Tests Passed

ijjk commented Mar 4, 2024 • edited Loading

Stats from current PR

sannajammeh commented Mar 4, 2024 • edited Loading

danieltott commented Mar 5, 2024

mertafor commented Mar 6, 2024 • edited Loading

chungweileong94 Mar 7, 2024

Choose a reason for hiding this comment

sam3d Mar 9, 2024

Choose a reason for hiding this comment

shuding commented Mar 10, 2024 • edited Loading

Tl;DR

Details

shuding commented Mar 4, 2024 •

edited

Loading

ijjk commented Mar 4, 2024 •

edited

Loading

ijjk commented Mar 4, 2024 •

edited

Loading

sannajammeh commented Mar 4, 2024 •

edited

Loading

mertafor commented Mar 6, 2024 •

edited

Loading

shuding commented Mar 10, 2024 •

edited

Loading