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

fix(next-international): preserve cookies in the middleware #271

Merged
merged 9 commits into from
Oct 28, 2023

Conversation

Yovach
Copy link
Contributor

@Yovach Yovach commented Oct 26, 2023

Fixes #212

The response from NextResponse.next and NextResponse.redirect doesn't contains any cookies except the Next-Locale set by the middleware.

At first, I wanted to clone the headers but this added a lot of elements that were not included in the basic Server Actions

EDIT: I've just noticed that the "Set-Cookie" header was defined for each request, which isn't great.

@vercel
Copy link

vercel bot commented Oct 26, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
next-international ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 27, 2023 8:28pm

@Yovach Yovach changed the title Clone cookies from NextRequest into NextResponse [DO NOT MERGE] Clone cookies from NextRequest into NextResponse Oct 26, 2023
@Yovach Yovach changed the title [DO NOT MERGE] Clone cookies from NextRequest into NextResponse Clone cookies from NextRequest into NextResponse Oct 26, 2023
@Yovach
Copy link
Contributor Author

Yovach commented Oct 26, 2023

I checked and it shouldn't be a problem.

We can use the init parameter of NextResponse.redirect and NextResponse.rewrite but it'll pass all headers

Copy link
Owner

@QuiiBz QuiiBz left a comment

Choose a reason for hiding this comment

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

Thanks! This seems like acceptable changes, I'll try the changes in an app before merging. Also thanks for the added documentation!

I only have one small suggestion:

packages/next-international/src/app/middleware/index.ts Outdated Show resolved Hide resolved
@Yovach
Copy link
Contributor Author

Yovach commented Oct 27, 2023

The fact that Set-Cookie is set on each request is probably an issue because it'll break sites that are using Cookie parameters.

I just noticed that this also removes things like "HttpOnly" and "Secure" so the PR doesn't seems to be ready yet :/

@Yovach
Copy link
Contributor Author

Yovach commented Oct 27, 2023

With my latest commit, the problem seems to have been solved as the Next-Locale cookie is only applied if necessary. I've tried this when we change the locale by url (http://localhost:3000/en => http://localhost:3000/fr) and it doesn't clear my current cookies.

Copy link
Owner

@QuiiBz QuiiBz left a comment

Choose a reason for hiding this comment

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

I just tried and everything seems to be working as expected, thanks!

I'll merge and make a pre-release before landing this change officially, to make sure we don't break anything.

@QuiiBz QuiiBz changed the title Clone cookies from NextRequest into NextResponse fix(next-international): preserve cookies in the middleware Oct 28, 2023
@QuiiBz QuiiBz merged commit ad9c30d into QuiiBz:main Oct 28, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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