-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
Partial Pre Rendering Headers #59447
Conversation
Tests Passed |
Stats from current PRDefault BuildGeneral 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 | |
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
ebb4587
to
5839ef7
Compare
7b5a465
to
e5e6a04
Compare
5839ef7
to
b0d1ac8
Compare
e5e6a04
to
ecd6a9e
Compare
39878a7
to
97d6669
Compare
97d6669
to
6f3d66a
Compare
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
6f3d66a
to
286b0ee
Compare
9ff9f39
to
03ecbd4
Compare
f4105c1
to
353d2e1
Compare
353d2e1
to
ca4985f
Compare
if (!streamFirstChunk) { | ||
streamFirstChunk = Date.now() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
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') | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now captured in the more fully featured ppr-full
suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
// Ensure we did a full page navigation, and not a client navigation. | ||
beforeNav = await browser.eval('window.beforeNav') | ||
expect(beforeNav).not.toBe(now) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were using Playwright directly you could listen for the "load" event: https://playwright.dev/docs/api/class-page#page-event-load
messages.push({ name, value: list.get(name) }) | ||
const delay = headers.get('x-delay') | ||
if (delay) { | ||
use(new Promise((resolve) => setTimeout(resolve, parseInt(delay, 10)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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:Additionally, the
X-NextJS-Cache
header has been updated for better support for PPR:This also enables the other pathnames in the test suites 🙌🏻
Closes NEXT-1840