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(cookies): Apply split cookie session middleware per route #2880

Conversation

gdarchen
Copy link
Contributor

@gdarchen gdarchen commented Jun 6, 2024

Description

When using split cookies to have different cookie name for the shop-api and the admin-api, we used to apply the cookieSession() middleware globally.

However, Nest Global Middlewares can't be applied to specific routes.
By doing Global Middlewares, the last applied app.use() middleware was overriding the above-mentioned ones.

Instead, we have to use Functional Middlewares for the specific paths we like.

As a consequence, for the case when the user wants to split the cookies, we do not do it in the bootstrap() function any longer but instead we do it in the AppModule.configure() method, where other middlewares route-specific are already applied.

Breaking changes

There is no breaking change as we just fix a behaviour that was not working.

Screenshots

N/A

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains a single feature
  • I have checked my own PR

👍 Most of the time:

  • I have added or updated test cases
  • I have updated the README if needed

Copy link

netlify bot commented Jun 6, 2024

Deploy Preview for effervescent-donut-4977b2 canceled.

Name Link
🔨 Latest commit 54a753b
🔍 Latest deploy log https://app.netlify.com/sites/effervescent-donut-4977b2/deploys/6661757a8a18bc000870ed6d

@gdarchen gdarchen marked this pull request as ready for review June 6, 2024 08:59
@michaelbromley michaelbromley merged commit 37de9f4 into vendure-ecommerce:master Jun 10, 2024
15 of 16 checks passed
@michaelbromley
Copy link
Member

Thanks for the fix 👍

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.

2 participants