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

Partial Pre Rendering Headers #59447

Merged
merged 1 commit into from
Dec 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 25 additions & 11 deletions packages/next/src/server/base-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1294,7 +1294,7 @@ export default abstract class Server<ServerOptions extends Options = Options> {
}

res.statusCode = Number(req.headers['x-invoke-status'])
let err = null
let err: Error | null = null

if (typeof req.headers['x-invoke-error'] === 'string') {
const invokeError = JSON.parse(
Expand Down Expand Up @@ -2601,7 +2601,18 @@ export default abstract class Server<ServerOptions extends Options = Options> {
return null
}

if (isSSG && !this.minimalMode) {
const didPostpone =
cacheEntry.value?.kind === 'PAGE' && !!cacheEntry.value.postponed

if (
isSSG &&
!this.minimalMode &&
// We don't want to send a cache header for requests that contain dynamic
// data. If this is a Dynamic RSC request or wasn't a Prefetch RSC
// request, then we should set the cache header.
!isDynamicRSCRequest &&
(!didPostpone || isPrefetchRSCRequest)
) {
// set x-nextjs-cache header to match the header
// we set for the image-optimizer
res.setHeader(
Expand Down Expand Up @@ -2789,13 +2800,8 @@ export default abstract class Server<ServerOptions extends Options = Options> {
res.statusCode = cachedData.status
}

// Mark that the request did postpone if this is a data request or we're
// testing. It's used to verify that we're actually serving a postponed
// request so we can trust the cache headers.
if (
cachedData.postponed &&
(isRSCRequest || process.env.__NEXT_TEST_MODE)
) {
// Mark that the request did postpone if this is a data request.
if (cachedData.postponed && isRSCRequest) {
res.setHeader(NEXT_DID_POSTPONE_HEADER, '1')
}

Expand All @@ -2813,7 +2819,12 @@ export default abstract class Server<ServerOptions extends Options = Options> {
return {
type: 'rsc',
body: cachedData.html,
revalidate: cacheEntry.revalidate,
// Dynamic RSC responses cannot be cached, even if they're
// configured with `force-static` because we have no way of
// distinguishing between `force-static` and pages that have no
// postponed state.
// TODO: distinguish `force-static` from pages with no postponed state (static)
revalidate: 0,
}
}

Expand Down Expand Up @@ -2881,7 +2892,10 @@ export default abstract class Server<ServerOptions extends Options = Options> {
return {
type: 'html',
body,
revalidate: cacheEntry.revalidate,
// We don't want to cache the response if it has postponed data because
// the response being sent to the client it's dynamic parts are streamed
// to the client on the same request.
revalidate: 0,
}
} else if (isDataReq) {
return {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import React, { Suspense } from 'react'
import { Dynamic } from '../../../../../components/dynamic'

export const dynamic = 'force-dynamic'

export function generateStaticParams() {
return []
}

export default ({ params: { slug } }) => {
return (
<Suspense
fallback={
<Dynamic pathname={`/dynamic/force-dynamic/nested/${slug}`} fallback />
}
>
<Dynamic pathname={`/dynamic/force-dynamic/nested/${slug}`} />
</Suspense>
)
}
10 changes: 3 additions & 7 deletions test/e2e/app-dir/ppr-full/app/dynamic/force-dynamic/page.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,10 @@ import { Dynamic } from '../../../components/dynamic'

export const dynamic = 'force-dynamic'

export default ({ params: { slug } }) => {
export default () => {
return (
<Suspense
fallback={
<Dynamic pathname={`/dynamic/force-dynamic/${slug}`} fallback />
}
>
<Dynamic pathname={`/dynamic/force-dynamic/${slug}`} />
<Suspense fallback={<Dynamic pathname="/dynamic/force-dynamic" fallback />}>
<Dynamic pathname="/dynamic/force-dynamic" />
</Suspense>
)
}
8 changes: 3 additions & 5 deletions test/e2e/app-dir/ppr-full/app/dynamic/force-static/page.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,10 @@ import { Dynamic } from '../../../components/dynamic'
export const dynamic = 'force-static'
export const revalidate = 60

export default ({ params: { slug } }) => {
export default () => {
return (
<Suspense
fallback={<Dynamic pathname={`/dynamic/force-static/${slug}`} fallback />}
>
<Dynamic pathname={`/dynamic/force-static/${slug}`} />
<Suspense fallback={<Dynamic pathname="/dynamic/force-static" fallback />}>
<Dynamic pathname="/dynamic/force-static" />
</Suspense>
)
}
12 changes: 2 additions & 10 deletions test/e2e/app-dir/ppr-full/app/layout.jsx
Original file line number Diff line number Diff line change
@@ -1,18 +1,10 @@
import { Links } from '../components/links'
import { Layout } from '../components/layout'

export default ({ children }) => {
return (
<html>
<body>
<h1>Partial Prerendering</h1>
<p>
Below are links that are associated with different pages that all will
partially prerender
</p>
<aside>
<Links />
</aside>
<main>{children}</main>
<Layout>{children}</Layout>
</body>
</html>
)
Expand Down
10 changes: 6 additions & 4 deletions test/e2e/app-dir/ppr-full/app/static/page.jsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import React from 'react'
import { Dynamic } from '../../components/dynamic'

export default () => {
return <Dynamic pathname="/static" fallback />
return (
<dl>
<dt>Pathname</dt>
<dd data-pathname="/static">/static</dd>
</dl>
)
}
51 changes: 26 additions & 25 deletions test/e2e/app-dir/ppr-full/components/dynamic.jsx
Original file line number Diff line number Diff line change
@@ -1,37 +1,38 @@
import React from 'react'
import { headers } from 'next/headers'
import React, { use } from 'react'
import * as next from 'next/headers'

export const Dynamic = ({ pathname, fallback }) => {
if (fallback) {
return <div>Loading...</div>
return <div>Dynamic Loading...</div>
}

const headers = next.headers()
const messages = []
const names = ['x-test-input', 'user-agent']
const list = headers()
for (const name of ['x-test-input', 'user-agent']) {
messages.push({ name, value: headers.get(name) })
}

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

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:

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

}

return (
<div id="needle">
<dl>
{pathname && (
<>
<dt>Pathname</dt>
<dd>{pathname}</dd>
</>
)}
{messages.map(({ name, value }) => (
<React.Fragment key={name}>
<dt>
Header: <code>{name}</code>
</dt>
<dd>{value ?? 'null'}</dd>
</React.Fragment>
))}
</dl>
</div>
<dl>
{pathname && (
<>
<dt>Pathname</dt>
<dd data-pathname={pathname}>{pathname}</dd>
</>
)}
{messages.map(({ name, value }) => (
<React.Fragment key={name}>
<dt>
Header: <code>{name}</code>
</dt>
<dd>{value ?? `MISSING:${name.toUpperCase()}`}</dd>
</React.Fragment>
))}
</dl>
)
}
18 changes: 18 additions & 0 deletions test/e2e/app-dir/ppr-full/components/layout.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import React from 'react'
import { Links } from './links'

export const Layout = ({ children }) => {
return (
<>
<h1>Partial Prerendering</h1>
<p>
Below are links that are associated with different pages that all will
partially prerender
</p>
<aside>
<Links />
</aside>
<main>{children}</main>
</>
)
}
13 changes: 13 additions & 0 deletions test/e2e/app-dir/ppr-full/components/links.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,18 @@ const links = [
{ href: '/no-suspense/nested/b', tag: 'no suspense, on-demand' },
{ href: '/no-suspense/nested/c', tag: 'no suspense, on-demand' },
{ href: '/dynamic/force-dynamic', tag: "dynamic = 'force-dynamic'" },
{
href: '/dynamic/force-dynamic/nested/a',
tag: "dynamic = 'force-dynamic', on-demand, no-gsp",
},
{
href: '/dynamic/force-dynamic/nested/b',
tag: "dynamic = 'force-dynamic', on-demand, no-gsp",
},
{
href: '/dynamic/force-dynamic/nested/c',
tag: "dynamic = 'force-dynamic', on-demand, no-gsp",
},
{ href: '/dynamic/force-static', tag: "dynamic = 'force-static'" },
{ href: '/edge/suspense', tag: 'edge, pre-generated' },
{ href: '/edge/suspense/a', tag: 'edge, pre-generated' },
Expand All @@ -27,6 +39,7 @@ const links = [
{ href: '/edge/no-suspense/a', tag: 'edge, no suspense, pre-generated' },
{ href: '/edge/no-suspense/b', tag: 'edge, no suspense, on-demand' },
{ href: '/edge/no-suspense/c', tag: 'edge, no suspense, on-demand' },
{ href: '/pages', tag: 'pages' },
]

export const Links = () => {
Expand Down
13 changes: 13 additions & 0 deletions test/e2e/app-dir/ppr-full/pages/pages.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import React from 'react'
import { Layout } from '../components/layout'

export default () => {
return (
<Layout>
<dl>
<dt>Pathname</dt>
<dd data-pathname="/pages">/pages</dd>
</dl>
</Layout>
)
}
Loading