Partial Pre Rendering Headers #59447

wyattjoh · 2023-12-09T21:30:15Z

This fixes some of headers (and adds associated tests) for pages when PPR is enabled. Namely, the Cache-Control headers are now returning correctly, reflecting the non-cachability of some requests:

Requests that postpone (dynamic data is streamed after the initial static shell is streamed)
Requests for the Dynamic RSC payload

Additionally, the X-NextJS-Cache header has been updated for better support for PPR:

Requests that postpone no longer return this header as it doesn't reflect the cache state of the request (because it streams)
Requests for the Prefetch RSC now returns the correct cache headers depending on the segment and pre-postpone state

This also enables the other pathnames in the test suites 🙌🏻

Closes NEXT-1840

ijjk · 2023-12-09T21:33:58Z

Tests Passed

ijjk · 2023-12-09T21:35:44Z

Stats from current PR

Default Build

General Overall increase ⚠️

	vercel/next.js canary	vercel/next.js fix/ppr-headers	Change
buildDuration	12.8s	12.7s	N/A
buildDurationCached	7s	6s	N/A
nodeModulesSize	200 MB	200 MB	⚠️ +6.36 kB
nextStartRea..uration (ms)	420ms	427ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	vercel/next.js fix/ppr-headers	Change
170-HASH.js gzip	26.8 kB	26.8 kB	N/A
199.HASH.js gzip	181 B	185 B	N/A
3f784ff6-HASH.js gzip	53.3 kB	53.3 kB	✓
framework-HASH.js gzip	45.2 kB	45.2 kB	✓
main-app-HASH.js gzip	241 B	242 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	N/A
Overall change	98.5 kB	98.5 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	vercel/next.js fix/ppr-headers	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	vercel/next.js fix/ppr-headers	Change
_app-HASH.js gzip	195 B	194 B	N/A
_error-HASH.js gzip	183 B	182 B	N/A
amp-HASH.js gzip	501 B	501 B	✓
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.5 kB	2.5 kB	N/A
edge-ssr-HASH.js gzip	255 B	255 B	✓
head-HASH.js gzip	349 B	350 B	N/A
hooks-HASH.js gzip	368 B	369 B	N/A
image-HASH.js gzip	4.27 kB	4.27 kB	N/A
index-HASH.js gzip	255 B	256 B	N/A
link-HASH.js gzip	2.61 kB	2.6 kB	N/A
routerDirect..HASH.js gzip	311 B	309 B	N/A
script-HASH.js gzip	384 B	384 B	✓
withRouter-HASH.js gzip	307 B	306 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	1.57 kB	1.57 kB	✓

Client Build Manifests

	vercel/next.js canary	vercel/next.js fix/ppr-headers	Change
_buildManifest.js gzip	484 B	482 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	vercel/next.js fix/ppr-headers	Change
index.html gzip	530 B	527 B	N/A
link.html gzip	541 B	542 B	N/A
withRouter.html gzip	524 B	522 B	N/A
Overall change	0 B	0 B	✓

Edge SSR bundle Size

	vercel/next.js canary	vercel/next.js fix/ppr-headers	Change
edge-ssr.js gzip	93.7 kB	93.7 kB	N/A
page.js gzip	146 kB	146 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	vercel/next.js fix/ppr-headers	Change
middleware-b..fest.js gzip	627 B	624 B	N/A
middleware-r..fest.js gzip	151 B	151 B	✓
middleware.js gzip	37.4 kB	37.4 kB	N/A
edge-runtime..pack.js gzip	1.92 kB	1.92 kB	✓
Overall change	2.07 kB	2.07 kB	✓

Next Runtimes

	vercel/next.js canary	vercel/next.js fix/ppr-headers	Change
