Skip to content

Commit

Permalink
pages router: ensure x-middleware-cache is respected (#67734)
Browse files Browse the repository at this point in the history
`x-middleware-cache: "no-store"` in Pages router is a way to signal to
the client that it should not store the response in the cache. However
in certain circumstances, namely when `unstable_skipClientCache` is
true, the data request would be awaited and then stored in the
`inflightCache` regardless of the header.

The original implementation of this in the router has logic to delete
the response from the inflight cache after the request has fulfilled
because `inflightCache` stores the unresolved promise. But in this
optimistic prefetch case, when we're only storing it in the cache once
the request is fulfilled, we can prevent a race condition where the
ignored prefetch is erroneously re-added to the cache by ensuring it's
never added to the cache to begin with if the response says not to.

Fixes #66881
Closes NEXT-3550
  • Loading branch information
ztanner committed Jul 13, 2024
1 parent 57baeec commit 9b6f713
Show file tree
Hide file tree
Showing 6 changed files with 74 additions and 2 deletions.
6 changes: 5 additions & 1 deletion packages/next/src/shared/lib/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,11 @@ function fetchNextData({
// without blocking navigation when stale data is available
if (unstable_skipClientCache && persistCache) {
return getData({}).then((data) => {
inflightCache[cacheKey] = Promise.resolve(data)
if (data.response.headers.get('x-middleware-cache') !== 'no-cache') {
// only update cache if not marked as no-cache
inflightCache[cacheKey] = Promise.resolve(data)
}

return data
})
}
Expand Down
2 changes: 2 additions & 0 deletions scripts/test-new-tests.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ async function main() {
...process.env,
NEXT_TEST_MODE: testMode,
NEXT_TEST_VERSION: nextTestVersion,
NEXT_EXTERNAL_TESTS_FILTERS:
testMode === 'deploy' ? 'test/deploy-tests-manifest.json' : undefined,
},
})
}
Expand Down
13 changes: 13 additions & 0 deletions test/e2e/middleware-rewrites/app/middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,19 @@ export async function middleware(request) {
return NextResponse.rewrite(url)
}

if (url.pathname === '/dynamic-no-cache/1') {
const rewriteUrl =
request.headers.get('purpose') === 'prefetch'
? '/dynamic-no-cache/1'
: '/dynamic-no-cache/2'

url.pathname = rewriteUrl

return NextResponse.rewrite(url, {
headers: { 'x-middleware-cache': 'no-cache' },
})
}

if (
url.pathname === '/rewrite-me-without-hard-navigation' ||
url.searchParams.get('path') === 'rewrite-me-without-hard-navigation'
Expand Down
22 changes: 22 additions & 0 deletions test/e2e/middleware-rewrites/app/pages/dynamic-no-cache/[id].js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
export default function Dynamic({ id }) {
return (
<main id="dynamic-page">
<h1>Page {id}</h1>
</main>
)
}

export const getStaticProps = async ({ params }) => {
return {
props: {
id: params.id,
},
}
}

export const getStaticPaths = async () => {
return {
paths: [{ params: { id: '1' } }, { params: { id: '2' } }],
fallback: false,
}
}
1 change: 1 addition & 0 deletions test/e2e/middleware-rewrites/app/pages/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ export default function Home() {
<Link href="/ssg" id="normal-ssg-link">
normal SSG link
</Link>
<Link href="/dynamic-no-cache/1">/dynamic-no-cache/1</Link>
<div />
<a
href=""
Expand Down
32 changes: 31 additions & 1 deletion test/e2e/middleware-rewrites/test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { NextInstance } from 'e2e-utils'
import { check, fetchViaHTTP, retry } from 'next-test-utils'
import { createNext, FileRef } from 'e2e-utils'
import escapeStringRegexp from 'escape-string-regexp'
import { Request } from 'playwright'
import { Request, Response } from 'playwright'

describe('Middleware Rewrite', () => {
let next: NextInstance
Expand Down Expand Up @@ -363,6 +363,36 @@ describe('Middleware Rewrite', () => {
})

if (!(global as any).isNextDev) {
it('should opt out of prefetch caching for dynamic routes', async () => {
const browser = await webdriver(next.url, '/')
await browser.eval('window.__SAME_PAGE = true')
await browser.waitForIdleNetwork()
let hasResolvedPrefetch = false

browser.on('response', async (res: Response) => {
const req = res.request()
const headers = await req.allHeaders()
if (
headers['purpose'] === 'prefetch' &&
req.url().includes('/dynamic-no-cache/1')
) {
hasResolvedPrefetch = true
}
})

await browser.elementByCss("[href='/dynamic-no-cache/1']").moveTo()

await retry(async () => {
expect(hasResolvedPrefetch).toBe(true)
})

await browser.elementByCss("[href='/dynamic-no-cache/1']").click()

await browser.waitForElementByCss('#dynamic-page')
expect(await browser.elementById('dynamic-page').text()).toBe('Page 2')
expect(await browser.eval('window.__SAME_PAGE')).toBe(true)
})

it('should not prefetch non-SSG routes', async () => {
const browser = await webdriver(next.url, '/')

Expand Down

0 comments on commit 9b6f713

Please sign in to comment.