Forward headers from React to static output and dynamic render #58162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sebmarkbage
requested review from
timneutkens,
ijjk,
shuding,
huozhi,
feedthejim,
ztanner and
wyattjoh
as code owners
sebmarkbage
commented
Nov 8, 2023
| @@ -184,9 +185,10 @@ export async function exportAppPage( | |||
| ) | |||
| } | |||
|
|
|||
| let headers: OutgoingHttpHeaders | undefined | |||
| const headers = toNodeOutgoingHttpHeaders(res.headers) | |||
There was a problem hiding this comment.
This is the case where a static build is generated.
It mirrors what we already do for custom routes:
Failing test suitesCommit: 1787df9
Expand output● ReactRefreshLogBox default › Module not found (missing global CSS) Read more about building and testing Next.js in contributing.md.
Expand output● app-dir static/dynamic handling › should output HTML/RSC files for static paths ● app-dir static/dynamic handling › should have correct prerender-manifest entries Read more about building and testing Next.js in contributing.md. |
sebmarkbage
commented
Nov 8, 2023
| @@ -737,9 +737,17 @@ async function renderToHTMLOrFlightImpl( | |||
| hasPostponed, | |||
| }) | |||
|
|
|||
| function onHeaders(headers: Headers): void { | |||
| // Copy headers created by React into the response object. | |||
| headers.forEach((value: string, key: string) => { | |||
There was a problem hiding this comment.
This will only actually be the Link: header so far but we could expand it later.
Stats from current PRDefault Build (Increase detected
|
| vercel/next.js canary | sebmarkbage/next.js onheaders | Change | |
|---|---|---|---|
| buildDuration | 10.4s | 10.5s | N/A |
| buildDurationCached | 6s | 6.5s | |
| nodeModulesSize | 175 MB | 175 MB | |
| nextStartRea..uration (ms) | 418ms | 421ms | N/A |
Client Bundles (main, webpack)
| vercel/next.js canary | sebmarkbage/next.js onheaders | Change | |
|---|---|---|---|
| 199-HASH.js gzip | 29.2 kB | 29.3 kB | N/A |
| 3f784ff6-HASH.js gzip | 53.2 kB | 53.2 kB | ✓ |
| 494.HASH.js gzip | 180 B | 181 B | N/A |
| framework-HASH.js gzip | 45.2 kB | 45.2 kB | ✓ |
| main-app-HASH.js gzip | 241 B | 239 B | N/A |
| main-HASH.js gzip | 31.7 kB | 31.7 kB | N/A |
| webpack-HASH.js gzip | 1.7 kB | 1.7 kB | ✓ |
| Overall change | 100 kB | 100 kB | ✓ |
Legacy Client Bundles (polyfills)
| vercel/next.js canary | sebmarkbage/next.js onheaders | Change | |
|---|---|---|---|
| polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
| Overall change | 31 kB | 31 kB | ✓ |
Client Pages
| vercel/next.js canary | sebmarkbage/next.js onheaders | Change | |
|---|---|---|---|
| _app-HASH.js gzip | 194 B | 195 B | N/A |
| _error-HASH.js gzip | 182 B | 181 B | N/A |
| amp-HASH.js gzip | 504 B | 506 B | N/A |
| css-HASH.js gzip | 322 B | 323 B | N/A |
| dynamic-HASH.js gzip | 2.5 kB | 2.5 kB | ✓ |
| edge-ssr-HASH.js gzip | 253 B | 255 B | N/A |
| head-HASH.js gzip | 348 B | 347 B | N/A |
| hooks-HASH.js gzip | 369 B | 368 B | N/A |
| image-HASH.js gzip | 4.3 kB | 4.3 kB | N/A |
| index-HASH.js gzip | 256 B | 256 B | ✓ |
| link-HASH.js gzip | 2.65 kB | 2.65 kB | N/A |
| routerDirect..HASH.js gzip | 311 B | 311 B | ✓ |
| script-HASH.js gzip | 384 B | 383 B | N/A |
| withRouter-HASH.js gzip | 307 B | 308 B | N/A |
| 1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
| Overall change | 3.17 kB | 3.17 kB | ✓ |
Client Build Manifests
| vercel/next.js canary | sebmarkbage/next.js onheaders | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 483 B | 484 B | N/A |
| Overall change | 0 B | 0 B | ✓ |
Rendered Page Sizes
| vercel/next.js canary | sebmarkbage/next.js onheaders | Change | |
|---|---|---|---|
| index.html gzip | 529 B | 527 B | N/A |
| link.html gzip | 541 B | 540 B | N/A |
| withRouter.html gzip | 524 B | 523 B | N/A |
| Overall change | 0 B | 0 B | ✓ |
Edge SSR bundle Size Overall increase ⚠️
| vercel/next.js canary | sebmarkbage/next.js onheaders | Change | |
|---|---|---|---|
| edge-ssr.js gzip | 92.1 kB | 92.2 kB | N/A |
| page.js gzip | 145 kB | 145 kB | |
| Overall change | 145 kB | 145 kB |
Middleware size
| vercel/next.js canary | sebmarkbage/next.js onheaders | Change | |
|---|---|---|---|
| middleware-b..fest.js gzip | 626 B | 627 B | N/A |
| middleware-r..fest.js gzip | 150 B | 151 B | N/A |
| middleware.js gzip | 24.8 kB | 24.8 kB | N/A |
| edge-runtime..pack.js gzip | 1.92 kB | 1.92 kB | ✓ |
| Overall change | 1.92 kB | 1.92 kB | ✓ |
Next Runtimes
| vercel/next.js canary | sebmarkbage/next.js onheaders | Change | |
|---|---|---|---|
| app-page-exp...dev.js gzip | 166 kB | 166 kB | N/A |
| app-page-exp..prod.js gzip | 93.3 kB | 93.4 kB | N/A |
| app-page-tur..prod.js gzip | 94.1 kB | 94.1 kB | N/A |
| app-page-tur..prod.js gzip | 88.7 kB | 88.8 kB | N/A |
| app-page.run...dev.js gzip | 136 kB | 136 kB | N/A |
| app-page.run..prod.js gzip | 88 kB | 88.1 kB | N/A |
| app-route-ex...dev.js gzip | 23.5 kB | 23.5 kB | ✓ |
| app-route-ex..prod.js gzip | 16.4 kB | 16.4 kB | ✓ |
| app-route-tu..prod.js gzip | 16.4 kB | 16.4 kB | ✓ |
| app-route-tu..prod.js gzip | 16 kB | 16 kB | ✓ |
| app-route.ru...dev.js gzip | 22.9 kB | 22.9 kB | ✓ |
| app-route.ru..prod.js gzip | 16 kB | 16 kB | ✓ |
| pages-api-tu..prod.js gzip | 9.37 kB | 9.37 kB | ✓ |
| pages-api.ru...dev.js gzip | 9.64 kB | 9.64 kB | ✓ |
| pages-api.ru..prod.js gzip | 9.37 kB | 9.37 kB | ✓ |
| pages-turbo...prod.js gzip | 21.5 kB | 21.5 kB | ✓ |
| pages.runtim...dev.js gzip | 22.2 kB | 22.2 kB | ✓ |
| pages.runtim..prod.js gzip | 21.5 kB | 21.5 kB | ✓ |
| server.runti..prod.js gzip | 48.5 kB | 48.6 kB | N/A |
| Overall change | 205 kB | 205 kB | ✓ |
Diff details
Diff for page.js
Diff too large to display
Diff for edge-ssr.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
Diff for server.runtime.prod.js
Diff too large to display
sebmarkbage
commented
Nov 8, 2023
| @@ -2373,10 +2373,12 @@ export default abstract class Server<ServerOptions extends Options = Options> { | |||
|
|
|||
| // Add any fetch tags that were on the page to the response headers. | |||
| const cacheTags = metadata.fetchTags | |||
|
|
|||
| // Copy the headers from the response. | |||
| headers = { ...res.getHeaders() } | |||
There was a problem hiding this comment.
This is the case where we lazily on-demand render a static page or the static part of a PPR page. This one is a bit sketchy. I'm not sure if this is the best way and if this res is actually always going to be the same response as what we expect here.
I'm trying to read back the headers we've already added in the render but I'm not sure this is 100% safe.
There was a problem hiding this comment.
This is the cause of the test failure because this response will have too many headers that we're not actually interested in writing to the meta data in it.
There was a problem hiding this comment.
Went with another approach: f501fce
sebmarkbage
commented
Nov 8, 2023
| const resHeaders = cachedData.headers | ||
| for (const key of Object.keys(resHeaders)) { | ||
| if (key === NEXT_CACHE_TAGS_HEADER) { | ||
| // Not sure if needs to be special. |
There was a problem hiding this comment.
This check is trying to mimic this behavior:
ijjk
reviewed
Nov 8, 2023
There was a problem hiding this comment.
This error seems related:
● app dir rendering › ISR › should revalidate the page when revalidate is configured
FetchError: Invalid response body while trying to fetch http://localhost:34825/isr-multiple/nested: incorrect header checkMaybe we should also copy this test and add it to a suite with the flag enabled
next.js/test/e2e/app-dir/app-static/app-static.test.ts
Lines 1271 to 1274 in 3977f90
wyattjoh
reviewed
Nov 8, 2023
sebmarkbage
force-pushed
the
onheaders
branch
from
d368b8d to
f4cd323
Compare
This ensures that we only track the static headers we want to track.
sebmarkbage
force-pushed
the
onheaders
branch
3 times, most recently
from
f93e582 to
762f10a
Compare
wyattjoh
approved these changes
Nov 8, 2023
Comment on lines
+2781
to
+2786
| if (typeof v !== 'undefined') { | ||
| if (typeof v === 'number') { | ||
| v = v.toString() | ||
| } | ||
| res.setHeader(key, v) | ||
| } |
There was a problem hiding this comment.
Personally prefer to perform an early continue/return rather than add more nesting, but not required!
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
React can emit a
Link:header for preloads instead of<link rel="preload">in certain scenarios when that can be useful. This works by listening to theonHeadersevent.In particular it's interesting for PPR because if you have something dynamic outside a Suspense boundary it generates an empty payload without any preloads in it. That's because when we do render the real shell we don't know what the document will look like. However, we can emit the
Linkheader for CSS, images and font preloads that we've already discovered. In effect, even a dynamic page gets PPR benefits by early fetching resources.Custom headers is supported for static a ROUTE but not a PAGE. So I had to add similar wiring to forward headers when it's a page being rendered.
It's important that this works every where, including dynamic routes, because otherwise we might miss out on preloads that we previously would've had.