Skip to content

Commit

Permalink
Ensure query is updated correctly with shallow and middleware (#39086)
Browse files Browse the repository at this point in the history
This ensures we properly update the query when doing a shallow navigation with middleware present and expands our test coverage of this kind of navigation. This also adds middleware rewrite coverage for the case described in #38405  and ensures we properly resolve dynamic middleware rewrites which was causing #39049

Test deployment with fix can be seen [here](https://middleware-i18n-redirect-repro-8jd3x47j9-ijjk-testing.vercel.app/gsp/first) and [here](https://nextjs-middleware-rewrite-l6q20cwo3-ijjk-testing.vercel.app/)

## Bug

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

Fixes: #38495
Fixes: #39049
Fixes: #38405
  • Loading branch information
ijjk committed Jul 28, 2022
1 parent 600bdb1 commit 3a31b96
Show file tree
Hide file tree
Showing 3 changed files with 210 additions and 11 deletions.
35 changes: 24 additions & 11 deletions packages/next/shared/lib/router/router.ts
Expand Up @@ -1347,7 +1347,10 @@ export default class Router implements BaseRouter {
if ('route' in routeInfo && isMiddlewareMatch) {
pathname = routeInfo.route || route
route = pathname
query = Object.assign({}, routeInfo.query || {}, query)

if (!routeProps.shallow) {
query = Object.assign({}, routeInfo.query || {}, query)
}

if (routeMatch && pathname !== parsed.pathname) {
Object.keys(routeMatch).forEach((key) => {
Expand All @@ -1359,8 +1362,15 @@ export default class Router implements BaseRouter {

if (isDynamicRoute(pathname)) {
const prefixedAs =
routeInfo.resolvedAs ||
addBasePath(addLocale(as, nextState.locale), true)
!routeProps.shallow && routeInfo.resolvedAs
? routeInfo.resolvedAs
: addBasePath(
addLocale(
new URL(as, location.href).pathname,
nextState.locale
),
true
)

let rewriteAs = prefixedAs

Expand Down Expand Up @@ -1701,6 +1711,10 @@ export default class Router implements BaseRouter {
return existingInfo
}

if (hasMiddleware) {
existingInfo = undefined
}

let cachedRouteInfo =
existingInfo &&
!('initial' in existingInfo) &&
Expand Down Expand Up @@ -1759,13 +1773,6 @@ export default class Router implements BaseRouter {
this.components[requestedRoute] = { ...existingInfo, route }
return { ...existingInfo, route }
}

cachedRouteInfo =
existingInfo &&
!('initial' in existingInfo) &&
process.env.NODE_ENV !== 'development'
? existingInfo
: undefined
}

if (route === '/api' || route.startsWith('/api/')) {
Expand Down Expand Up @@ -2346,7 +2353,7 @@ function getMiddlewareData<T extends FetchDataOutput>(
parseData: true,
})

const fsPathname = removeTrailingSlash(pathnameInfo.pathname)
let fsPathname = removeTrailingSlash(pathnameInfo.pathname)
return Promise.all([
options.router.pageLoader.getPageList(),
getClientBuildManifest(),
Expand Down Expand Up @@ -2385,6 +2392,12 @@ function getMiddlewareData<T extends FetchDataOutput>(
as = parsedRewriteTarget.pathname
Object.assign(parsedRewriteTarget.query, result.parsedAs.query)
}
} else if (!pages.includes(fsPathname)) {
const resolvedPathname = resolveDynamicRoute(fsPathname, pages)

if (resolvedPathname !== fsPathname) {
fsPathname = resolvedPathname
}
}

const resolvedHref = !pages.includes(fsPathname)
Expand Down
5 changes: 5 additions & 0 deletions test/e2e/middleware-rewrites/app/middleware.js
Expand Up @@ -13,6 +13,11 @@ export async function middleware(request) {
return NextResponse.next()
}

if (url.pathname.includes('/fallback-true-blog/rewritten')) {
request.nextUrl.pathname = '/about'
return NextResponse.rewrite(request.nextUrl)
}

if (url.pathname.startsWith('/about') && url.searchParams.has('override')) {
const isExternal = url.searchParams.get('override') === 'external'
return NextResponse.rewrite(
Expand Down
181 changes: 181 additions & 0 deletions test/e2e/middleware-rewrites/test/index.test.ts
Expand Up @@ -6,6 +6,7 @@ import webdriver from 'next-webdriver'
import { NextInstance } from 'test/lib/next-modes/base'
import { check, fetchViaHTTP } from 'next-test-utils'
import { createNext, FileRef } from 'e2e-utils'
import escapeStringRegexp from 'escape-string-regexp'

describe('Middleware Rewrite', () => {
let next: NextInstance
Expand Down Expand Up @@ -311,6 +312,186 @@ describe('Middleware Rewrite', () => {
`/rewrite-to-afterfiles-rewrite`
)
})

it('should handle shallow navigation correctly (non-dynamic page)', async () => {
const browser = await webdriver(next.url, '/about')
const requests = []

browser.on('request', (req) => {
const url = req.url()
if (url.includes('_next/data')) requests.push(url)
})

await browser.eval(
`next.router.push('/about?hello=world', undefined, { shallow: true })`
)
await check(() => browser.eval(`next.router.query.hello`), 'world')

expect(await browser.eval(`next.router.pathname`)).toBe('/about')
expect(
JSON.parse(await browser.eval(`JSON.stringify(next.router.query)`))
).toEqual({ hello: 'world' })
expect(await browser.eval('location.pathname')).toBe('/about')
expect(await browser.eval('location.search')).toBe('?hello=world')
expect(requests).toEqual([])

await browser.eval(
`next.router.push('/about', undefined, { shallow: true })`
)
await check(
() => browser.eval(`next.router.query.hello || 'empty'`),
'empty'
)

expect(await browser.eval(`next.router.pathname`)).toBe('/about')
expect(
JSON.parse(await browser.eval(`JSON.stringify(next.router.query)`))
).toEqual({})
expect(await browser.eval('location.pathname')).toBe('/about')
expect(await browser.eval('location.search')).toBe('')
expect(requests).toEqual([])
})

it('should handle shallow navigation correctly (dynamic page)', async () => {
const browser = await webdriver(next.url, '/fallback-true-blog/first', {
waitHydration: false,
})
let requests = []

browser.on('request', (req) => {
const url = req.url()
if (url.includes('_next/data')) requests.push(url)
})

// wait for initial query update request
await check(() => {
if (requests.length > 0) {
requests = []
return 'yup'
}
}, 'yup')

await browser.eval(
`next.router.push('/fallback-true-blog/first?hello=world', undefined, { shallow: true })`
)
await check(() => browser.eval(`next.router.query.hello`), 'world')

expect(await browser.eval(`next.router.pathname`)).toBe(
'/fallback-true-blog/[slug]'
)
expect(
JSON.parse(await browser.eval(`JSON.stringify(next.router.query)`))
).toEqual({ hello: 'world', slug: 'first' })
expect(await browser.eval('location.pathname')).toBe(
'/fallback-true-blog/first'
)
expect(await browser.eval('location.search')).toBe('?hello=world')
expect(requests).toEqual([])

await browser.eval(
`next.router.push('/fallback-true-blog/second', undefined, { shallow: true })`
)
await check(() => browser.eval(`next.router.query.slug`), 'second')

expect(await browser.eval(`next.router.pathname`)).toBe(
'/fallback-true-blog/[slug]'
)
expect(
JSON.parse(await browser.eval(`JSON.stringify(next.router.query)`))
).toEqual({
slug: 'second',
})
expect(await browser.eval('location.pathname')).toBe(
'/fallback-true-blog/second'
)
expect(await browser.eval('location.search')).toBe('')
expect(requests).toEqual([])
})

it('should resolve dynamic route after rewrite correctly', async () => {
const browser = await webdriver(next.url, '/fallback-true-blog/first', {
waitHydration: false,
})
let requests = []

browser.on('request', (req) => {
const url = new URL(
req
.url()
.replace(new RegExp(escapeStringRegexp(next.buildId)), 'BUILD_ID')
).pathname

if (url.includes('_next/data')) requests.push(url)
})

// wait for initial query update request
await check(() => {
if (requests.length > 0) {
requests = []
return 'yup'
}
}, 'yup')

expect(await browser.eval(`next.router.pathname`)).toBe(
'/fallback-true-blog/[slug]'
)
expect(
JSON.parse(await browser.eval(`JSON.stringify(next.router.query)`))
).toEqual({
slug: 'first',
})
expect(await browser.eval('location.pathname')).toBe(
'/fallback-true-blog/first'
)
expect(await browser.eval('location.search')).toBe('')
expect(requests).toEqual([])

await browser.eval(`next.router.push('/fallback-true-blog/rewritten')`)
await check(
() => browser.eval('document.documentElement.innerHTML'),
/About Page/
)

expect(await browser.eval(`next.router.pathname`)).toBe('/about')
expect(
JSON.parse(await browser.eval(`JSON.stringify(next.router.query)`))
).toEqual({})
expect(await browser.eval('location.pathname')).toBe(
'/fallback-true-blog/rewritten'
)
expect(await browser.eval('location.search')).toBe('')
expect(requests).toEqual([
`/_next/data/BUILD_ID/en/fallback-true-blog/rewritten.json`,
])

await browser.eval(`next.router.push('/fallback-true-blog/second')`)
await check(
() => browser.eval(`next.router.pathname`),
'/fallback-true-blog/[slug]'
)

expect(await browser.eval(`next.router.pathname`)).toBe(
'/fallback-true-blog/[slug]'
)
expect(
JSON.parse(await browser.eval(`JSON.stringify(next.router.query)`))
).toEqual({
slug: 'second',
})
expect(await browser.eval('location.pathname')).toBe(
'/fallback-true-blog/second'
)
expect(await browser.eval('location.search')).toBe('')
expect(
requests.filter(
(req) =>
![
`/_next/data/BUILD_ID/en/fallback-true-blog/rewritten.json`,
`/_next/data/BUILD_ID/en/fallback-true-blog/second.json`,
].includes(req)
)
).toEqual([])
})
}

function testsWithLocale(locale = '') {
Expand Down

0 comments on commit 3a31b96

Please sign in to comment.