fix: Merge `link` header from middleware with the ones from React #73431

amannn · 2024-12-02T15:10:37Z

amannn · 2024-12-02T15:12:17Z

packages/next/src/server/app-render/app-render.tsx

+  key: string,
+  value: string | string[]
+) => {
+  if (key === 'link' && res.getHeader(key)) {


I've currently applied the behavior for merging header values only for link. That's the only one I've encountered the issue with so far and it also seems to be the only one that React sets currently.

ijjk · 2024-12-02T17:27:28Z

Allow CI Workflow Run

approve CI run for commit: 1e6188b

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

amannn · 2024-12-05T13:34:18Z

@samcx Any chance you could have a look at this PR? I think this should address #69000 :)

ijjk · 2024-12-05T18:55:53Z

Failing test suites

Commit: 4e9b024

__NEXT_EXPERIMENTAL_PPR=true pnpm test-dev test/e2e/persistent-caching/persistent-caching.test.ts (PPR)

persistent-caching > should persistent cache loaders

Expand output

● persistent-caching › should persistent cache loaders

expect(received).toBe(expected) // Object.is equality

Expected: "Timestamp = 1734937342803"
Received: "Timestamp = 1734937363410"

  55 |       const browser = await next.browser('/pages')
  56 |       // TODO Persistent Caching for webpack dev server is broken
> 57 |       expect(await browser.elementByCss('main').text()).toBe(pagesTimestamp)
     |                                                         ^
  58 |       await browser.close()
  59 |     }
  60 |   })

  at Object.toBe (e2e/persistent-caching/persistent-caching.test.ts:57:57)

Read more about building and testing Next.js in contributing.md.

pnpm test-dev test/development/acceptance-app/hydration-error.test.ts

Error overlay for hydration errors in App router > should only show one hydration error when bad nesting happened - p under p
Error overlay for hydration errors in App router > should collapse and uncollapse properly when there are many frames

Expand output

● Error overlay for hydration errors in App router › should only show one hydration error when bad nesting happened - p under p

expect(received).toBe(expected) // Object.is equality

Expected: 2
Received: 1

  567 |     await session.openRedbox()
  568 |
