Relax encoding on dynamic prerendered routes (#14717)

It should be enough to encode the characters that `path-to-regexp` uses as path delimiters (`/#?`).

Fixes #14691

packages/next/build/utils.ts

@@ -20,6 +20,7 @@ import prettyBytes from '../lib/pretty-bytes'
 import { recursiveReadDir } from '../lib/recursive-readdir'
 import { getRouteMatcher, getRouteRegex } from '../next-server/lib/router/utils'
 import { isDynamicRoute } from '../next-server/lib/router/utils/is-dynamic'
+import escapePathDelimiters from '../next-server/lib/router/utils/escape-path-delimiters'
 import { findPageFile } from '../server/lib/find-page-file'
 import { GetStaticPaths } from 'next/types'
 import { denormalizePagePath } from '../next-server/server/normalize-page-path'
@@ -631,8 +632,8 @@ export async function buildStaticPaths(
           .replace(
             replaced,
             repeat
-              ? (paramValue as string[]).map(encodeURIComponent).join('/')
-              : encodeURIComponent(paramValue as string)
+              ? (paramValue as string[]).map(escapePathDelimiters).join('/')
+              : escapePathDelimiters(paramValue as string)
           )
           .replace(/(?!^)\/$/, '')
       })

packages/next/client/page-loader.js

@@ -3,6 +3,7 @@ import mitt from '../next-server/lib/mitt'
 import { isDynamicRoute } from './../next-server/lib/router/utils/is-dynamic'
 import { getRouteMatcher } from './../next-server/lib/router/utils/route-matcher'
 import { getRouteRegex } from './../next-server/lib/router/utils/route-regex'
+import escapePathDelimiters from '../next-server/lib/router/utils/escape-path-delimiters'
 import getAssetPathFromRoute from './../next-server/lib/router/utils/get-asset-path-from-route'
 
 function hasRel(rel, link) {
@@ -153,8 +154,8 @@ export default class PageLoader {
             (interpolatedRoute = interpolatedRoute.replace(
               replaced,
               repeat
-                ? value.map(encodeURIComponent).join('/')
-                : encodeURIComponent(value)
+                ? value.map(escapePathDelimiters).join('/')
+                : escapePathDelimiters(value)
             ))
           )
         })

packages/next/next-server/lib/router/utils/escape-path-delimiters.ts

