Skip to content

Commit

Permalink
Fix resolving href with a rewrite (#16975)
Browse files Browse the repository at this point in the history
This makes sure we properly resolve a rewrite when only the `href` value is used. This was causing a full-reload and was missed in the existing test since we weren't making sure a full navigation didn't occur which has been added in this PR. 

Fixes: #16974
  • Loading branch information
ijjk authored Sep 10, 2020
1 parent 72b78f3 commit dcdcf49
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 6 deletions.
27 changes: 23 additions & 4 deletions packages/next/next-server/lib/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ export default class Router implements BaseRouter {
method = 'replaceState'
}

const route = removePathTrailingSlash(pathname)
let route = removePathTrailingSlash(pathname)
const { shallow = false } = options

// we need to resolve the as value using rewrites for dynamic SSG
Expand All @@ -625,6 +625,25 @@ export default class Router implements BaseRouter {
query,
(p: string) => this._resolveHref({ pathname: p }, pages).pathname!
)

if (resolvedAs !== as) {
const potentialHref = removePathTrailingSlash(
this._resolveHref(
Object.assign({}, parsed, { pathname: resolvedAs }),
pages,
false
).pathname!
)

// if this directly matches a page we need to update the href to
// allow the correct page chunk to be loaded
if (pages.includes(potentialHref)) {
route = potentialHref
pathname = potentialHref
parsed.pathname = pathname
url = formatWithValidation(parsed)
}
}
}
resolvedAs = delBasePath(resolvedAs)

Expand Down Expand Up @@ -980,10 +999,10 @@ export default class Router implements BaseRouter {
return this.asPath !== asPath
}

_resolveHref(parsedHref: UrlObject, pages: string[]) {
_resolveHref(parsedHref: UrlObject, pages: string[], applyBasePath = true) {
const { pathname } = parsedHref
const cleanPathname = removePathTrailingSlash(
denormalizePagePath(delBasePath(pathname!))
denormalizePagePath(applyBasePath ? delBasePath(pathname!) : pathname!)
)

if (cleanPathname === '/404' || cleanPathname === '/_error') {
Expand All @@ -998,7 +1017,7 @@ export default class Router implements BaseRouter {
isDynamicRoute(page) &&
getRouteRegex(page).re.test(cleanPathname!)
) {
parsedHref.pathname = addBasePath(page)
parsedHref.pathname = applyBasePath ? addBasePath(page) : page
return true
}
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { ParsedUrlQuery } from 'querystring'
import pathMatch from './path-match'
import prepareDestination from './prepare-destination'
import { Rewrite } from '../../../../lib/load-custom-routes'
import { removePathTrailingSlash } from '../../../../client/normalize-trailing-slash'

const customRouteMatcher = pathMatch(true)

Expand Down Expand Up @@ -33,7 +34,7 @@ export default function resolveRewrites(
asPath = destRes.parsedDestination.pathname!
Object.assign(query, destRes.parsedDestination.query)

if (pages.includes(asPath)) {
if (pages.includes(removePathTrailingSlash(asPath))) {
// check if we now match a page as this means we are done
// resolving the rewrites
break
Expand Down
2 changes: 1 addition & 1 deletion test/integration/custom-routes/pages/auto-export/[slug].js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { useRouter } from 'next/router'

export default function Page() {
return `auto-export ${useRouter().query.slug}`
return <p id="auto-export">auto-export {useRouter().query.slug}</p>
}
4 changes: 4 additions & 0 deletions test/integration/custom-routes/pages/nav.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,9 @@ export default () => (
<a id="to-params">to params</a>
</Link>
<br />
<Link href="/rewriting-to-auto-export">
<a id="to-rewritten-dynamic">to rewritten dynamic</a>
</Link>
<br />
</>
)
18 changes: 18 additions & 0 deletions test/integration/custom-routes/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -316,11 +316,13 @@ const runTests = (isDev = false) => {

it('should work with rewrite when manually specifying href/as', async () => {
const browser = await webdriver(appPort, '/nav')
await browser.eval('window.beforeNav = 1')
await browser
.elementByCss('#to-params-manual')
.click()
.waitForElementByCss('#query')

expect(await browser.eval('window.beforeNav')).toBe(1)
const query = JSON.parse(await browser.elementByCss('#query').text())
expect(query).toEqual({
something: '1',
Expand All @@ -330,18 +332,34 @@ const runTests = (isDev = false) => {

it('should work with rewrite when only specifying href', async () => {
const browser = await webdriver(appPort, '/nav')
await browser.eval('window.beforeNav = 1')
await browser
.elementByCss('#to-params')
.click()
.waitForElementByCss('#query')

expect(await browser.eval('window.beforeNav')).toBe(1)
const query = JSON.parse(await browser.elementByCss('#query').text())
expect(query).toEqual({
something: '1',
another: 'value',
})
})

it('should work with rewrite when only specifying href and ends in dynamic route', async () => {
const browser = await webdriver(appPort, '/nav')
await browser.eval('window.beforeNav = 1')
await browser
.elementByCss('#to-rewritten-dynamic')
.click()
.waitForElementByCss('#auto-export')

expect(await browser.eval('window.beforeNav')).toBe(1)

const text = await browser.eval(() => document.documentElement.innerHTML)
expect(text).toContain('auto-export hello')
})

it('should match a page after a rewrite', async () => {
const html = await renderViaHTTP(appPort, '/to-hello')
expect(html).toContain('Hello')
Expand Down

0 comments on commit dcdcf49

Please sign in to comment.