vercel / next.js

The React Framework
https://nextjs.org
MIT License
123.39k stars 26.32k forks source link

When a Server Action that calls `redirect` is composed with an Action on the Client `undefined` is returned to the Client. #66704

Open schimi-dev opened 1 month ago

schimi-dev commented 1 month ago

Link to the code that reproduces this issue

https://github.com/schimi-dev/next-redirect-bug-reproduction

To Reproduce

  1. Start the application in development mode and navigate to http://localhost:3000
  2. Click on "Edit Profile"
  3. Click on "Save"
  4. You can see in the browser console, that undefined was returned by the Server Action. Typescript does not know this. This is caused by the redirect function used inside the Server Action.

Current vs. Expected behavior

Current: When redirect is called by the Server Action undefined is returned to the client and the code after the await statement on the client is executed.

Expected: The code after the await statement on the client is not exected, since redirect has a never return type.

Provide environment information

Operating System:
  Platform: win32
  Arch: x64
  Version: Windows 11 Pro
  Available memory (MB): 16113
  Available CPU cores: 8
Binaries:
  Node: 20.11.1
  npm: N/A
  Yarn: N/A
  pnpm: N/A
Relevant Packages:
  next: 15.0.0-rc.0 // Latest available version is detected (15.0.0-rc.0).
  eslint-config-next: 15.0.0-rc.0
  react: 19.0.0-rc-f994737d14-20240522
  react-dom: 19.0.0-rc-f994737d14-20240522
  typescript: 5.4.5
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

Navigation

Which stage(s) are affected? (Select all that apply)

next dev (local), next start (local), Vercel (Deployed), Other (Deployed)

Additional context

https://react.dev/reference/rsc/server-actions#composing-server-actions-with-actions

This problem is especially tricky and can lead to subtle bugs with conditional redirects, such as handling a session timeout in the data access layer by redirecting the user to the login page.

Shahindavoodicom commented 1 month ago

The code currently lacks type safety and purity. Each function should adhere to the single responsibility principle, performing only one task. Specifically, the updateUser function should solely handle updating a user. The authentication logic that redirects the user should be separated from the updateUser function. Additionally, you need to clearly define the return types of all functions.

schimi-dev commented 1 month ago

@Shahindavoodicom Nope, basically the whole React Conf was about composability of the new primitives. The React Docs highlight an important concept of composing Server Actions with Actions on the Client. https://react.dev/reference/rsc/server-actions#composing-server-actions-with-actions

Moreover, Next.js recommends handling aspects such as session verification and authorization checks close to the actual call to a data source by building a Data Access Layer (DAL) where aspects such as session verification are done: https://nextjs.org/blog/security-nextjs-server-components-actions https://nextjs.org/docs/app/building-your-application/authentication https://www.youtube.com/watch?v=N_sUsq_y10U

Therefore, composing these primitives needs to work. Moreover, not explicitely using types in Typescript, at places where they can be derived via inference does not at all mean that the code is not type safe: https://www.typescriptlang.org/docs/handbook/2/functions.html#inference

So, if the redirect function statically states to return a never type but at runtime there is a scenario where code after that function is executed with an undefined return value this is problematic. It should not be the job of the person using that function to provide an explicit return type in order to achieve type safety. This should work via inference.

beckerei commented 1 month ago

So, if the redirect function statically states to return a never type but at runtime there is a scenario where code after that function is executed with an undefined return value this is problematic. It should not be the job of the person using that function to provide an explicit return type in order to achieve type safety. This should work via inference.

There is more to it, you can't event type your server action correctly if you are not aware of that behaviour. I usually use result objects indicating ok/failure + additional data, but a simplyfied version just returns a boolean. E.g.

'use server';

import { getUser } from 'somewhere'

export async function action(id: number): boolean {
    const user = await getUser(id)

    if(!user) redirect('/login')

    if(user.hasPermission()) {
        doSomething()
        return true
    }

    return false
}

I can now call it and work on the result on the client:

const success = await action(44)

if(!success) { 
    // show permission error to user
 } else {
    // show success to the user
}

type signature says this is fine. But instead result is undefined and the code is actually executing the else path, while none of the paths should actually be executed.

If I know of the behaviour I have to adapt my type to return boolean or undefined and explicitly handle the undefined behaviour.

Shahindavoodicom commented 1 month ago

@Shahindavoodicom Nope, basically the whole React Conf was about composability of the new primitives. The React Docs highlight an important concept of composing Server Actions with Actions on the Client. https://react.dev/reference/rsc/server-actions#composing-server-actions-with-actions

Moreover, Next.js recommends handling aspects such as session verification and authorization checks close to the actual call to a data source by building a Data Access Layer (DAL) where aspects such as session verification are done: https://nextjs.org/blog/security-nextjs-server-components-actions https://nextjs.org/docs/app/building-your-application/authentication https://www.youtube.com/watch?v=N_sUsq_y10U

Therefore, composing these primitives needs to work. Moreover, not explicitely using types in Typescript, at places where they can be derived via inference does not at all mean that the code is not type safe: https://www.typescriptlang.org/docs/handbook/2/functions.html#inference

So, if the redirect function statically states to return a never type but at runtime there is a scenario where code after that function is executed with an undefined return value this is problematic. It should not be the job of the person using that function to provide an explicit return type in order to achieve type safety. This should work via inference.

Yes, you are right. I tried running your project and defining the types, but it still returns undefined. This is strange because redirect returns a never type and, as you mentioned, it should not execute any statements after your await code.