Skip to content

Commit

Permalink
Make sure to break rewrites chain when dynamic route matches (#16455)
Browse files Browse the repository at this point in the history
This makes sure to also check if a dynamic route matched after resolving a rewrite on the client to match behavior on the server. It also adds tests for this behavior to ensure it is working properly. 

Fixes: #16454
  • Loading branch information
ijjk authored Aug 22, 2020
1 parent 42e1d5b commit 8a1c993
Show file tree
Hide file tree
Showing 10 changed files with 154 additions and 2 deletions.
9 changes: 8 additions & 1 deletion packages/next/next-server/lib/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,14 @@ export default class Router implements BaseRouter {
let resolvedAs = as

if (process.env.__NEXT_HAS_REWRITES) {
resolvedAs = resolveRewrites(as, pages, basePath, rewrites, query)
resolvedAs = resolveRewrites(
as,
pages,
basePath,
rewrites,
query,
(p: string) => this._resolveHref({ pathname: p }, pages).pathname!
)
}
resolvedAs = delBasePath(resolvedAs)

Expand Down
10 changes: 9 additions & 1 deletion packages/next/next-server/lib/router/utils/resolve-rewrites.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ export default function resolveRewrites(
pages: string[],
basePath: string,
rewrites: Rewrite[],
query: ParsedUrlQuery
query: ParsedUrlQuery,
resolveHref: (path: string) => string
) {
if (!pages.includes(asPath)) {
for (const rewrite of rewrites) {
Expand All @@ -37,6 +38,13 @@ export default function resolveRewrites(
// resolving the rewrites
break
}

// check if we match a dynamic-route, if so we break the rewrites chain
const resolvedHref = resolveHref(asPath)

if (resolvedHref !== asPath && pages.includes(resolvedHref)) {
break
}
}
}
}
Expand Down
14 changes: 14 additions & 0 deletions test/integration/rewrites-client-resolving/next.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
module.exports = {
rewrites() {
return [
{
source: '/:path*',
destination: '/:path*',
},
{
source: '/:path*(/?(?!.html))',
destination: '/category/:path*',
},
]
},
}
1 change: 1 addition & 0 deletions test/integration/rewrites-client-resolving/pages/404.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default () => '404 page'
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { useRouter } from 'next/router'

export default () => (
<p id="category">category: {useRouter().query.slug?.join('/')}</p>
)
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default () => <p id="categories">categories</p>
26 changes: 26 additions & 0 deletions test/integration/rewrites-client-resolving/pages/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import Link from 'next/link'

export default () => (
<>
<Link href="/product">
<a id="products-link">to /product</a>
</Link>
<br />
<Link href="/product/first">
<a id="product-link">to /product/first</a>
</Link>
<br />
<Link href="/category">
<a id="categories-link">to /category</a>
</Link>
<br />
<Link href="/category/first">
<a id="category-link">to /category/first</a>
</Link>
<br />
<Link href="/category/hello/world">
<a id="category-link-again">to /category/hello/world</a>
</Link>
<br />
</>
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { useRouter } from 'next/router'

export default () => <p id="product">product: {useRouter().query.productId}</p>
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default () => <p id="products">products</p>
86 changes: 86 additions & 0 deletions test/integration/rewrites-client-resolving/test/index.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/* eslint-env jest */

import { join } from 'path'
import webdriver from 'next-webdriver'
import {
killApp,
findPort,
nextBuild,
nextStart,
launchApp,
} from 'next-test-utils'

jest.setTimeout(1000 * 60 * 2)

const appDir = join(__dirname, '../')
let appPort
let app

const runTests = () => {
it('should break rewrites chain when dynamic route matches', async () => {
const browser = await webdriver(appPort, '/')
await browser.elementByCss('#product-link').click()
await browser.waitForElementByCss('#product')

expect(await browser.elementByCss('#product').text()).toBe('product: first')
})

it('should break rewrites chain when normal page matches', async () => {
const browser = await webdriver(appPort, '/')
await browser.elementByCss('#products-link').click()
await browser.waitForElementByCss('#products')

expect(await browser.elementByCss('#products').text()).toBe('products')
})

it('should break rewrites chain when dynamic catch-all route matches', async () => {
const browser = await webdriver(appPort, '/')
await browser.elementByCss('#category-link').click()
await browser.waitForElementByCss('#category')

expect(await browser.elementByCss('#category').text()).toBe(
'category: first'
)
})

it('should break rewrites chain when dynamic catch-all route multi-level matches', async () => {
const browser = await webdriver(appPort, '/')
await browser.elementByCss('#category-link-again').click()
await browser.waitForElementByCss('#category')

expect(await browser.elementByCss('#category').text()).toBe(
'category: hello/world'
)
})

it('should break rewrites chain after matching /category', async () => {
const browser = await webdriver(appPort, '/')
await browser.elementByCss('#categories-link').click()
await browser.waitForElementByCss('#categories')

expect(await browser.elementByCss('#categories').text()).toBe('categories')
})
}

describe('Client-side rewrites resolving', () => {
describe('dev mode', () => {
beforeAll(async () => {
appPort = await findPort()
app = await launchApp(appDir, appPort)
})
afterAll(() => killApp(app))

runTests()
})

describe('production mode', () => {
beforeAll(async () => {
appPort = await findPort()
await nextBuild(appDir)
app = await nextStart(appDir, appPort)
})
afterAll(() => killApp(app))

runTests()
})
})

0 comments on commit 8a1c993

Please sign in to comment.