app-page-exp...dev.js gzip	168 kB	168 kB	✓
app-page-exp..prod.js gzip	94.1 kB	94.1 kB	✓
app-page-tur..prod.js gzip	94.8 kB	94.8 kB	✓
app-page-tur..prod.js gzip	89.4 kB	89.4 kB	✓
app-page.run...dev.js gzip	138 kB	138 kB	✓
app-page.run..prod.js gzip	88.7 kB	88.7 kB	✓
app-route-ex...dev.js gzip	24 kB	24 kB	✓
app-route-ex..prod.js gzip	16.6 kB	16.6 kB	✓
app-route-tu..prod.js gzip	16.6 kB	16.6 kB	✓
app-route-tu..prod.js gzip	16.2 kB	16.2 kB	✓
app-route.ru...dev.js gzip	23.4 kB	23.4 kB	✓
app-route.ru..prod.js gzip	16.2 kB	16.2 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.9 kB	21.9 kB	✓
pages.runtim...dev.js gzip	22.5 kB	22.5 kB	✓
pages.runtim..prod.js gzip	21.9 kB	21.9 kB	✓
server.runti..prod.js gzip	49.4 kB	49.4 kB	N/A
Overall change	881 kB	881 kB	✓

Diff details

Diff for page.js

Diff too large to display

Diff for edge-ssr.js

Diff too large to display

Diff for server.runtime.prod.js

Diff too large to display

Commit: ca4985f

wyattjoh · 2023-12-12T21:42:16Z

Current dependencies on/for this PR:

canary
- PR Partial Pre Rendering Headers #59447 👈

This stack of pull requests is managed by Graphite.

wyattjoh · 2023-12-14T19:29:02Z

test/e2e/app-dir/ppr-full/ppr-full.test.ts

+      if (!streamFirstChunk) {
+        streamFirstChunk = Date.now()
+      }


This is needed so we can verify if the static part was cached or not without requiring the header to be exposed on tests. This is consistent with @acdlite's suggestions on making the tests more "user-facing".

wyattjoh · 2023-12-14T19:30:48Z

test/e2e/app-dir/ppr/ppr.test.ts

-      if (!isNextDev) {
-        it('should cache the static part', async () => {
-          // First, render the page to populate the cache.
-          let res = await next.fetch(pathname)
-          expect(res.status).toBe(200)
-          expect(res.headers.get('x-nextjs-postponed')).toBe('1')
-
-          // Then, render the page again.
-          res = await next.fetch(pathname)
-          expect(res.status).toBe(200)
-          expect(res.headers.get('x-nextjs-cache')).toBe('HIT')
-          expect(res.headers.get('x-nextjs-postponed')).toBe('1')
-        })
-      }


This is now captured in the more fully featured ppr-full suite.

acdlite

I don't have confidence in my ability to review the actual fix because the base-server module confuses the hell out of me, but the test coverage looks good 😆 so I trust you

acdlite · 2023-12-14T19:51:14Z

test/e2e/app-dir/ppr-full/ppr-full.test.ts

+
+              // Ensure we did a full page navigation, and not a client navigation.
+              beforeNav = await browser.eval('window.beforeNav')
+              expect(beforeNav).not.toBe(now)


If we were using Playwright directly you could listen for the "load" event: https://playwright.dev/docs/api/class-page#page-event-load

acdlite · 2023-12-14T20:05:06Z

test/e2e/app-dir/ppr-full/components/dynamic.jsx

