Skip to content

Commit

Permalink
Ensure prefetch heuristic matches with and without middleware (#39920)
Browse files Browse the repository at this point in the history
This fixes the prefetching handling when middleware is present as currently we are incorrectly triggering data requests for non-SSG pages to handle the case where a middleware rewrite is present and pointing to an SSG path. Since the majority use case won't be rewriting in middleware and this incurs a lot of potentially heavy requests to `getServerSideProps` paths this matches the prefetch handling when middleware isn't present and only prefetches the data route when we match an SSG path. 

Fixes: [slack thread](https://vercel.slack.com/archives/C0289CGVAR2/p1661364670589519?thread_ts=1661199566.675759&cid=C0289CGVAR2)
Fixes: #38918

## Bug

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`
  • Loading branch information
ijjk committed Aug 26, 2022
1 parent 6a65dc5 commit f7df3fc
Show file tree
Hide file tree
Showing 6 changed files with 41 additions and 111 deletions.
71 changes: 7 additions & 64 deletions packages/next/shared/lib/router/router.ts
Expand Up @@ -2231,12 +2231,6 @@ export default class Router implements BaseRouter {
? options.locale || undefined
: this.locale

const isMiddlewareMatch = await matchesMiddleware({
asPath: asPath,
locale: locale,
router: this,
})

if (process.env.__NEXT_HAS_REWRITES && asPath.startsWith('/')) {
let rewrites: any
;({ __rewrites: rewrites } = await getClientBuildManifest())
Expand Down Expand Up @@ -2264,9 +2258,7 @@ export default class Router implements BaseRouter {
pathname = rewritesResult.resolvedHref
parsed.pathname = pathname

if (!isMiddlewareMatch) {
url = formatWithValidation(parsed)
}
url = formatWithValidation(parsed)
}
}
parsed.pathname = resolveDynamicRoute(parsed.pathname, pages)
Expand All @@ -2281,74 +2273,25 @@ export default class Router implements BaseRouter {
) || {}
)

if (!isMiddlewareMatch) {
url = formatWithValidation(parsed)
}
url = formatWithValidation(parsed)
}

// Prefetch is not supported in development mode because it would trigger on-demand-entries
if (process.env.NODE_ENV !== 'production') {
return
}

// TODO: if the route middleware's data request
// resolves to is not an SSG route we should bust the cache
// but we shouldn't allow prefetch to keep triggering
// requests for SSP pages
const data = await withMiddlewareEffects({
fetchData: () =>
fetchNextData({
dataHref: this.pageLoader.getDataHref({
href: formatWithValidation({ pathname, query }),
skipInterpolation: true,
asPath: resolvedAs,
locale,
}),
hasMiddleware: true,
isServerRender: this.isSsr,
parseJSON: true,
inflightCache: this.sdc,
persistCache: !this.isPreview,
isPrefetch: true,
}),
asPath: asPath,
locale: locale,
router: this,
})

/**
* If there was a rewrite we apply the effects of the rewrite on the
* current parameters for the prefetch.
*/
if (data?.effect.type === 'rewrite') {
parsed.pathname = data.effect.resolvedHref
pathname = data.effect.resolvedHref
query = { ...query, ...data.effect.parsedAs.query }
resolvedAs = data.effect.parsedAs.pathname
url = formatWithValidation(parsed)
}

/**
* If there is a redirect to an external destination then we don't have
* to prefetch content as it will be unused.
*/
if (data?.effect.type === 'redirect-external') {
return
}

const route = removeTrailingSlash(pathname)

await Promise.all([
this.pageLoader._isSsg(route).then((isSsg) => {
return isSsg
? fetchNextData({
dataHref:
data?.dataHref ||
this.pageLoader.getDataHref({
href: url,
asPath: resolvedAs,
locale: locale,
}),
dataHref: this.pageLoader.getDataHref({
href: url,
asPath: resolvedAs,
locale: locale,
}),
isServerRender: false,
parseJSON: true,
inflightCache: this.sdc,
Expand Down
4 changes: 4 additions & 0 deletions test/e2e/middleware-rewrites/app/pages/index.js
Expand Up @@ -47,6 +47,10 @@ export default function Home() {
<a id="override-with-internal-rewrite">Rewrite me to internal path</a>
</Link>
<div />
<Link href="/ssg">
<a id="normal-ssg-link">normal SSG link</a>
</Link>
<div />
<a
href=""
id="link-to-shallow-push"
Expand Down
14 changes: 14 additions & 0 deletions test/e2e/middleware-rewrites/app/pages/ssg.js
@@ -0,0 +1,14 @@
export default function Main({ now }) {
return (
<div>
<h1 id="ssg">SSG Page</h1>
<p id="now">{now}</p>
</div>
)
}

export const getStaticProps = () => ({
props: {
now: Date.now(),
},
})
11 changes: 2 additions & 9 deletions test/e2e/middleware-rewrites/test/index.test.ts
Expand Up @@ -276,21 +276,14 @@ describe('Middleware Rewrite', () => {
})

if (!(global as any).isNextDev) {
it('should cache data requests correctly', async () => {
it('should not prefetch non-SSG routes', async () => {
const browser = await webdriver(next.url, '/')

await check(async () => {
const hrefs = await browser.eval(
`Object.keys(window.next.router.sdc)`
)
for (const url of [
'/en/about.json?override=external',
'/en/about.json?override=internal',
'/en/rewrite-me-external-twice.json',
'/en/rewrite-me-to-about.json?override=internal',
'/en/rewrite-me-to-vercel.json',
'/en/rewrite-to-ab-test.json',
]) {
for (const url of ['/en/ssg.json']) {
if (!hrefs.some((href) => href.includes(url))) {
return JSON.stringify(hrefs, null, 2)
}
Expand Down
42 changes: 11 additions & 31 deletions test/integration/dynamic-routing/test/index.test.js
Expand Up @@ -46,37 +46,17 @@ function runTests({ dev, serverless }) {
}

const cacheKeys = await getCacheKeys()
expect(cacheKeys).toEqual(
process.env.__MIDDLEWARE_TEST
? [
'/_next/data/BUILD_ID/[name].json?another=value&name=%5Bname%5D',
'/_next/data/BUILD_ID/added-later/first.json?name=added-later&comment=first',
'/_next/data/BUILD_ID/blog/321/comment/123.json?name=321&id=123',
'/_next/data/BUILD_ID/d/dynamic-1.json?id=dynamic-1',
'/_next/data/BUILD_ID/index.json',
'/_next/data/BUILD_ID/on-mount/test-w-hash.json?post=test-w-hash',
'/_next/data/BUILD_ID/p1/p2/all-ssg/hello.json?rest=hello',
'/_next/data/BUILD_ID/p1/p2/all-ssg/hello1/hello2.json?rest=hello1&rest=hello2',
'/_next/data/BUILD_ID/p1/p2/all-ssr/:42.json?rest=%3A42',
'/_next/data/BUILD_ID/p1/p2/all-ssr/hello.json?rest=hello',
'/_next/data/BUILD_ID/p1/p2/all-ssr/hello1%2F/he%2Fllo2.json?rest=hello1%2F&rest=he%2Fllo2',
'/_next/data/BUILD_ID/p1/p2/all-ssr/hello1/hello2.json?rest=hello1&rest=hello2',
'/_next/data/BUILD_ID/p1/p2/nested-all-ssg/hello.json?rest=hello',
'/_next/data/BUILD_ID/p1/p2/nested-all-ssg/hello1/hello2.json?rest=hello1&rest=hello2',
'/_next/data/BUILD_ID/post-1.json?fromHome=true&name=post-1',
'/_next/data/BUILD_ID/post-1.json?hidden=value&name=post-1',
'/_next/data/BUILD_ID/post-1.json?name=post-1',
'/_next/data/BUILD_ID/post-1.json?name=post-1&another=value',
'/_next/data/BUILD_ID/post-1/comment-1.json?name=post-1&comment=comment-1',
'/_next/data/BUILD_ID/post-1/comments.json?name=post-1',
]
: [
'/_next/data/BUILD_ID/p1/p2/all-ssg/hello.json?rest=hello',
'/_next/data/BUILD_ID/p1/p2/all-ssg/hello1/hello2.json?rest=hello1&rest=hello2',
'/_next/data/BUILD_ID/p1/p2/nested-all-ssg/hello.json?rest=hello',
'/_next/data/BUILD_ID/p1/p2/nested-all-ssg/hello1/hello2.json?rest=hello1&rest=hello2',
]
)
expect(cacheKeys).toEqual([
...(process.env.__MIDDLEWARE_TEST
? // data route is fetched with middleware due to query hydration
// since middleware matches the index route
['/_next/data/BUILD_ID/index.json']
: []),
'/_next/data/BUILD_ID/p1/p2/all-ssg/hello.json?rest=hello',
'/_next/data/BUILD_ID/p1/p2/all-ssg/hello1/hello2.json?rest=hello1&rest=hello2',
'/_next/data/BUILD_ID/p1/p2/nested-all-ssg/hello.json?rest=hello',
'/_next/data/BUILD_ID/p1/p2/nested-all-ssg/hello1/hello2.json?rest=hello1&rest=hello2',
])

// ensure no new cache entries after navigation
const links = [
Expand Down
10 changes: 3 additions & 7 deletions test/integration/middleware-prefetch/tests/index.test.js
Expand Up @@ -71,24 +71,20 @@ describe('Middleware Production Prefetch', () => {
const mapped = hrefs.map((href) =>
new URL(href).pathname.replace(/^\/_next\/data\/[^/]+/, '')
)
assert.deepEqual(mapped, [
'/index.json',
'/made-up.json',
'/ssg-page-2.json',
])
assert.deepEqual(mapped, ['/index.json'])
return 'yes'
}, 'yes')
})

it(`doesn't prefetch when the destination will be rewritten`, async () => {
it(`prefetches provided path even if it will be rewritten`, async () => {
const browser = await webdriver(context.appPort, `/`)
await browser.elementByCss('#ssg-page-2').moveTo()
await check(async () => {
const scripts = await browser.elementsByCss('script')
const attrs = await Promise.all(
scripts.map((script) => script.getAttribute('src'))
)
return attrs.find((src) => src.includes('/ssg-page-2')) ? 'nope' : 'yes'
return attrs.find((src) => src.includes('/ssg-page-2')) ? 'yes' : 'nope'
}, 'yes')
})
})

0 comments on commit f7df3fc

Please sign in to comment.