Skip to content

Commit

Permalink
Ensure client cache keys match between prefetch and transition (#38089)
Browse files Browse the repository at this point in the history 
* Ensure client cache keys match between prefetch and transition

* handle some middleware cases

* lint-fix

* fix lint rename
  • Loading branch information
@ijjk
ijjk committed Jun 28, 2022
1 parent a72e136 commit d959d28
Show file tree
Hide file tree
Showing 2 changed files with 148 additions and 25 deletions.
80 changes: 55 additions & 25 deletions packages/next/shared/lib/router/router.ts
View file
Expand Up @@ -1199,7 +1199,7 @@ export default class Router implements BaseRouter {
router: this,
}))

if (!isMiddlewareMatch && shouldResolveHref && pathname !== '/_error') {
if (shouldResolveHref && pathname !== '/_error') {
;(options as any)._shouldResolveHref = true

if (process.env.__NEXT_HAS_REWRITES && as.startsWith('/')) {
Expand All @@ -1216,22 +1216,30 @@ export default class Router implements BaseRouter {
handleHardNavigation({ url: as, router: this })
return true
}
resolvedAs = rewritesResult.asPath
if (!isMiddlewareMatch) {
resolvedAs = rewritesResult.asPath
}

if (rewritesResult.matchedPage && rewritesResult.resolvedHref) {
// if this directly matches a page we need to update the href to
// allow the correct page chunk to be loaded
pathname = rewritesResult.resolvedHref
parsed.pathname = addBasePath(pathname)
url = formatWithValidation(parsed)

if (!isMiddlewareMatch) {
url = formatWithValidation(parsed)
}
}
} else {
parsed.pathname = resolveDynamicRoute(pathname, pages)

if (parsed.pathname !== pathname) {
pathname = parsed.pathname
parsed.pathname = addBasePath(pathname)
url = formatWithValidation(parsed)

if (!isMiddlewareMatch) {
url = formatWithValidation(parsed)
}
}
}
}
Expand All @@ -1250,13 +1258,14 @@ export default class Router implements BaseRouter {
resolvedAs = removeLocale(removeBasePath(resolvedAs), nextState.locale)

let route = removeTrailingSlash(pathname)
let routeMatch: { [paramName: string]: string | string[] } | false = false

if (!isMiddlewareMatch && isDynamicRoute(route)) {
if (isDynamicRoute(route)) {
const parsedAs = parseRelativeUrl(resolvedAs)
const asPathname = parsedAs.pathname

const routeRegex = getRouteRegex(route)
const routeMatch = getRouteMatcher(routeRegex)(asPathname)
routeMatch = getRouteMatcher(routeRegex)(asPathname)
const shouldInterpolate = route === asPathname
const interpolatedAs = shouldInterpolate
? interpolateAs(route, asPathname, query)
Expand All @@ -1267,7 +1276,7 @@ export default class Router implements BaseRouter {
(param) => !query[param]
)

if (missingParams.length > 0) {
if (missingParams.length > 0 && !isMiddlewareMatch) {
if (process.env.NODE_ENV !== 'production') {
console.warn(
`${
Expand Down Expand Up @@ -1329,6 +1338,14 @@ export default class Router implements BaseRouter {
route = pathname
query = Object.assign({}, routeInfo.query || {}, query)

if (routeMatch && pathname !== parsed.pathname) {

This comment has been minimized.

Sign in to view

Copy link
@samuelcole

samuelcole Sep 7, 2022

pathname !== parsed.pathname when there's a basePath configured (pathname does not have the basePath, but parsed.pathname does), which means the query params are removed.

this means that the client-side routing is missing query params when a basePath is configured for me.

This comment has been minimized.

Sign in to view

Copy link
@samuelcole

samuelcole Sep 8, 2022

#38528
Object.keys(routeMatch).forEach((key) => {
if (routeMatch && query[key] === routeMatch[key]) {
delete query[key]
}
})
}

if (isDynamicRoute(pathname)) {
const prefixedAs =
routeInfo.resolvedAs ||
Expand All @@ -1346,10 +1363,10 @@ export default class Router implements BaseRouter {
rewriteAs = localeResult.pathname
}
const routeRegex = getRouteRegex(pathname)
const routeMatch = getRouteMatcher(routeRegex)(rewriteAs)
const curRouteMatch = getRouteMatcher(routeRegex)(rewriteAs)

if (routeMatch) {
Object.assign(query, routeMatch)
if (curRouteMatch) {
Object.assign(query, curRouteMatch)
}
}
}
Expand Down Expand Up @@ -1994,6 +2011,17 @@ export default class Router implements BaseRouter {
const pages = await this.pageLoader.getPageList()
let resolvedAs = asPath

const locale =
typeof options.locale !== 'undefined'
? 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 All @@ -2020,18 +2048,25 @@ export default class Router implements BaseRouter {
// allow the correct page chunk to be loaded
pathname = rewritesResult.resolvedHref
parsed.pathname = pathname
url = formatWithValidation(parsed)

if (!isMiddlewareMatch) {
url = formatWithValidation(parsed)
}
}
} else {
parsed.pathname = resolveDynamicRoute(parsed.pathname, pages)
}
parsed.pathname = resolveDynamicRoute(parsed.pathname, pages)

if (parsed.pathname !== pathname) {
pathname = parsed.pathname
parsed.pathname = pathname
Object.assign(
query,
getRouteMatcher(getRouteRegex(parsed.pathname))(asPath) || {}
)
if (isDynamicRoute(parsed.pathname)) {
pathname = parsed.pathname
parsed.pathname = pathname
Object.assign(
query,
getRouteMatcher(getRouteRegex(parsed.pathname))(
parsePath(asPath).pathname
) || {}
)

if (!isMiddlewareMatch) {
url = formatWithValidation(parsed)
}
}
Expand All @@ -2041,11 +2076,6 @@ export default class Router implements BaseRouter {
return
}

const locale =
typeof options.locale !== 'undefined'
? options.locale || undefined
: this.locale

// 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
Expand Down
93 changes: 93 additions & 0 deletions test/integration/dynamic-routing/test/index.test.js
View file
Expand Up @@ -28,6 +28,99 @@ const appDir = join(__dirname, '../')
const buildIdPath = join(appDir, '.next/BUILD_ID')

function runTests({ dev, serverless }) {
if (!dev) {
it('should have correct cache entries on prefetch', async () => {
const browser = await webdriver(appPort, '/')
await browser.waitForCondition('!!window.next.router.isReady')

const getCacheKeys = async () => {
return (await browser.eval('Object.keys(window.next.router.sdc)'))
.map((key) => {
// strip http://localhost:PORT
// and then strip buildId prefix
return key
.substring(key.indexOf('/_next'))
.replace(/\/_next\/data\/(.*?)\//, '/_next/data/BUILD_ID/')
})
.sort()
}

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',
]
)

// ensure no new cache entries after navigation
const links = [
{
linkSelector: '#ssg-catch-all-single',
waitForSelector: '#all-ssg-content',
},
{
linkSelector: '#ssg-catch-all-single-interpolated',
waitForSelector: '#all-ssg-content',
},
{
linkSelector: '#ssg-catch-all-multi',
waitForSelector: '#all-ssg-content',
},
{
linkSelector: '#ssg-catch-all-multi-no-as',
waitForSelector: '#all-ssg-content',
},
{
linkSelector: '#ssg-catch-all-multi',
waitForSelector: '#all-ssg-content',
},
{
linkSelector: '#nested-ssg-catch-all-single',
waitForSelector: '#nested-all-ssg-content',
},
{
linkSelector: '#nested-ssg-catch-all-multi',
waitForSelector: '#nested-all-ssg-content',
},
]

for (const { linkSelector, waitForSelector } of links) {
await browser.elementByCss(linkSelector).click()
await browser.waitForElementByCss(waitForSelector)
await browser.back()
await browser.waitForElementByCss(linkSelector)
}
const newCacheKeys = await getCacheKeys()
expect(newCacheKeys).toEqual(cacheKeys)
})
}

if (dev) {
it('should not have error after pinging WebSocket', async () => {
const browser = await webdriver(appPort, '/')
Expand Down

0 comments on commit d959d28

Please sign in to comment.