QuiiBz / next-international

Type-safe internationalization (i18n) for Next.js
https://next-international.vercel.app
MIT License
1.26k stars 59 forks source link

Cookies are not being included into the response header when "redirect" is performed (Server Action) #212

Closed ofranquesa closed 10 months ago

ofranquesa commented 11 months ago

Describe the bug I'm using the brand new "Server Actions" feature from 13.5. My server action just checks a dumb password, sets a cookie login=true and redirect the browser. However, the redirect ('/') doesn't include the "login" cookie. I've reproduced this issue with a brand new next-app project, introducing first the server action and working as expected and then adding the next-international functionality, resulting on the described problem/bug.

To Reproduce Steps to reproduce the behavior:

  1. Clone the public repo with the described scenario ready (next-international + Server Action) test repo
  2. Go to "/" -> you will be redirected to /de/password
  3. Introduce a correct password

Expected behavior

  1. Cookie "login" should be set in user's browser
  2. User should be redirected to "/"

Current behavior Post request response is not including the SET-COOKIE header for the cookie "login"

About (please complete the following information): Operating System: Platform: darwin Arch: arm64 Version: Darwin Kernel Version 21.4.0: Mon Feb 21 20:35:58 PST 2022; root:xnu-8020.101.4~2/RELEASE_ARM64_T6000 Binaries: Node: 16.16.0 npm: 8.11.0 Yarn: N/A pnpm: 7.9.0 Relevant Packages: next: 13.5.4 eslint-config-next: 13.5.4 react: 18.2.0 react-dom: 18.2.0 typescript: 5.2.2 Next.js Config: output: N/A

Additional context Add any other context about the problem here.

QuiiBz commented 11 months ago

I haven't used Server Actions a lot - does it break because of next-international's middleware? AKA does removing the middleware make it work?

I'll take a look at the repo tomorrow.

ofranquesa commented 11 months ago

Thanks for your quick reply!

No, I've tested it on my site with middleware and removing all next-international instances/code and the cookie was included into the response header. I've prepared the repo so it could be easy for you to find out... Let me know if I can help you out somehow.

By the end, and I still don't understand why, but I had to replace redirect('/') to redirect('http://anydomain.com'). By doing this, then everything worked as expected (less redirects and cookie INCLUDED in all subsequent internal redirects)

QuiiBz commented 11 months ago

I had to replace redirect('/') to redirect('http://anydomain.com')

This feels like a bug Next.js, doesn't it? next-international only adds cookies, so the middleware shouldn't have anything to do with this issue.

ofranquesa commented 11 months ago

Well, yes and no, because actually when I use ('/') with no next-international, browser is redirected correctly. Also TBH the Next.js docs on some of the new features are really poor , precisely with the redirect() function, there is no clue how internally works, etc...

QuiiBz commented 11 months ago

I tried to find an issue about this on the Next.js repo but couldn't find anything. This will probably require a bit more knowledge of the internals of Next.js, but I'll do my best to understand what's going on.

Just to confirm, using redirect('http://anydomain.com') works as expected?

ofranquesa commented 11 months ago

Thank you. I don't know if it's coming from Next or International... I've gave you access to the private repo where I am doing my site (branch "build_site"). If you replace the redirect instruction when the password is correct to '/' and observer all the requests on the Chrome console, it's a bit strange what it happens. (5 calls, two of them "HEAD"). Then replacing the '/' by an absolute path like "http://mydomainc.com" everything works fine.

Otherwise, if I remove the next-international and use redirect('/') , then also works as expected. I hope it helps.

Pavelas commented 11 months ago

I'm experiencing the same issue. It seems that there's a problem with the middleware.

If you will comment out the following line:

function addLocaleToResponse(response: NextResponse, locale: string) {
  response.headers.set(LOCALE_HEADER, locale)
  //response.cookies.set(LOCALE_COOKIE, locale)
  return response
}

It will fix the issue, but in this case it breaks translations.

QuiiBz commented 11 months ago

@Pavelas could you share the content of your Server Action?

I'm really suspecting this to be a bug with Next.js, as next-international's middleware only read and set cookies.

Pavelas commented 11 months ago

Yeah sure, so basically what I am trying to do with my Server Action is just to set up auth tokens.

export async function loginUser(_: LoginUserState, formData: FormData) {
  // ... some code here

  const { accessToken, refreshToken } = response.data

  setAuthCookies(accessToken, refreshToken)
  redirect(ROUTES.PROTECTED)
}

And then I have utility function setAuthCookies where I am using next/headers, code was simplified for better readability:

import { cookies } from 'next/headers'

export function setAuthCookies(accessToken: string, refreshToken: string) {
  cookies().set('access_token', accessToken)
  cookies().set('refresh_token', refreshToken)
}

My temporary solution was to duplicate your middleware and update locale cookie only when it's needed. I've found that some other libraries are using this approach.

Yovach commented 11 months ago

