From c9492a8cc947f183ec608931239ef04a235776fd Mon Sep 17 00:00:00 2001 From: Jan Potoms <2109932+Janpot@users.noreply.github.com> Date: Mon, 13 Jul 2020 16:42:27 +0200 Subject: [PATCH] Relax encoding on dynamic prerendered routes (#14717) It should be enough to encode the characters that `path-to-regexp` uses as path delimiters (`/#?`). Fixes https://github.com/vercel/next.js/issues/14691 --- packages/next/build/utils.ts | 5 +- packages/next/client/page-loader.js | 5 +- .../router/utils/escape-path-delimiters.ts | 4 + .../pages/catchall-explicit/[...slug].js | 15 +- .../prerender/pages/dynamic/[slug].js | 27 ++++ test/integration/prerender/pages/index.js | 24 +++ test/integration/prerender/test/index.test.js | 148 ++++++++++++++++++ 7 files changed, 222 insertions(+), 6 deletions(-) create mode 100644 packages/next/next-server/lib/router/utils/escape-path-delimiters.ts create mode 100644 test/integration/prerender/pages/dynamic/[slug].js diff --git a/packages/next/build/utils.ts b/packages/next/build/utils.ts index e20e9d48ce0ee..5e4627f2b99e2 100644 --- a/packages/next/build/utils.ts +++ b/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(/(?!^)\/$/, '') }) diff --git a/packages/next/client/page-loader.js b/packages/next/client/page-loader.js index 9f54bd70ea8c9..e6c393b6dc199 100644 --- a/packages/next/client/page-loader.js +++ b/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) )) ) }) diff --git a/packages/next/next-server/lib/router/utils/escape-path-delimiters.ts b/packages/next/next-server/lib/router/utils/escape-path-delimiters.ts new file mode 100644 index 0000000000000..4bdb0fb796576 --- /dev/null +++ b/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)) +} diff --git a/test/integration/prerender/pages/catchall-explicit/[...slug].js b/test/integration/prerender/pages/catchall-explicit/[...slug].js index bcdb9c07434e0..2b8cb9d53ef87 100644 --- a/test/integration/prerender/pages/catchall-explicit/[...slug].js +++ b/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

Hi {slug.join(' ')}

+ return ( + <> +

Hi {slug.join(' ')}

{' '} + + to home + + + ) } diff --git a/test/integration/prerender/pages/dynamic/[slug].js b/test/integration/prerender/pages/dynamic/[slug].js new file mode 100644 index 0000000000000..67876322003a9 --- /dev/null +++ b/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 ( + <> +

Hi {slug}!

{' '} + + to home + + + ) +} diff --git a/test/integration/prerender/pages/index.js b/test/integration/prerender/pages/index.js index ac34b844f7016..02bf56107cadb 100644 --- a/test/integration/prerender/pages/index.js +++ b/test/integration/prerender/pages/index.js @@ -62,6 +62,30 @@ const Page = ({ world, time }) => { to optional catchall page /value +
+ + to dynamic [first] page + + + to dynamic [second] page + +
+ + + to catchall-explicit [first]/[second] page + + + + + to catchall-explicit [third]/[fourth] page + + ) } diff --git a/test/integration/prerender/test/index.test.js b/test/integration/prerender/test/index.test.js index c0579b4644468..0cb40fe3a13e1 100644 --- a/test/integration/prerender/test/index.test.js +++ b/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/(?[^/]+?)\\.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(