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): Correctly apply global and per-route middlewares #2911

Conversation

gdarchen
Copy link
Contributor

Description

In a previous PR, I tried to fix an issue causing some per-route middlewares being broken when using the split cookies for the Admin and Shop APIs.

But since it was released in the 2.2.6, the cookies split is no longer working at all.

After investigating, it turned out we were still badly applying the per-route middleware:

  • We did not set a global middleware in the bootstrap.ts file
  • We were overriding the per-route middlewares in the app.modile.ts file because of the configuration on the / route

In this PR, I made sure to:

  1. Apply a global cookies middleware, either using the right cookie name:
    • If the cookies have to be split, use the shop cookie name
    • Else, use the shared cookie name if provided
    • If not provided, default to session
  2. Only apply specific cookieSession middlewares on the /shop-api and /admin-api routes

Breaking changes

No breaking change.

Screenshots

vendure.mp4

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

vercel bot commented Jun 19, 2024

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

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Jun 19, 2024 3:34pm

@gdarchen
Copy link
Contributor Author

Note

@michaelbromley
I made a video to prove I could make it work properly on the dev-server.
But that would be awesome if we could deploy like a canary version from a pull request (based on a label?) to test this version on our actual projects before merging it.
Do you think it's something doable?

@michaelbromley
Copy link
Member

Hi @gdarchen thanks for the quick turn around on this. I just published @vendure/core@2.2.7-canary.0 under the canary tag. Please let me know whether it works as intended and then we can publish the patch 👍

@gdarchen
Copy link
Contributor Author

Hi @michaelbromley 👋
I just tested a deployment on our staging environment using the canary version you released and it seems to work as expected!

@michaelbromley michaelbromley merged commit 2d2e518 into vendure-ecommerce:master Jun 21, 2024
13 checks passed
@choim4389
Copy link

When will it be distributed to a stable version? This issue is an important feature of our product.

@michaelbromley
Copy link
Member

Likely this week.

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.

3 participants