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

Show a proper error if a server function's bound args cannot be serialized #73471

Merged
merged 13 commits into from
Dec 3, 2024
Merged
12 changes: 10 additions & 2 deletions crates/next-custom-transforms/src/transforms/server_actions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2578,8 +2578,16 @@ fn collect_idents_in_pat(pat: &Pat, idents: &mut Vec<Ident>) {
}

fn collect_decl_idents_in_stmt(stmt: &Stmt, idents: &mut Vec<Ident>) {
if let Stmt::Decl(Decl::Var(var)) = &stmt {
collect_idents_in_var_decls(&var.decls, idents);
if let Stmt::Decl(decl) = stmt {
match decl {
Decl::Var(var) => {
collect_idents_in_var_decls(&var.decls, idents);
}
Decl::Fn(fn_decl) => {
idents.push(fn_decl.ident.clone());
}
_ => {}
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
function createCachedFn(start) {
function fn() {
return start + Math.random()
}

return async () => {
'use cache'
return fn()
}
}

function createServerAction(start) {
function fn() {
return start + Math.random()
}

return async () => {
'use server'
console.log(fn())
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/* __next_internal_action_entry_do_not_use__ {"401c36b06e398c97abe5d5d7ae8c672bfddf4e1b91":"$$RSC_SERVER_ACTION_2","c03128060c414d59f8552e4788b846c0d2b7f74743":"$$RSC_SERVER_CACHE_0"} */ import { registerServerReference } from "private-next-rsc-server-reference";
import { encryptActionBoundArgs, decryptActionBoundArgs } from "private-next-rsc-action-encryption";
import { cache as $$cache__ } from "private-next-rsc-cache-wrapper";
export var $$RSC_SERVER_CACHE_0 = $$cache__("default", "c03128060c414d59f8552e4788b846c0d2b7f74743", 1, async function([$$ACTION_ARG_0]) {
return $$ACTION_ARG_0();
});
function createCachedFn(start) {
function fn() {
return start + Math.random();
}
return $$RSC_SERVER_REF_1.bind(null, encryptActionBoundArgs("c03128060c414d59f8552e4788b846c0d2b7f74743", [
fn
]));
}
var $$RSC_SERVER_REF_1 = /*#__TURBOPACK_DISABLE_EXPORT_MERGING__*/ registerServerReference($$RSC_SERVER_CACHE_0, "c03128060c414d59f8552e4788b846c0d2b7f74743", null);
export const /*#__TURBOPACK_DISABLE_EXPORT_MERGING__*/ $$RSC_SERVER_ACTION_2 = async function($$ACTION_CLOSURE_BOUND) {
var [$$ACTION_ARG_0] = await decryptActionBoundArgs("401c36b06e398c97abe5d5d7ae8c672bfddf4e1b91", $$ACTION_CLOSURE_BOUND);
console.log($$ACTION_ARG_0());
};
function createServerAction(start) {
function fn() {
return start + Math.random();
}
return registerServerReference($$RSC_SERVER_ACTION_2, "401c36b06e398c97abe5d5d7ae8c672bfddf4e1b91", null).bind(null, encryptActionBoundArgs("401c36b06e398c97abe5d5d7ae8c672bfddf4e1b91", [
fn
]));
}
34 changes: 33 additions & 1 deletion packages/next/src/server/app-render/encryption.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,43 @@ async function encodeActionBoundArg(actionId: string, arg: string) {
export async function encryptActionBoundArgs(actionId: string, args: any[]) {
const { clientModules } = getClientReferenceManifestForRsc()

// An error stack that's created here looks like this:
// Error:
// at encryptActionBoundArg
// at <actual userland call site>
const stack = new Error().stack!.split('\n').slice(2).join('\n')

let error: Error | undefined

// Using Flight to serialize the args into a string.
const serialized = await streamToString(
renderToReadableStream(args, clientModules)
renderToReadableStream(args, clientModules, {
onError(err) {
// We're only reporting one error at a time, starting with the first.
if (error) {
return
}

// Use the original error message...
error = err instanceof Error ? err : new Error(String(err))
// ...and attach the previously created stack, because err.stack is a
// useless Flight Server call stack.
error.stack = stack
},
})
)

if (error) {
if (process.env.NODE_ENV === 'development') {
// Logging the error is needed for server functions that are passed to the
// client where the decryption is not done during rendering. Console
// replaying allows us to still show the error dev overlay in this case.
console.error(error)
}

throw error
}

// Encrypt the serialized string with the action id as the salt.
// Add a prefix to later ensure that the payload is correctly decrypted, similar
// to a checksum.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
'use client'

import { useActionState } from 'react'

export function Client({ getValue }) {
const [result, formAction] = useActionState(getValue, 0)

return (
<form action={formAction}>
<p>{result}</p>
<button>Submit</button>
</form>
)
}
16 changes: 16 additions & 0 deletions test/e2e/app-dir/use-cache-close-over-function/app/client/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { Client } from './client'

function createCachedFn(start: number) {
function fn() {
return start
}

return async () => {
'use cache'
return Math.random() + fn()
}
}

export default async function Page() {
return <Client getValue={createCachedFn(42)} />
}
11 changes: 11 additions & 0 deletions test/e2e/app-dir/use-cache-close-over-function/app/layout.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { Suspense } from 'react'

export default function Root({ children }: { children: React.ReactNode }) {
return (
<html>
<body>
<Suspense>{children}</Suspense>
</body>
</html>
)
}
14 changes: 14 additions & 0 deletions test/e2e/app-dir/use-cache-close-over-function/app/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import Link from 'next/link'

export default function Page() {
return (
<>
<p>
<Link href="/client">Client</Link>
</p>
<p>
<Link href="/server">Server</Link>
</p>
</>
)
}
16 changes: 16 additions & 0 deletions test/e2e/app-dir/use-cache-close-over-function/app/server/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
function createCachedFn(start: number) {
function fn() {
return start
}

return async () => {
'use cache'
return Math.random() + fn()
}
}

const getCachedValue = createCachedFn(42)

export default async function Page() {
return <p>{getCachedValue()}</p>
}
11 changes: 11 additions & 0 deletions test/e2e/app-dir/use-cache-close-over-function/next.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/**
* @type {import('next').NextConfig}
*/
const nextConfig = {
experimental: {
dynamicIO: true,
serverSourceMaps: true,
},
}

module.exports = nextConfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
import { nextTestSetup } from 'e2e-utils'
import {
assertHasRedbox,
getRedboxDescription,
getRedboxSource,
openRedbox,
} from 'next-test-utils'

describe('use-cache-close-over-function', () => {
const { next, isNextDev, skipped } = nextTestSetup({
files: __dirname,
skipDeployment: true,
skipStart: process.env.NEXT_TEST_MODE !== 'dev',
})

if (skipped) {
return
}

if (isNextDev) {
it('should show an error toast for client-side usage', async () => {
const browser = await next.browser('/client')

await openRedbox(browser)

const errorDescription = await getRedboxDescription(browser)
const errorSource = await getRedboxSource(browser)

expect(errorDescription).toMatchInlineSnapshot(`
"[ Prerender ] Error: Functions cannot be passed directly to Client Components unless you explicitly expose it by marking it with "use server". Or maybe you meant to call this function rather than return it.
Copy link
Member

@lubieowoce lubieowoce Dec 3, 2024

Choose a reason for hiding this comment

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

nit: wouldn't a "use cache" function would also avoid this? i guess this message is coming from react which doesn't know about that directive, but maybe we need to postprocess the message to rephrase it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this message from React is a bit outdated. Not only regarding the directive, but also the "passed to Client Components" part. For "use cache" that's a server=>cache environment transition, and not the previously assumed server=>client transition.

[function fn]
^^^^^^^^^^^"
`)

expect(errorSource).toMatchInlineSnapshot(`
"app/client/page.tsx (8:3) @ createCachedFn

6 | }
7 |
> 8 | return async () => {
| ^
9 | 'use cache'
10 | return Math.random() + fn()
11 | }"
`)
})

it('should show the error overlay for server-side usage', async () => {
const browser = await next.browser('/server')

await assertHasRedbox(browser)

const errorDescription = await getRedboxDescription(browser)
const errorSource = await getRedboxSource(browser)

expect(errorDescription).toMatchInlineSnapshot(`
"[ Prerender ] Error: Functions cannot be passed directly to Client Components unless you explicitly expose it by marking it with "use server". Or maybe you meant to call this function rather than return it.
[function fn]
^^^^^^^^^^^"
`)

expect(errorSource).toMatchInlineSnapshot(`
"app/server/page.tsx (6:3) @ createCachedFn

4 | }
5 |
> 6 | return async () => {
| ^
7 | 'use cache'
8 | return Math.random() + fn()
9 | }"
`)
})
} else {
it('should fail the build with an error', async () => {
const { cliOutput } = await next.build()

expect(cliOutput).toInclude(`
Error: Functions cannot be passed directly to Client Components unless you explicitly expose it by marking it with "use server". Or maybe you meant to call this function rather than return it.
[function]
^^^^^^^^`)

expect(cliOutput).toMatch(
/Error occurred prerendering page "\/(client|server)"/
)
})
}
})
Loading