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: Merge link header from middleware with the ones from React #73431

Open
wants to merge 11 commits into
base: canary
Choose a base branch
from
26 changes: 19 additions & 7 deletions packages/next/src/server/app-render/app-render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1688,6 +1688,7 @@ async function renderToStream(
let reactServerResult: null | ReactServerResult = null

const setHeader = res.setHeader.bind(res)
Copy link
Contributor

Choose a reason for hiding this comment

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

we can just use res.appendHeader insteads for React's onHeaders callback. I'd remove the other changes. appending is not always useful. for instance it doesn't make sense for location. If you want to resubmit the PR with just appendHeader for the onHeaders callback I think we can merge quickly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many thanks for the review @gnoff!

I've refactored the fix to use res.appendHeader now instead and reverted the other changes (51fd66b).

I've extracted the part of setting metadata.headers to a shared function to be used for both setHeader and appendHeader—hope that makes sense!

Let me know if there's something else left to do! I've also enabled edits by maintainers if there's something quick you'd like to adjust.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some seemingly unrelated tests failed in the last CI run (example), any chance the tests are a bit flaky?

const appendHeader = res.appendHeader.bind(res)

try {
if (
Expand Down Expand Up @@ -1857,7 +1858,7 @@ async function renderToStream(
nonce: ctx.nonce,
onHeaders: (headers: Headers) => {
headers.forEach((value, key) => {
setHeader(key, value)
appendHeader(key, value)
})
},
maxHeadersLength: renderOpts.reactMaxHeadersLength,
Expand Down Expand Up @@ -2519,14 +2520,25 @@ async function prerenderToStream(
| null
| ReactServerPrerenderResult
| ServerPrerenderStreamResult = null
const setHeader = (name: string, value: string | string[]) => {
res.setHeader(name, value)

const setMetadataHeader = (name: string) => {
metadata.headers ??= {}
metadata.headers[name] = res.getHeader(name)

}
const setHeader = (name: string, value: string | string[]) => {
res.setHeader(name, value)
setMetadataHeader(name)
return res
}
const appendHeader = (name: string, value: string | string[]) => {
if (Array.isArray(value)) {
value.forEach((item) => {
res.appendHeader(name, item)
})
} else {
res.appendHeader(name, value)
}
setMetadataHeader(name)
}

let prerenderStore: PrerenderStore | null = null

Expand Down Expand Up @@ -2875,7 +2887,7 @@ async function prerenderToStream(
},
onHeaders: (headers: Headers) => {
headers.forEach((value, key) => {
setHeader(key, value)
appendHeader(key, value)
})
},
maxHeadersLength: renderOpts.reactMaxHeadersLength,
Expand Down Expand Up @@ -3493,7 +3505,7 @@ async function prerenderToStream(
onError: htmlRendererErrorHandler,
onHeaders: (headers: Headers) => {
headers.forEach((value, key) => {
setHeader(key, value)
appendHeader(key, value)
})
},
maxHeadersLength: renderOpts.reactMaxHeadersLength,
Expand Down
7 changes: 7 additions & 0 deletions test/e2e/app-dir/app-middleware/app-middleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,13 @@ describe('app-dir with middleware', () => {
})
})

it('retains a link response header from the middleware', async () => {
const res = await next.fetch('/preloads')
expect(res.headers.get('link')).toContain(
'<https://example.com/page>; rel="alternate"; hreflang="en"'
)
})

it('should be possible to modify cookies & read them in an RSC in a single request', async () => {
const browser = await next.browser('/rsc-cookies')

Expand Down
13 changes: 13 additions & 0 deletions test/e2e/app-dir/app-middleware/app/preloads/page.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import Image from 'next/image'
import { Inter } from 'next/font/google'

const inter = Inter({ subsets: ['latin'] })

export default function Page() {
return (
<>
<Image priority src="/favicon.ico" alt="favicon" width={16} height={16} />
<h1 className={inter.className}>Hello World</h1>
</>
)
}
9 changes: 9 additions & 0 deletions test/e2e/app-dir/app-middleware/middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,15 @@ export async function middleware(request) {
return res
}

if (request.nextUrl.pathname === '/preloads') {
const res = NextResponse.next({
headers: {
link: '<https://example.com/page>; rel="alternate"; hreflang="en"',
},
})
return res
}

return NextResponse.next({
request: {
headers: headersFromRequest,
Expand Down
Loading