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: delay write cookie headers to first write #5986

Closed
wants to merge 16 commits into from
Closed

Conversation

PatrickJS
Copy link
Member

Overview

fixes #5951

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests / types / typos

Description

set headers once but allow for dev to add cookie values

Copy link

netlify bot commented Mar 9, 2024

Deploy Preview for qwik-insights ready!

Name Link
🔨 Latest commit ef2c3b2
🔍 Latest deploy log https://app.netlify.com/sites/qwik-insights/deploys/660a4e10b172b5000846e4d8
😎 Deploy Preview https://deploy-preview-5986--qwik-insights.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

cloudflare-pages bot commented Mar 9, 2024

Deploying qwik-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: ef2c3b2
Status: ✅  Deploy successful!
Preview URL: https://0218750d.qwik-docs.pages.dev
Branch Preview URL: https://fix-5951.qwik-docs.pages.dev

View logs

packages/qwik-city/middleware/node/http.ts Outdated Show resolved Hide resolved
@PatrickJS
Copy link
Member Author

@wmertens we need to delay writing to headers until we first "write" to the client. that way any app code can run and add cookie headers before we stream.

Copy link
Member

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

This looks good, but it doesn't apply this to all integrations right?

And what about other headers, don't they have the same problem? Or is the problem specifically that you can only send one cookie header?

@PatrickJS
Copy link
Member Author

yes this applies to all headers. the e2e tests hang with this fix but when I run it locally it works correctly in preview-only. so there is something going on. @wmertens is there a better way to hook into this?

@wmertens
Copy link
Member

I don't think there's a better way because each adapter had their own way of sending cookies.

I think the e2e problem may be when there's no write and the response just closes without sending the cookies? Odd but maybe.

In any case you should handle once still being true on close.

@wmertens
Copy link
Member

@PatrickJS notice that the e2e tests fail on this

Copy link
Member

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

Looks good!

@wmertens
Copy link
Member

wmertens commented Apr 1, 2024

...but sadly the e2e fails. Try removing the once=false in close? Not sure that's needed or even desired

@PatrickJS
Copy link
Member Author

@wmertens I'm thinking e2e is waiting for headers which is why it fails during e2e but not when running locally. maybe there's another way

@PatrickJS PatrickJS marked this pull request as draft May 1, 2024 00:32
@PatrickJS PatrickJS self-assigned this May 13, 2024
@PatrickJS
Copy link
Member Author

I think it's better having devs use plugin@<name>.ts and middleware for this

@PatrickJS PatrickJS closed this May 15, 2024
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.

[🐞] Cookie not set when using $server
3 participants