> 569 |     expect(await getRedboxTotalErrorCount(browser)).toBe(2)
      |                                                     ^
  570 |
  571 |     const description = await session.getRedboxDescription()
  572 |     expect(description).toContain(

  at Object.toBe (development/acceptance-app/hydration-error.test.ts:569:53)

● Error overlay for hydration errors in App router › should collapse and uncollapse properly when there are many frames

expect(received).toBe(expected) // Object.is equality

Expected: 4
Received: NaN

  788 |
  789 |     retry(async () => {
> 790 |       expect(await getRedboxTotalErrorCount(browser)).toBe(4)
      |                                                       ^
  791 |     })
  792 |
  793 |     const description = await session.getRedboxDescription()

  at toBe (development/acceptance-app/hydration-error.test.ts:790:55)
  at retry (lib/next-test-utils.ts:806:14)

Read more about building and testing Next.js in contributing.md.

pnpm test-dev test/e2e/app-dir/actions/app-action.test.ts

app-dir action handling > should forward action request to a worker that contains the action handler (node)
app-dir action handling > fetch actions > should handle redirects to routes that provide an invalid RSC response

Expand output

● app-dir action handling › should forward action request to a worker that contains the action handler (node)

page.waitForSelector: Timeout 60000ms exceeded.
Call log:
  - waiting for locator('#other-page')

  423 |     return this.chain(() => {
  424 |       return page
> 425 |         .waitForSelector(selector, { timeout, state: 'attached' })
      |          ^
  426 |         .then(async (el) => {
  427 |           // it seems selenium waits longer and tests rely on this behavior
  428 |           // so we wait for the load event fire before returning

  at waitForSelector (lib/browsers/playwright.ts:425:10)
  at e2e/app-dir/actions/app-action.test.ts:862:7

● app-dir action handling › fetch actions › should handle redirects to routes that provide an invalid RSC response

expect(received).toContain(expected) // indexOf

Expected substring: "Hello from a pages route"
Received string:    "0.25412810211446035
Client
Server
Client and Server
0
+1+1 (Slow)-1*2
redirect to a pages route
submit
test"

  1269 |
  1270 |       await retry(async () => {
> 1271 |         expect(await browser.elementByCss('body').text()).toContain(
       |                                                           ^
  1272 |           'Hello from a pages route'
  1273 |         )
  1274 |         expect(await browser.url()).toBe(`${next.url}/pages-dir`)

  at toContain (e2e/app-dir/actions/app-action.test.ts:1271:59)
  at retry (lib/next-test-utils.ts:806:14)
  at Object.<anonymous> (e2e/app-dir/actions/app-action.test.ts:1270:7)

Read more about building and testing Next.js in contributing.md.

ijjk · 2024-12-05T19:00:27Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	amannn/next.js fix/link-middleware	Change
buildDuration	19.1s	16.3s	N/A
buildDurationCached	15.4s	13s	N/A
nodeModulesSize	416 MB	416 MB	⚠️ +15.8 kB
nextStartRea..uration (ms)	456ms	452ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	amannn/next.js fix/link-middleware	Change
1187-HASH.js gzip	52.4 kB	52.4 kB	N/A
8276.HASH.js gzip	169 B	168 B	N/A
8377-HASH.js gzip	5.36 kB	5.36 kB	N/A
bccd1874-HASH.js gzip	52.8 kB	52.8 kB	N/A
framework-HASH.js gzip	57.5 kB	57.5 kB	N/A
main-app-HASH.js gzip	233 B	235 B	N/A
main-HASH.js gzip	34.1 kB	34.1 kB	N/A
webpack-HASH.js gzip	1.71 kB	1.71 kB	N/A
Overall change	0 B	0 B	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	amannn/next.js fix/link-middleware	Change
polyfills-HASH.js gzip	39.4 kB	39.4 kB	✓
Overall change	39.4 kB	39.4 kB	✓

Client Pages

	vercel/next.js canary	amannn/next.js fix/link-middleware	Change
_app-HASH.js gzip	193 B	193 B	✓
_error-HASH.js gzip	193 B	193 B	✓
amp-HASH.js gzip	512 B	510 B	N/A
css-HASH.js gzip	343 B	342 B	N/A
dynamic-HASH.js gzip	1.84 kB	1.84 kB	✓
edge-ssr-HASH.js gzip	265 B	265 B	✓
head-HASH.js gzip	363 B	362 B	N/A
hooks-HASH.js gzip	393 B	392 B	N/A
image-HASH.js gzip	4.49 kB	4.49 kB	N/A
index-HASH.js gzip	268 B	268 B	✓
link-HASH.js gzip	2.35 kB	2.34 kB	N/A
routerDirect..HASH.js gzip	328 B	328 B	✓
script-HASH.js gzip	397 B	397 B	✓
withRouter-HASH.js gzip	323 B	326 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	3.59 kB	3.59 kB	✓

Client Build Manifests

	vercel/next.js canary	amannn/next.js fix/link-middleware	Change
_buildManifest.js gzip	749 B	746 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	amannn/next.js fix/link-middleware	Change
index.html gzip	523 B	524 B	N/A
link.html gzip	538 B	538 B	✓
withRouter.html gzip	519 B	521 B	N/A
Overall change	538 B	538 B	✓

Edge SSR bundle Size

	vercel/next.js canary	amannn/next.js fix/link-middleware	Change
edge-ssr.js gzip	128 kB	128 kB	N/A
page.js gzip	206 kB	206 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	amannn/next.js fix/link-middleware	Change
middleware-b..fest.js gzip	668 B	666 B	N/A
middleware-r..fest.js gzip	155 B	156 B	N/A
middleware.js gzip	31.2 kB	31.2 kB	N/A
edge-runtime..pack.js gzip	844 B	844 B	✓
Overall change	844 B	844 B	✓

Next Runtimes

	vercel/next.js canary	amannn/next.js fix/link-middleware	Change
274-experime...dev.js gzip	322 B	322 B	✓
274.runtime.dev.js gzip	314 B	314 B	✓
app-page-exp...dev.js gzip	359 kB	359 kB	N/A
app-page-exp..prod.js gzip	129 kB	129 kB	N/A
app-page-tur..prod.js gzip	142 kB	142 kB	N/A
app-page-tur..prod.js gzip	137 kB	137 kB	N/A
app-page.run...dev.js gzip	347 kB	347 kB	N/A
app-page.run..prod.js gzip	125 kB	125 kB	N/A
app-route-ex...dev.js gzip	37.5 kB	37.5 kB	✓
app-route-ex..prod.js gzip	25.5 kB	25.5 kB	✓
app-route-tu..prod.js gzip	25.5 kB	25.5 kB	✓
app-route-tu..prod.js gzip	25.4 kB	25.4 kB	✓
app-route.ru...dev.js gzip	39.2 kB	39.2 kB	✓
app-route.ru..prod.js gzip	25.4 kB	25.4 kB	✓
pages-api-tu..prod.js gzip	9.69 kB	9.69 kB	✓
pages-api.ru...dev.js gzip	11.6 kB	11.6 kB	✓
pages-api.ru..prod.js gzip	9.68 kB	9.68 kB	✓
pages-turbo...prod.js gzip	21.7 kB	21.7 kB	✓
pages.runtim...dev.js gzip	27.5 kB	27.5 kB	✓
pages.runtim..prod.js gzip	21.7 kB	21.7 kB	✓
server.runti..prod.js gzip	916 kB	916 kB	✓
Overall change	1.2 MB	1.2 MB	✓

build cache Overall increase ⚠️

	vercel/next.js canary	amannn/next.js fix/link-middleware	Change
0.pack gzip	2.08 MB	2.08 MB	N/A
index.pack gzip	73 kB	73.7 kB	⚠️ +670 B
Overall change	73 kB	73.7 kB	⚠️ +670 B

Diff details

Diff for main-HASH.js

Diff too large to display

Diff for app-page-exp..ntime.dev.js

Diff too large to display

Diff for app-page-exp..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page.runtime.dev.js

Diff too large to display

Diff for app-page.runtime.prod.js

Diff too large to display

Commit: 4e9b024

samcx · 2024-12-05T22:30:17Z

@amannn It looks like we need to update the test in the comment above mine :one-eye-cowboy:

amannn · 2024-12-05T23:03:49Z

@samcx Thanks for running the CI suite!

Hmm, that's a bit odd, locally the test was passing for me. I've adapted it in 1e6188b to make the test more robust.

Can we have another look if CI passes now?

samcx · 2024-12-06T22:00:54Z

@amannn New conflicts again :oh_no: . I'll add the CI label so we don't need to wait for us again.

# Conflicts: # packages/next/src/server/app-render/app-render.tsx

amannn · 2024-12-09T09:21:24Z

@samcx Tests are passing! 🎉

amannn · 2024-12-12T15:12:44Z

@samcx Any chance you could consider this PR for being merged? You have quite a busy canary branch, would be cool to get this in before there's another conflict 😄

amannn · 2024-12-18T16:00:34Z

@samcx Sorry to bother you again, is there something missing in this PR or could it be merged?

samcx · 2024-12-18T18:47:48Z

@amannn Sorry I haven't able to take a look yet—let me take a look now!

gnoff · 2024-12-20T23:02:37Z

packages/next/src/server/app-render/app-render.tsx

@@ -1634,7 +1651,9 @@ async function renderToStream(

  let reactServerResult: null | ReactServerResult = null

-  const setHeader = res.setHeader.bind(res)


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

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.

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

fix: Merge link header from middleware with the ones from React

4bae4fd

ijjk added tests type: next labels Dec 2, 2024

amannn commented Dec 2, 2024

View reviewed changes

amannn mentioned this pull request Dec 2, 2024

next/font interferes with header links from middleware. #69000

Open

This was referenced Dec 3, 2024

Adding hreflang as examples removes link preloads amannn/next-intl#1591

Closed

Next.js 15 breaks alternate links amannn/next-intl#1442

Open

Merge branch 'canary' into fix/link-middleware

5011994

improve assertion

1e6188b

samcx added the CI approved Approve running CI for fork label Dec 6, 2024

amannn added 4 commits December 9, 2024 08:14

Merge remote-tracking branch 'upstream/canary' into fix/link-middleware

a3dd18e

# Conflicts: # packages/next/src/server/app-render/app-render.tsx

run tests again

6941d9f

another attempt to run tests

3293d23

Merge branch 'canary' into fix/link-middleware

41c3dff

amannn mentioned this pull request Dec 10, 2024

Content Negotiation & hreflang Support amannn/next-intl#1577

Closed

tobiashaefermann mentioned this pull request Dec 13, 2024

Link header is not set in Next.js 15.1 amannn/next-intl#1609

Closed

3 tasks

amannn mentioned this pull request Dec 15, 2024

docs: Update link to next-intl in internationalization docs #73788

Closed

samcx self-requested a review December 18, 2024 18:48

amannn mentioned this pull request Dec 20, 2024

Update docs and examples for async params in Next.js 15 amannn/next-intl#1587

Open

gnoff requested changes Dec 20, 2024

View reviewed changes

amannn added 3 commits December 21, 2024 08:47

Merge remote-tracking branch 'upstream/canary' into fix/link-middleware

81f28f1

use appendHeader instead

51fd66b

rename shared function

fe09dcc

amannn requested a review from gnoff December 21, 2024 08:43

Merge remote-tracking branch 'upstream/canary' into fix/link-middleware

4e9b024

vordgi mentioned this pull request Dec 26, 2024

Fix headers rewriting #71925

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Merge `link` header from middleware with the ones from React #73431

fix: Merge `link` header from middleware with the ones from React #73431

amannn commented Dec 2, 2024

amannn Dec 2, 2024

ijjk commented Dec 2, 2024 •

edited

Loading

amannn commented Dec 5, 2024

ijjk commented Dec 5, 2024 •

edited

Loading

ijjk commented Dec 5, 2024 •

edited

Loading

samcx commented Dec 5, 2024

amannn commented Dec 5, 2024

samcx commented Dec 6, 2024

amannn commented Dec 9, 2024

amannn commented Dec 12, 2024

amannn commented Dec 18, 2024

samcx commented Dec 18, 2024

gnoff Dec 20, 2024

amannn Dec 21, 2024

amannn Dec 21, 2024

		@@ -1634,7 +1651,9 @@ async function renderToStream(

		let reactServerResult: null \| ReactServerResult = null

		const setHeader = res.setHeader.bind(res)

fix: Merge link header from middleware with the ones from React #73431

Are you sure you want to change the base?

fix: Merge link header from middleware with the ones from React #73431

Conversation

amannn commented Dec 2, 2024

amannn Dec 2, 2024

Choose a reason for hiding this comment

ijjk commented Dec 2, 2024 • edited Loading

amannn commented Dec 5, 2024

ijjk commented Dec 5, 2024 • edited Loading

Failing test suites

ijjk commented Dec 5, 2024 • edited Loading

Stats from current PR

samcx commented Dec 5, 2024

amannn commented Dec 5, 2024

samcx commented Dec 6, 2024

amannn commented Dec 9, 2024

amannn commented Dec 12, 2024

amannn commented Dec 18, 2024

samcx commented Dec 18, 2024

gnoff Dec 20, 2024

Choose a reason for hiding this comment

amannn Dec 21, 2024

Choose a reason for hiding this comment

amannn Dec 21, 2024

Choose a reason for hiding this comment

fix: Merge `link` header from middleware with the ones from React #73431

fix: Merge `link` header from middleware with the ones from React #73431

ijjk commented Dec 2, 2024 •

edited

Loading

ijjk commented Dec 5, 2024 •

edited

Loading

ijjk commented Dec 5, 2024 •

edited

Loading