fix: support setting cookies from MiddlewareResponse #2027

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.

Sign up for GitHub

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

Jump to bottom

Merged

kodiakhq merged 22 commits into main from repro-cookies

Apr 4, 2023

Contributor

Skn0tt commented Apr 4, 2023 •

edited

Loading

Summary

Fixes https://github.com/netlify/pillar-support/issues/350

Test plan

added an e2e test

Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal

Standard checks:

Check the Deploy Preview's Demo site for your PR's functionality
Add docs when necessary

🧪 Once merged, make sure to update the version if needed and that it was published correctly.


          fix: try building repro

bf7c0ef

netlify bot commented Apr 4, 2023 •

edited

Loading

✅ Deploy Preview for next-plugin-edge-middleware ready!

Name	Link
🔨 Latest commit	`8ffb737`
🔍 Latest deploy log	https://app.netlify.com/sites/next-plugin-edge-middleware/deploys/642c7cb93ff1ed0008c08d9d
😎 Deploy Preview	https://deploy-preview-2027--next-plugin-edge-middleware.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Apr 4, 2023 •

edited

Loading

✅ Deploy Preview for netlify-plugin-nextjs-nx-monorepo-demo ready!

Name	Link
🔨 Latest commit	`8ffb737`
🔍 Latest deploy log	https://app.netlify.com/sites/netlify-plugin-nextjs-nx-monorepo-demo/deploys/642c7cb9d69fda0008edf3c2
😎 Deploy Preview	https://deploy-preview-2027--netlify-plugin-nextjs-nx-monorepo-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

github-actions bot added the type: bug label

netlify bot commented Apr 4, 2023 •

edited

Loading

✅ Deploy Preview for next-plugin-canary ready!

Name	Link
🔨 Latest commit	`8ffb737`
🔍 Latest deploy log	https://app.netlify.com/sites/next-plugin-canary/deploys/642c7cb965bc410008b84c96
😎 Deploy Preview	https://deploy-preview-2027--next-plugin-canary.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Apr 4, 2023 •

edited

Loading

✅ Deploy Preview for next-i18next-demo ready!

Name	Link
🔨 Latest commit	`8ffb737`
🔍 Latest deploy log	https://app.netlify.com/sites/next-i18next-demo/deploys/642c7cb9b66d8200073ee6bc
😎 Deploy Preview	https://deploy-preview-2027--next-i18next-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Apr 4, 2023 •

edited

Loading

✅ Deploy Preview for netlify-plugin-nextjs-static-root-demo ready!

Name	Link
🔨 Latest commit	`8ffb737`
🔍 Latest deploy log	https://app.netlify.com/sites/netlify-plugin-nextjs-static-root-demo/deploys/642c7cb9d1c23100080c4b87
😎 Deploy Preview	https://deploy-preview-2027--netlify-plugin-nextjs-static-root-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Apr 4, 2023 •

edited

Loading

✅ Deploy Preview for netlify-plugin-nextjs-demo ready!

Name	Link
🔨 Latest commit	`8ffb737`
🔍 Latest deploy log	https://app.netlify.com/sites/netlify-plugin-nextjs-demo/deploys/642c7cb9f59b3c000742d591
😎 Deploy Preview	https://deploy-preview-2027--netlify-plugin-nextjs-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Apr 4, 2023 •

edited

Loading

✅ Deploy Preview for netlify-plugin-nextjs-next-auth-demo ready!

Name	Link
🔨 Latest commit	`8ffb737`
🔍 Latest deploy log	https://app.netlify.com/sites/netlify-plugin-nextjs-next-auth-demo/deploys/642c7cb9796e8100083dbdc8
😎 Deploy Preview	https://deploy-preview-2027--netlify-plugin-nextjs-next-auth-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Apr 4, 2023 •

edited

Loading

✅ Deploy Preview for nextjs-plugin-custom-routes-demo ready!

Name	Link
🔨 Latest commit	`8ffb737`
🔍 Latest deploy log	https://app.netlify.com/sites/nextjs-plugin-custom-routes-demo/deploys/642c7cb9796e8100083dbdcd
😎 Deploy Preview	https://deploy-preview-2027--nextjs-plugin-custom-routes-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Apr 4, 2023 •

