Skip to content

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

Merged
merged 22 commits into from
Apr 4, 2023
Merged

fix: support setting cookies from MiddlewareResponse #2027

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
bf7c0ef
fix: try building repro
Skn0tt Apr 4, 2023
f079df1
fix: install next middleware before building
Skn0tt Apr 4, 2023
0ff3cf4
fix: update test expectation
Skn0tt Apr 4, 2023
6b8ae95
fix: use local version of utils
Skn0tt Apr 4, 2023
6ce0d37
fix: typo
Skn0tt Apr 4, 2023
ad127f2
fix: some more
Skn0tt Apr 4, 2023
188c283
fix: the bug
Skn0tt Apr 4, 2023
e87c484
fix: typo
Skn0tt Apr 4, 2023
3395f20
fix: revert changes to next.config.js
Skn0tt Apr 4, 2023
b93c80a
fix: typo
Skn0tt Apr 4, 2023
f841be2
fix: oops
Skn0tt Apr 4, 2023
69047bb
fix: header exists (see .has above)
Skn0tt Apr 4, 2023
c9fadfb
fix: use headers.get
Skn0tt Apr 4, 2023
f4a2f98
fix: move test into cypress code
Skn0tt Apr 4, 2023
ce7d5ad
fix: revert changes to e2e
Skn0tt Apr 4, 2023
3d38c55
fix: remove console.log
Skn0tt Apr 4, 2023
2e0f9d4
fix: add another assertion for headers
Skn0tt Apr 4, 2023
0fe1289
revert one more line
Skn0tt Apr 4, 2023
983b23a
Merge branch 'main' into repro-cookies
taty2010 Apr 4, 2023
8ac422a
chore: trigger build
taty2010 Apr 4, 2023
1388048
Merge branch 'main' into repro-cookies
taty2010 Apr 4, 2023
8ffb737
chore: updated config to include cookies/middleware
taty2010 Apr 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/runtime/src/templates/edge-shared/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,12 @@ export const buildResponse = async ({
if (request.method === 'HEAD' || request.method === 'OPTIONS') {
return response.originResponse
}

// NextResponse doesn't set cookies onto the originResponse, so we need to copy them over
if (response.headers.has('set-cookie')) {
response.originResponse.headers.set('set-cookie', response.headers.get('set-cookie')!)
}

// If it's JSON we don't need to use the rewriter, we can just parse it
if (response.originResponse.headers.get('content-type')?.includes('application/json')) {
const props = await response.originResponse.json()
Expand Down
8 changes: 8 additions & 0 deletions test/e2e/modified-tests/middleware-general/app/middleware.js
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely. They should be a new suite of tests though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* global globalThis */
import { NextRequest, NextResponse, URLPattern } from 'next/server'
import { MiddlewareRequest } from '@netlify/next'
import magicValue from 'shared-package'

export const config = { regions: 'auto' }
Expand Down Expand Up @@ -71,6 +72,13 @@ export async function middleware(request) {
return NextResponse.rewrite(url)
}

if (url.pathname === '/cookie-repro') {
const mwRequest = new MiddlewareRequest(request)
const mwResponse = await mwRequest.next()
mwResponse.cookies.set('foo', 'bar')
return mwResponse
}

if (url.pathname.startsWith('/fetch-user-agent-default')) {
try {
const apiRoute = new URL(url)
Expand Down
9 changes: 9 additions & 0 deletions test/e2e/modified-tests/middleware-general/test/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ describe('Middleware Runtime', () => {
start: 'next start',
},
},
dependencies: {
'@netlify/next': 'file:' + join(__dirname, '../../../../../packages/next'),
},
startCommand: (global as any).isNextDev ? 'yarn dev' : 'yarn start',
buildCommand: 'yarn build',
env: {
Expand Down Expand Up @@ -465,6 +468,12 @@ describe('Middleware Runtime', () => {
expect(readMiddlewareError(response)).toContain(urlsError)
})

// https://github.com/netlify/pillar-support/issues/350
it('supports setting cookies', async () => {
const response = await fetchViaHTTP(next.url, `/cookie-repro`)
expect(response.headers.get('set-cookie')).toContain('foo')
})

if (!(global as any).isNextDeploy) {
// these errors differ on Vercel
it('should throw when using Request with a relative URL', async () => {
Expand Down