@@ -0,0 +1,4 @@
+// escape delimiters used by path-to-regexp
+export default function escapePathDelimiters(segment: string): string {
+  return segment.replace(/[/#?]/g, (char: string) => encodeURIComponent(char))
+}

test/integration/prerender/pages/catchall-explicit/[...slug].js

@@ -1,3 +1,5 @@
+import Link from 'next/link'
+
 export async function getStaticProps({ params: { slug } }) {
   if (slug[0] === 'delayby3s') {
     await new Promise((resolve) => setTimeout(resolve, 3000))
@@ -18,13 +20,22 @@ export async function getStaticPaths() {
       '/catchall-explicit/second',
       { params: { slug: ['another', 'value'] } },
       '/catchall-explicit/hello/another',
+      '/catchall-explicit/[first]/[second]',
+      { params: { slug: ['[third]', '[fourth]'] } },
     ],
     fallback: false,
   }
 }
 
-export default ({ slug }) => {
+export default function Page({ slug }) {
   // Important to not check for `slug` existence (testing that build does not
   // render fallback version and error)
-  return <p id="catchall">Hi {slug.join(' ')}</p>
+  return (
+    <>
+      <p id="catchall">Hi {slug.join(' ')}</p>{' '}
+      <Link href="/">
+        <a id="home">to home</a>
+      </Link>
+    </>
+  )
 }

test/integration/prerender/pages/dynamic/[slug].js

@@ -0,0 +1,27 @@
+import Link from 'next/link'
+
+export async function getStaticProps({ params: { slug } }) {
+  return {
+    props: { slug },
+  }
+}
+
+export async function getStaticPaths() {
+  return {
+    paths: [{ params: { slug: '[first]' } }, '/dynamic/[second]'],
+    fallback: false,
+  }
+}
+
+export default ({ slug }) => {
+  // Important to not check for `slug` existence (testing that build does not
+  // render fallback version and error)
+  return (
+    <>
+      <p id="param">Hi {slug}!</p>{' '}
+      <Link href="/">
+        <a id="home">to home</a>
+      </Link>
+    </>
+  )
+}

test/integration/prerender/pages/index.js

@@ -62,6 +62,30 @@ const Page = ({ world, time }) => {
       <Link href="/catchall-optional/[[...slug]]" as="/catchall-optional/value">
         <a id="catchall-optional-value">to optional catchall page /value</a>
       </Link>
+      <br />
+      <Link href="/dynamic/[slug]" as="/dynamic/[first]">
+        <a id="dynamic-first">to dynamic [first] page</a>
+      </Link>
+      <Link href="/dynamic/[slug]" as="/dynamic/[second]">
+        <a id="dynamic-second">to dynamic [second] page</a>
+      </Link>
+      <br />
+      <Link
+        href="/catchall-explicit/[...slug]"
+        as="/catchall-explicit/[first]/[second]"
+      >
+        <a id="catchall-explicit-string">
+          to catchall-explicit [first]/[second] page
+        </a>
+      </Link>
+      <Link
+        href="/catchall-explicit/[...slug]"
+        as="/catchall-explicit/[third]/[fourth]"
+      >
+        <a id="catchall-explicit-object">
+          to catchall-explicit [third]/[fourth] page
+        </a>
+      </Link>
     </>
   )
 }

test/integration/prerender/test/index.test.js

@@ -117,6 +117,16 @@ const expectedManifestRoutes = () => ({
     initialRevalidateSeconds: 1,
     srcRoute: '/catchall-explicit/[...slug]',
   },
+  '/catchall-explicit/[first]/[second]': {
+    dataRoute: `/_next/data/${buildId}/catchall-explicit/[first]/[second].json`,
+    initialRevalidateSeconds: 1,
+    srcRoute: '/catchall-explicit/[...slug]',
+  },
+  '/catchall-explicit/[third]/[fourth]': {
+    dataRoute: `/_next/data/${buildId}/catchall-explicit/[third]/[fourth].json`,
+    initialRevalidateSeconds: 1,
+    srcRoute: '/catchall-explicit/[...slug]',
+  },
   '/catchall-optional': {
     dataRoute: `/_next/data/${buildId}/catchall-optional.json`,
     initialRevalidateSeconds: false,
@@ -142,6 +152,16 @@ const expectedManifestRoutes = () => ({
     initialRevalidateSeconds: false,
     srcRoute: null,
   },
+  '/dynamic/[first]': {
+    dataRoute: `/_next/data/${buildId}/dynamic/[first].json`,
+    initialRevalidateSeconds: false,
+    srcRoute: '/dynamic/[slug]',
+  },
+  '/dynamic/[second]': {
+    dataRoute: `/_next/data/${buildId}/dynamic/[second].json`,
+    initialRevalidateSeconds: false,
+    srcRoute: '/dynamic/[slug]',
+  },
   '/index': {
     dataRoute: `/_next/data/${buildId}/index/index.json`,
     initialRevalidateSeconds: false,
@@ -307,6 +327,50 @@ const navigateTest = (dev = false) => {
     await browser.elementByCss('#home').click()
     await browser.waitForElementByCss('#comment-1')
 
+    // go to /dynamic/[first]
+    await browser.elementByCss('#dynamic-first').click()
+    await browser.waitForElementByCss('#home')
+    text = await browser.elementByCss('#param').text()
+    expect(text).toMatch(/Hi \[first\]!/)
+    expect(await browser.eval('window.didTransition')).toBe(1)
+
+    // go to /
+    await browser.elementByCss('#home').click()
+    await browser.waitForElementByCss('#comment-1')
+
+    // go to /dynamic/[second]
+    await browser.elementByCss('#dynamic-second').click()
+    await browser.waitForElementByCss('#home')
+    text = await browser.elementByCss('#param').text()
+    expect(text).toMatch(/Hi \[second\]!/)
+    expect(await browser.eval('window.didTransition')).toBe(1)
+
+    // go to /
+    await browser.elementByCss('#home').click()
+    await browser.waitForElementByCss('#comment-1')
+
+    // go to /catchall-explicit/[first]/[second]
+    await browser.elementByCss('#catchall-explicit-string').click()
+    await browser.waitForElementByCss('#home')
+    text = await browser.elementByCss('#catchall').text()
+    expect(text).toMatch(/Hi \[first\] \[second\]/)
+    expect(await browser.eval('window.didTransition')).toBe(1)
+
+    // go to /
+    await browser.elementByCss('#home').click()
+    await browser.waitForElementByCss('#comment-1')
+
+    // go to /catchall-explicit/[first]/[second]
+    await browser.elementByCss('#catchall-explicit-object').click()
+    await browser.waitForElementByCss('#home')
+    text = await browser.elementByCss('#catchall').text()
+    expect(text).toMatch(/Hi \[third\] \[fourth\]/)
+    expect(await browser.eval('window.didTransition')).toBe(1)
+
+    // go to /
+    await browser.elementByCss('#home').click()
+    await browser.waitForElementByCss('#comment-1')
+
     // go to /catchall-optional/value
     await browser.elementByCss('#catchall-optional-value').click()
     await browser.waitForElementByCss('#home')
@@ -460,6 +524,68 @@ const runTests = (dev = false, isEmulatedServerless = false) => {
     ).toBe(true)
   })
 
+  it('should SSR dynamic page with brackets in param as object', async () => {
+    const html = await renderViaHTTP(appPort, '/dynamic/[first]')
+    const $ = cheerio.load(html)
+    expect($('#param').text()).toMatch(/Hi \[first\]!/)
+  })
+
+  it('should navigate to dynamic page with brackets in param as object', async () => {
+    const browser = await webdriver(appPort, '/')
+    await browser.elementByCss('#dynamic-first').click()
+    await browser.waitForElementByCss('#param')
+    const value = await browser.elementByCss('#param').text()
+    expect(value).toMatch(/Hi \[first\]!/)
+  })
+
+  it('should SSR dynamic page with brackets in param as string', async () => {
+    const html = await renderViaHTTP(appPort, '/dynamic/[second]')
+    const $ = cheerio.load(html)
+    expect($('#param').text()).toMatch(/Hi \[second\]!/)
+  })
+
+  it('should navigate to dynamic page with brackets in param as string', async () => {
+    const browser = await webdriver(appPort, '/')
+    await browser.elementByCss('#dynamic-second').click()
+    await browser.waitForElementByCss('#param')
+    const value = await browser.elementByCss('#param').text()
+    expect(value).toMatch(/Hi \[second\]!/)
+  })
+
+  it('should SSR catch-all page with brackets in param as string', async () => {
+    const html = await renderViaHTTP(
+      appPort,
+      '/catchall-explicit/[first]/[second]'
+    )
+    const $ = cheerio.load(html)
+    expect($('#catchall').text()).toMatch(/Hi \[first\] \[second\]/)
+  })
+
+  it('should navigate to catch-all page with brackets in param as string', async () => {
+    const browser = await webdriver(appPort, '/')
+    await browser.elementByCss('#catchall-explicit-string').click()
+    await browser.waitForElementByCss('#catchall')
+    const value = await browser.elementByCss('#catchall').text()
+    expect(value).toMatch(/Hi \[first\] \[second\]/)
+  })
+
+  it('should SSR catch-all page with brackets in param as object', async () => {
+    const html = await renderViaHTTP(
+      appPort,
+      '/catchall-explicit/[third]/[fourth]'
+    )
+    const $ = cheerio.load(html)
+    expect($('#catchall').text()).toMatch(/Hi \[third\] \[fourth\]/)
+  })
+
+  it('should navigate to catch-all page with brackets in param as object', async () => {
+    const browser = await webdriver(appPort, '/')
+    await browser.elementByCss('#catchall-explicit-object').click()
+    await browser.waitForElementByCss('#catchall')
+    const value = await browser.elementByCss('#catchall').text()
+    expect(value).toMatch(/Hi \[third\] \[fourth\]/)
+  })
+
   // TODO: dev currently renders this page as blocking, meaning it shows the
   // server error instead of continously retrying. Do we want to change this?
   if (!dev) {
@@ -1016,6 +1142,20 @@ const runTests = (dev = false, isEmulatedServerless = false) => {
           ),
           page: '/default-revalidate',
         },
+        {
+          dataRouteRegex: normalizeRegEx(
+            `^\\/_next\\/data\\/${escapeRegex(
+              buildId
+            )}\\/dynamic\\/([^\\/]+?)\\.json$`
+          ),
+          namedDataRouteRegex: `^/_next/data/${escapeRegex(
+            buildId
+          )}/dynamic/(?<slug>[^/]+?)\\.json$`,
+          page: '/dynamic/[slug]',
+          routeKeys: {
+            slug: 'slug',
+          },
+        },
         {
           dataRouteRegex: normalizeRegEx(
             `^\\/_next\\/data\\/${escapeRegex(
@@ -1125,6 +1265,14 @@ const runTests = (dev = false, isEmulatedServerless = false) => {
             '^\\/blog\\/([^\\/]+?)\\/([^\\/]+?)(?:\\/)?$'
           ),
         },
+        '/dynamic/[slug]': {
+          dataRoute: `/_next/data/${buildId}/dynamic/[slug].json`,
+          dataRouteRegex: normalizeRegEx(
+            `^\\/_next\\/data\\/${escapedBuildId}\\/dynamic\\/([^\\/]+?)\\.json$`
+          ),
+          fallback: false,
+          routeRegex: normalizeRegEx(`^\\/dynamic\\/([^\\/]+?)(?:\\/)?$`),
+        },
         '/fallback-only/[slug]': {
           dataRoute: `/_next/data/${buildId}/fallback-only/[slug].json`,
           dataRouteRegex: normalizeRegEx(