qwik icon indicating copy to clipboard operation
qwik copied to clipboard

fix: delay write cookie headers to first write

Open PatrickJS opened this issue 1 year ago • 5 comments

Overview

fixes https://github.com/BuilderIO/qwik/issues/5951

What is it?

  • [ ] Feature / enhancement
  • [x] Bug
  • [ ] Docs / tests / types / typos

Description

set headers once but allow for dev to add cookie values

PatrickJS avatar Mar 09 '24 22:03 PatrickJS

Deploy Preview for qwik-insights ready!

Name Link
Latest commit ef2c3b24096d0a83ea7973fae84fc25ff3d3f361
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.

netlify[bot] avatar Mar 09 '24 22:03 netlify[bot]

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

@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.

PatrickJS avatar Mar 10 '24 03:03 PatrickJS

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?

PatrickJS avatar Mar 10 '24 18:03 PatrickJS

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 avatar Mar 10 '24 18:03 wmertens

@PatrickJS notice that the e2e tests fail on this

wmertens avatar Mar 30 '24 09:03 wmertens

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

wmertens avatar Apr 01 '24 14:04 wmertens

@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 avatar Apr 01 '24 18:04 PatrickJS

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

PatrickJS avatar May 15 '24 02:05 PatrickJS