edited

Loading

✅ Deploy Preview for netlify-plugin-nextjs-export-demo ready!

Name	Link
🔨 Latest commit	`8ffb737`
🔍 Latest deploy log	https://app.netlify.com/sites/netlify-plugin-nextjs-export-demo/deploys/642c7cb9ed3f0d0008808381
😎 Deploy Preview	https://deploy-preview-2027--netlify-plugin-nextjs-export-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

Skn0tt added 8 commits

April 4, 2023 11:21


          fix: install next middleware before building

f079df1


          fix: update test expectation

0ff3cf4


          fix: use local version of utils

6b8ae95


          fix: typo

6ce0d37


          fix: some more

ad127f2


          fix: the bug

188c283


          fix: typo

e87c484


          fix: revert changes to next.config.js

3395f20

Skn0tt changed the title ~~fix: try building repro~~ fix: support setting cookies from MiddlewareResponse

Skn0tt added 3 commits

April 4, 2023 12:23


          fix: typo

b93c80a


          fix: oops

f841be2


          fix: header exists (see .has above)

69047bb

Skn0tt self-assigned this


          fix: use headers.get

c9fadfb

ascorbic reviewed

View reviewed changes

test/e2e/modified-tests/middleware-general/app/middleware.js Outdated

Contributor

ascorbic Apr 4, 2023

These files are the tests from the Next.js repo, so shouldn't have new ones added. New ones should go in either Cypress or Jest tests

Contributor

orinokai Apr 4, 2023

that's true (and let me know if you want me to move them @Skn0tt because the folder structure can be a bit difficult to navigate)
however, i've wondered for a while whether we should use the Next.js repo pattern for our own e2e/integration tests - i.e. spinning up small, specific Next.js apps rather than testing against the demo sites, which include a mixed bag of functionality - it seems easier to reason around

Contributor

ascorbic Apr 4, 2023

Absolutely. They should be a new suite of tests though

Contributor Author

Skn0tt Apr 4, 2023

and let me know if you want me to move them @Skn0tt because the folder structure can be a bit difficult to navigate

that'd be lovely, yeah!

i'm not sure wether mix fix actually fixes the problem, but having tests in the right directory would definitely help :D

Contributor

orinokai commented Apr 4, 2023

@taty2010 could you cast your eye over this, because I think you were dealing with a PR not long ago that involved cookies and middleware?

Skn0tt added 3 commits

April 4, 2023 15:51


          fix: move test into cypress code

f4a2f98


          fix: revert changes to e2e

ce7d5ad


          fix: remove console.log

3d38c55

Skn0tt added 2 commits

April 4, 2023 15:56


          fix: add another assertion for headers

2e0f9d4


          revert one more line

0fe1289

Skn0tt marked this pull request as ready for review

April 4, 2023 14:03

Skn0tt requested a review from a team

April 4, 2023 14:03

Contributor Author

Skn0tt commented Apr 4, 2023

got everything working locally, would appreciate some help with the failing CI tests!

Contributor

taty2010 commented Apr 4, 2023

currently looking into the failing tests.

taty2010 and others added 4 commits

April 4, 2023 10:46


          Merge branch 'main' into repro-cookies

983b23a


          chore: trigger build

8ac422a


          Merge branch 'main' into repro-cookies


          chore: updated config to include cookies/middleware

8ffb737

taty2010 approved these changes

View reviewed changes

Contributor

taty2010 left a comment

prev failing tests are now passing and everything else looks gtm

nickytonline approved these changes

View reviewed changes

nickytonline left a comment

Tested this with @taty2010. 🚢

taty2010 added the automerge label

kodiakhq bot merged commit a630ab4 into main

kodiakhq bot deleted the repro-cookies branch

April 4, 2023 21:16

token-generator-app bot mentioned this pull request

chore: release main #2029

Merged

Contributor Author

Skn0tt commented Apr 5, 2023 •

edited

Loading

thanks for taking this across the finish line!

nickytonline mentioned this pull request

fix: checks when Next.js' implementation of ResponseCookies._headers is undefined now #2119

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge type: bug