Hi, I don't know if it's related but these lines seems be the source of this issue : https://github.com/vercel/next.js/blob/canary/packages/next/src/server/app-render/action-handler.ts#L546-L554

because they aren't set for "fetch functions" (server actions)

QuiiBz commented 10 months ago

My temporary solution was to duplicate your middleware and update locale cookie only when it's needed

@Pavelas could you share the code that you updated in the middleware? That would help fix this issue.

Yovach commented 10 months ago

Hi, On the repo shared by ofranquesa, I set the experimental.logging.level to "verbose" like this :

/** @type {import('next').NextConfig} */
const nextConfig = {
    reactStrictMode: true,
    experimental: {
        serverActions: true,
        logging: {
            level: "verbose"
        }
    }
}

module.exports = nextConfig

And on POST on the password action, I have these logs:

 POST /de/password 200 in 2559ms
 │ HEAD http://localhost:3000/ 200 in 960ms (cache: SKIP)
 │  │  Cache missed reason: (auto no cache)
 │  │ GET http://localhost:3000/ 200 in 37ms (cache: SKIP)
 │  │  │  Cache missed reason: (auto no cache)

"Cache missed" is set at the following line : https://github.com/vercel/next.js/blob/canary/packages/next/src/server/next-server.ts#L1123

and the "auto no cache" is set here : https://github.com/vercel/next.js/blob/canary/packages/next/src/server/lib/patch-fetch.ts#L343

So it's maybe related to Next.js

ofranquesa commented 10 months ago

Thanks @Yovach and @QuiiBz for looking into this.

Shall we then report to Next.js at this point?

Yovach commented 10 months ago

Thanks @Yovach and @QuiiBz for looking into this.

Shall we then report to Next.js at this point?

I think I've found a fix but I need to be sure that it doesn't break anything

QuiiBz commented 10 months ago

Thanks to @Yovach, we merged #271 and I released next-international@1.2.0-rc.0 to make sure it works as expected and doesn't break anything. Please try this version and let me know if the issue is resolved - so we can release the fix officially!

Yovach commented 10 months ago

Thanks to @Yovach, we merged #271 and I released next-international@1.2.0-rc.0 to make sure it works as expected and doesn't break anything. Please try this version and let me know if the issue is resolved - so we can release the fix officially!

In my case, next-international@1.2.0-rc.0 solved the issue and didn't break anything (apparently).

QuiiBz commented 10 months ago

@ofranquesa could you confirm that next-international@1.2.0-rc.0 also fixed the issue on your side, so we can release the fix officially?

ofranquesa commented 10 months ago

sorry about the late reply. I'll try it out this evening and let you know. But if the fix works on my example repo, then it should be fixed! Thanks!

QuiiBz commented 10 months ago

I’ve published the stable release, thanks everyone!

https://github.com/QuiiBz/next-international/releases/tag/1.1.4

ofranquesa commented 10 months ago

I had now finally the time to test the update on my app... now I am getting on the console:

[next-international] The locale 'undefined' is not supported. Defined locales are: [de, en].

coming from create-i18n-provider-client.tsx?

I didn't change anything from my code and on my middleware stays:

const I18nMiddleware = createI18nMiddleware({ locales: ['en', 'de'], defaultLocale: 'de' })

Yovach commented 10 months ago

I had now finally the time to test the update on my app... now I am getting on the console:

[next-international] The locale 'undefined' is not supported. Defined locales are: [de, en].

coming from create-i18n-provider-client.tsx?

I didn't change anything from my code and on my middleware stays:

const I18nMiddleware = createI18nMiddleware({ locales: ['en', 'de'], defaultLocale: 'de' })

I don't have this issue, can you provide the code with createI18nServer and createI18nClient ?

ofranquesa commented 10 months ago

import { createI18nClient } from 'next-international/client' export const { useI18n, useScopedI18n, I18nProviderClient, useChangeLocale, useCurrentLocale, } = createI18nClient({ de: () => import('./de'), en: () => import('./en') })

import { createI18nServer } from 'next-international/server' export const { getI18n, getScopedI18n, getStaticParams, getCurrentLocale } = createI18nServer({ de: () => import('./de'), en: () => import('./en') })

Yovach commented 10 months ago

const I18nMiddleware = createI18nMiddleware({ locales: ['en', 'de'], defaultLocale: 'de' })

Can you try with the following option (on createI18nClient and createI18nServer) :

{
  fallbackLocale: en,
},

When en is

import en from "./en";

EDIT: Have you tried this ? https://next-international.vercel.app/docs/app-setup#move-your-existing-files

QuiiBz commented 10 months ago

The locale shouldn't be undefined, the middleware is likely mis-configured. Happy to dig more into it if you could update your reproduction!

ofranquesa commented 10 months ago

@Yovach I've tried both and didn't work it.

@QuiiBz the bug repo is also returning this error, but I think it has something to do with my machine. I've updated yesterday Node to v18.18.2 and since then I am getting error here and there (also react wise). Probably has nothing to do with next-international.