Skip to content

Commit

Permalink
disable static prefetching behavior for dynamic segments (vercel#58609)
Browse files Browse the repository at this point in the history
### What?
When a layout segment forces dynamic rendering (such as with
`force-dynamic` or `revalidate: 0`), navigating to sub-pages of that
layout will attempt to re-render the layout, also resulting in side
effects re-running. This means if your layout relies on a data fetch and
you render the result of that data in the layout, it will unexpectedly
change when navigating between sub-paths, as described in vercel#57326.

As a separate issue (but caused by the same underlying mechanism), when
using `searchParams` on a dynamic page, changes to those search params
will be erroneously ignored when navigating, as described in vercel#57075

### Why?
As a performance optimization we generate static prefetch files for
dynamic segments ([original
PR](vercel#54403)). This makes it so
that when prefetching is turned on, the prefetch can be served quickly
from the edge without needing to invoke unnecessarily. We're able to
eagerly serve things that can be safely prefetched. This is nice for
cases where a path has a `loading.js` that we can eagerly render while
waiting for the dynamic data to be loaded.

This causes a problem with layouts that opt into dynamic rendering: when
the page loads and a prefetch is kicked off for the sub-page, it'll load
the static prefetch, which won't be generated with the same router state
as the dynamically rendered page. This causes a mismatch between the two
trees, and when navigating within the same segment, a refetch will be
added to the router because it thinks that it's navigating to a new
layout.

This also causes issues for dynamic pages that use `searchParams`. The
static prefetch will be generated without any knowledge of search
params, and when the prefetch occurs, we still match to the prefetch
generated without search params. This will make the router think that no
change occurs, and the UI will not update to reflect the change.

### How?
There's ongoing work by @acdlite to refactor the client router.
Hopefully it will be easier to re-land this once that work is finished.
For now, I've reverted the behavior as it doesn't seem to be worth the
bugs it currently causes. I've also added tests so that when we do
re-land this behavior, we can catch these subtleties.

Fixes vercel#57326
Fixes vercel#57075

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
  • Loading branch information
ztanner and kodiakhq[bot] authored Nov 22, 2023
1 parent f6b50ae commit 3043fee
Show file tree
Hide file tree
Showing 13 changed files with 229 additions and 62 deletions.
19 changes: 15 additions & 4 deletions packages/next/src/export/routes/app-page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,14 @@ async function generatePrefetchRsc(
htmlFilepath: string,
renderOpts: RenderOpts,
fileWriter: FileWriter
) {
): Promise<boolean> {
// TODO: Re-enable once this is better supported client-side
// It's currently not reliable to generate these prefetches because the client router
// depends on the RSC payload being generated with FlightRouterState. When we generate these prefetches
// without router state, it causes mismatches on client-side nav, resulting in subtle navigation bugs
// like unnecessarily re-rendering layouts.
return false

// When we're in PPR, the RSC payload is emitted as the prefetch payload, so
// attempting to generate a prefetch RSC is an error.
if (renderOpts.experimental.ppr) {
Expand All @@ -64,13 +71,15 @@ async function generatePrefetchRsc(

const prefetchRscData = await prefetchRenderResult.toUnchunkedString(true)

if ((renderOpts as any).store.staticPrefetchBailout) return
if ((renderOpts as any).store.staticPrefetchBailout) return false

await fileWriter(
ExportedAppPageFiles.FLIGHT,
htmlFilepath.replace(/\.html$/, RSC_PREFETCH_SUFFIX),
prefetchRscData
)

return true
}

export async function exportAppPage(
Expand All @@ -94,7 +103,7 @@ export async function exportAppPage(

try {
if (isAppPrefetch) {
await generatePrefetchRsc(
const generated = await generatePrefetchRsc(
req,
path,
res,
Expand All @@ -104,7 +113,9 @@ export async function exportAppPage(
fileWriter
)

return { revalidate: 0 }
if (generated) {
return { revalidate: 0 }
}
}

const result = await lazyRenderAppPage(
Expand Down
46 changes: 23 additions & 23 deletions packages/next/src/server/base-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ import {
NEXT_RSC_UNION_QUERY,
ACTION,
NEXT_ROUTER_PREFETCH_HEADER,
RSC_CONTENT_TYPE_HEADER,
} from '../client/components/app-router-headers'
import type {
MatchOptions,
Expand Down Expand Up @@ -2315,28 +2314,29 @@ export default abstract class Server<ServerOptions extends Options = Options> {
{ page: pathname, params: opts.params, query, renderOpts }
)
} else if (isAppPageRouteModule(routeModule)) {
if (
!opts.experimental.ppr &&
isPrefetchRSCRequest &&
process.env.NODE_ENV === 'production' &&
!this.minimalMode
) {
try {
const prefetchRsc = await this.getPrefetchRsc(resolvedUrlPathname)
if (prefetchRsc) {
res.setHeader(
'cache-control',
'private, no-cache, no-store, max-age=0, must-revalidate'
)
res.setHeader('content-type', RSC_CONTENT_TYPE_HEADER)
res.body(prefetchRsc).send()
return null
}
} catch {
// We fallback to invoking the function if prefetch data is not
// available.
}
}
// TODO: Re-enable once static prefetches re-land
// if (
// !opts.experimental.ppr &&
// isPrefetchRSCRequest &&
// process.env.NODE_ENV === 'production' &&
// !this.minimalMode
// ) {
// try {
// const prefetchRsc = await this.getPrefetchRsc(resolvedUrlPathname)
// if (prefetchRsc) {
// res.setHeader(
// 'cache-control',
// 'private, no-cache, no-store, max-age=0, must-revalidate'
// )
// res.setHeader('content-type', RSC_CONTENT_TYPE_HEADER)
// res.body(prefetchRsc).send()
// return null
// }
// } catch {
// // We fallback to invoking the function if prefetch data is not
// // available.
// }
// }

const module = components.routeModule as AppPageRouteModule

Expand Down
19 changes: 19 additions & 0 deletions test/e2e/app-dir/app-prefetch/app/force-dynamic/layout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import React from 'react'

export const dynamic = 'force-dynamic'

export default async function Layout({ children }) {
console.log('re-fetching in layout')
const data = await fetch(
'https://next-data-api-endpoint.vercel.app/api/random'
)
const randomNumber = await data.text()

return (
<div>
<p id="random-number">{randomNumber}</p>

{children}
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import Link from 'next/link'

export default function Home({ searchParams }) {
return (
<>
<div id="search-params-data">{JSON.stringify(searchParams)}</div>
<Link href="?foo=true">Add search params</Link>
<Link href="/force-dynamic/search-params">Clear Params</Link>
</>
)
}
10 changes: 10 additions & 0 deletions test/e2e/app-dir/app-prefetch/app/force-dynamic/test-page/page.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import Link from 'next/link'

export default function Page() {
return (
<div id="test-page">
Hello from /force-dynamic/test-page{' '}
<Link href="/force-dynamic/test-page/sub-page">To Sub Page</Link>
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import Link from 'next/link'

export default function Page() {
return (
<div id="sub-page">
Hello from /force-dynamic/test-page/sub-page{' '}
<Link href="/force-dynamic/test-page">Back to Test Page</Link>
</div>
)
}
19 changes: 19 additions & 0 deletions test/e2e/app-dir/app-prefetch/app/revalidate-0/layout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import React from 'react'

export const revalidate = 0

export default async function Layout({ children }) {
console.log('re-fetching in layout')
const data = await fetch(
'https://next-data-api-endpoint.vercel.app/api/random'
)
const randomNumber = await data.text()

return (
<div>
<p id="random-number">{randomNumber}</p>

{children}
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import Link from 'next/link'

export default function Home({ searchParams }) {
return (
<>
<div id="search-params-data">{JSON.stringify(searchParams)}</div>
<Link href="?foo=true">Add search params</Link>
<Link href="/revalidate-0/search-params">Clear Params</Link>
</>
)
}
10 changes: 10 additions & 0 deletions test/e2e/app-dir/app-prefetch/app/revalidate-0/test-page/page.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import Link from 'next/link'

export default function Page() {
return (
<div id="test-page">
Hello from /revalidate-0/test-page{' '}
<Link href="/revalidate-0/test-page/sub-page">To Sub Page</Link>
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import Link from 'next/link'

export default function Page() {
return (
<div id="sub-page">
Hello from /revalidate-0/test-page/sub-page{' '}
<Link href="/revalidate-0/test-page">Back to Test Page</Link>
</div>
)
}
87 changes: 71 additions & 16 deletions test/e2e/app-dir/app-prefetch/prefetching.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -301,24 +301,79 @@ createNextDescribe(
expect(prefetchResponse).toContain('Loading Prefetch Auto')
})

it('should not generate static prefetches for layouts that opt into dynamic rendering', async () => {
await next.stop()
const rootLoading = await next.readFile('./app/loading.js')
await next.deleteFile('./app/loading.js')
await next.start()
expect(
await next
.readFile('.next/server/app/prefetch-dynamic-usage/foo.prefetch.rsc')
.catch(() => false)
).toBeFalsy()
describe('dynamic rendering', () => {
async function hasStaticPrefetch(filePath: string): Promise<boolean> {
try {
await next.readFile(`.next/server/app${filePath}`)
return true
} catch {
return false
}
}
it('should not generate a static prefetch for layouts that use cookies/headers', async () => {
expect(
await hasStaticPrefetch('/prefetch-dynamic-usage/foo.prefetch.rsc')
).toBe(false)

expect(
await next
.readFile('.next/server/app/prefetch-dynamic-usage/foo.prefetch.rsc')
.catch(() => false)
).toBeFalsy()
expect(
await hasStaticPrefetch('/prefetch-dynamic-usage/bar.prefetch.rsc')
).toBe(false)
})

await next.patchFile('./app/loading', rootLoading)
describe.each(['/force-dynamic', '/revalidate-0'])('%s', (basePath) => {
it('should not re-render layout when navigating between sub-pages', async () => {
const logStartIndex = next.cliOutput.length

const browser = await next.browser(`${basePath}/test-page`)
let initialRandomNumber = await browser
.elementById('random-number')
.text()
await browser
.elementByCss(`[href="${basePath}/test-page/sub-page"]`)
.click()

await check(() => browser.hasElementByCssSelector('#sub-page'), true)

const newRandomNumber = await browser
.elementById('random-number')
.text()

expect(initialRandomNumber).toBe(newRandomNumber)

await check(() => {
const logOccurrences =
next.cliOutput.slice(logStartIndex).split('re-fetching in layout')
.length - 1

return logOccurrences
}, 1)
})

it('should update search params following a link click', async () => {
const browser = await next.browser(`${basePath}/search-params`)
await check(
() => browser.elementById('search-params-data').text(),
/{}/
)
await browser.elementByCss('[href="?foo=true"]').click()
await check(
() => browser.elementById('search-params-data').text(),
/{"foo":"true"}/
)
await browser
.elementByCss(`[href="${basePath}/search-params"]`)
.click()
await check(
() => browser.elementById('search-params-data').text(),
/{}/
)
await browser.elementByCss('[href="?foo=true"]').click()
await check(
() => browser.elementById('search-params-data').text(),
/{"foo":"true"}/
)
})
})
})
}
)
Loading

0 comments on commit 3043fee

Please sign in to comment.