-    messages.push({ name, value: list.get(name) })
+  const delay = headers.get('x-delay')
+  if (delay) {
+    use(new Promise((resolve) => setTimeout(resolve, parseInt(delay, 10))))


Not worth blocking the PR, but: I don't love the use of timers to detect I/O because it's inherently a bit flaky (something unrelated could cause it to be too slow) and it makes the tests run slower.

What I'm planning to do for testing client navigations is spin up an HTTP server and push to an event log whenever the app sends a request, like this:

next.js/test/e2e/app-dir/ppr-navigations/ppr-navigations.test.ts

Lines 19 to 80 in 6b3d145

const TestLog = createTestLog()

let pendingRequests = new Map()

server = createTestDataServer(async (key, res) => {

TestLog.log('REQUEST: ' + key)

if (pendingRequests.has(key)) {

throw new Error('Request already pending for ' + key)

}

pendingRequests.set(key, res)

})

const port = await findPort()

server.listen(port)

next = await createNext({

files: __dirname,

env: { TEST_DATA_SERVICE_URL: `http://localhost:${port}` },

})

// There should have been no data requests during build

TestLog.assert([])

const browser = await next.browser(

'/loading-tsx-no-partial-rendering/start'

)

// Use a text input to set the target URL.

const input = await browser.elementByCss('input')

await input.fill('/loading-tsx-no-partial-rendering/yay')

// This causes a <Link> to appear. (We create the Link after initial render

// so we can control when the prefetch happens.)

const link = await browser.elementByCss('a')

expect(await link.getAttribute('href')).toBe(

'/loading-tsx-no-partial-rendering/yay'

)

// The <Link> triggers a prefetch. Even though this route has a loading.tsx

// boundary, we're still able to prefetch the static data in the page.

// Without PPR, we would have stopped prefetching at the loading.tsx

// boundary. (The dynamic data is not fetched until navigation.)

await TestLog.waitFor(['REQUEST: yay [static]'])

// Navigate. This will trigger the dynamic fetch.

await link.click()

// TODO: Even though the prefetch request hasn't resolved yet, we should

// have already started fetching the dynamic data. Currently, the dynamic

// is fetched lazily during rendering, creating a waterfall. The plan is to

// remove this waterfall by initiating the fetch directly inside the

// router navigation handler, not during render.

TestLog.assert([])

// Finish loading the static data

pendingRequests.get('yay [static]').resolve()

// The static UI appears

await browser.elementById('static')

const container = await browser.elementById('container')

expect(await container.innerHTML()).toEqual(

'Loading dynamic...<div id="static">yay [static]</div>'

)

// The dynamic data is fetched

TestLog.assert(['REQUEST: yay [dynamic]'])

I like the event log pattern for testing because it also detects things like if the same fetch happens more than once, which is useful for catching caching bugs.

Specifically the delay is to test the "sending the static part of the payload first" behavior of PPR. From my understanding, it's the most black-box way of determining if the static part was cached + sent versus regenerated. I'm not too sure how the logs accomplish the same thing.

I could see using a server like this to instead to send a specific "random" value for it to use (the static value) and then a random value on future renders to help distinguish between what was rendered + returned statically and what was streamed dynamically without reaching into the implementation of it.

I think the way I'd write the test is to check that the static content appears in the UI but no requests were made to the HTTP service. The implication then is that the content must have been cached because how else would it have appeared on the page.

ijjk added area: tests created-by: Next.js team PRs by the Next.js team type: next labels Dec 9, 2023

wyattjoh force-pushed the fix/ppr-headers branch 3 times, most recently from ebb4587 to 5839ef7 Compare December 10, 2023 04:09

wyattjoh force-pushed the refactor/page-info branch from 7b5a465 to e5e6a04 Compare December 11, 2023 22:21

wyattjoh force-pushed the fix/ppr-headers branch from 5839ef7 to b0d1ac8 Compare December 11, 2023 22:22

wyattjoh force-pushed the refactor/page-info branch from e5e6a04 to ecd6a9e Compare December 11, 2023 22:23

wyattjoh force-pushed the fix/ppr-headers branch 2 times, most recently from 39878a7 to 97d6669 Compare December 11, 2023 23:59

Base automatically changed from refactor/page-info to canary December 12, 2023 21:37

wyattjoh force-pushed the fix/ppr-headers branch from 97d6669 to 6f3d66a Compare December 12, 2023 21:42

wyattjoh force-pushed the fix/ppr-headers branch from 6f3d66a to 286b0ee Compare December 12, 2023 22:11

wyattjoh marked this pull request as ready for review December 13, 2023 14:45

wyattjoh requested review from timneutkens, ijjk, shuding, huozhi, feedthejim and ztanner as code owners December 13, 2023 14:45

wyattjoh force-pushed the fix/ppr-headers branch 2 times, most recently from 9ff9f39 to 03ecbd4 Compare December 14, 2023 00:19

wyattjoh marked this pull request as draft December 14, 2023 18:37

wyattjoh force-pushed the fix/ppr-headers branch 3 times, most recently from f4105c1 to 353d2e1 Compare December 14, 2023 19:27

fix: ensure correct headers are emitted with PPR

ca4985f

wyattjoh force-pushed the fix/ppr-headers branch from 353d2e1 to ca4985f Compare December 14, 2023 19:29

wyattjoh commented Dec 14, 2023

View reviewed changes

wyattjoh marked this pull request as ready for review December 14, 2023 19:30

acdlite approved these changes Dec 14, 2023

View reviewed changes

wyattjoh merged commit 2da04af into canary Dec 14, 2023
69 checks passed

wyattjoh deleted the fix/ppr-headers branch December 14, 2023 20:14

github-actions bot added the locked label Dec 29, 2023

github-actions bot locked as resolved and limited conversation to collaborators Dec 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial Pre Rendering Headers #59447

Partial Pre Rendering Headers #59447

wyattjoh commented Dec 9, 2023 •

edited by padmaia

ijjk commented Dec 9, 2023 •

edited

ijjk commented Dec 9, 2023 •

edited

wyattjoh commented Dec 12, 2023

wyattjoh Dec 14, 2023

wyattjoh Dec 14, 2023

acdlite left a comment

acdlite Dec 14, 2023

acdlite Dec 14, 2023

wyattjoh Dec 14, 2023

acdlite Dec 14, 2023

	const TestLog = createTestLog()
	let pendingRequests = new Map()
	server = createTestDataServer(async (key, res) => {
	TestLog.log('REQUEST: ' + key)
	if (pendingRequests.has(key)) {
	throw new Error('Request already pending for ' + key)
	}
	pendingRequests.set(key, res)
	})
	const port = await findPort()
	server.listen(port)
	next = await createNext({
	files: __dirname,
	env: { TEST_DATA_SERVICE_URL: `http://localhost:${port}` },
	})

	// There should have been no data requests during build
	TestLog.assert([])

	const browser = await next.browser(
	'/loading-tsx-no-partial-rendering/start'
	)

	// Use a text input to set the target URL.
	const input = await browser.elementByCss('input')
	await input.fill('/loading-tsx-no-partial-rendering/yay')

	// This causes a <Link> to appear. (We create the Link after initial render
	// so we can control when the prefetch happens.)
	const link = await browser.elementByCss('a')
	expect(await link.getAttribute('href')).toBe(
	'/loading-tsx-no-partial-rendering/yay'
	)

	// The <Link> triggers a prefetch. Even though this route has a loading.tsx
	// boundary, we're still able to prefetch the static data in the page.
	// Without PPR, we would have stopped prefetching at the loading.tsx
	// boundary. (The dynamic data is not fetched until navigation.)
	await TestLog.waitFor(['REQUEST: yay [static]'])

	// Navigate. This will trigger the dynamic fetch.
	await link.click()

	// TODO: Even though the prefetch request hasn't resolved yet, we should
	// have already started fetching the dynamic data. Currently, the dynamic
	// is fetched lazily during rendering, creating a waterfall. The plan is to
	// remove this waterfall by initiating the fetch directly inside the
	// router navigation handler, not during render.
	TestLog.assert([])

	// Finish loading the static data
	pendingRequests.get('yay [static]').resolve()

	// The static UI appears
	await browser.elementById('static')
	const container = await browser.elementById('container')
	expect(await container.innerHTML()).toEqual(
	'Loading dynamic...<div id="static">yay [static]</div>'
	)

	// The dynamic data is fetched
	TestLog.assert(['REQUEST: yay [dynamic]'])

Partial Pre Rendering Headers #59447

Partial Pre Rendering Headers #59447

Conversation

wyattjoh commented Dec 9, 2023 • edited by padmaia

ijjk commented Dec 9, 2023 • edited

Tests Passed

ijjk commented Dec 9, 2023 • edited

Stats from current PR

wyattjoh commented Dec 12, 2023

wyattjoh Dec 14, 2023

Choose a reason for hiding this comment

wyattjoh Dec 14, 2023

Choose a reason for hiding this comment

acdlite left a comment

Choose a reason for hiding this comment

acdlite Dec 14, 2023

Choose a reason for hiding this comment

acdlite Dec 14, 2023

Choose a reason for hiding this comment

wyattjoh Dec 14, 2023

Choose a reason for hiding this comment

acdlite Dec 14, 2023

Choose a reason for hiding this comment

wyattjoh commented Dec 9, 2023 •

edited by padmaia

ijjk commented Dec 9, 2023 •

edited

ijjk commented Dec 9, 2023 •

edited