From bc2cb2210ff7b407a9312a7ce91715fecaf8cdfc Mon Sep 17 00:00:00 2001 From: Jan Potoms <2109932+Janpot@users.noreply.github.com> Date: Tue, 23 Jun 2020 07:49:48 +0200 Subject: [PATCH] Fix prerendered nested index handling (#14383) Noticed this while reviewing https://github.com/vercel/next.js/pull/14376. After having done https://github.com/vercel/next.js/pull/13699, this code didn't feel right to me: ```js function prepareRoute(path: string) { path = delBasePath(path || '') // this /index rewrite is problematic, it makes pages/index.js // and pages/index/index.js point to the same thing: return toRoute(!path || path === '/' ? '/index' : path) } ``` Added a nested index page to the prerender tests and found it was rendering the `/` route on navigation. This uncovered 2 more places around the dataroute where the index path was not translated correctly. **edit:** Just to note that there was nothing wrong with https://github.com/vercel/next.js/pull/14376, the issue was already there, I just noticed it while reading that PR --- packages/next/client/page-loader.js | 21 ++++------ .../next/next-server/lib/router/router.ts | 9 +++-- .../router/utils/get-asset-path-from-route.ts | 14 +++++++ .../router/utils/get-route-from-asset-path.ts | 16 ++++++++ .../server/get-route-from-entrypoint.ts | 4 +- .../next/next-server/server/next-server.ts | 16 ++++---- test/integration/basepath/pages/hello.js | 6 +++ test/integration/basepath/pages/index.js | 4 +- .../integration/basepath/pages/index/index.js | 22 ++++++++++ test/integration/basepath/test/index.test.js | 40 +++++++++++++++++-- test/integration/prerender/pages/index.js | 4 ++ .../prerender/pages/index/index.js | 18 +++++++++ test/integration/prerender/test/index.test.js | 21 ++++++++++ 13 files changed, 164 insertions(+), 31 deletions(-) create mode 100644 packages/next/next-server/lib/router/utils/get-asset-path-from-route.ts create mode 100644 packages/next/next-server/lib/router/utils/get-route-from-asset-path.ts create mode 100644 test/integration/basepath/pages/index/index.js create mode 100644 test/integration/prerender/pages/index/index.js diff --git a/packages/next/client/page-loader.js b/packages/next/client/page-loader.js index 3c483b9726..c2960466e5 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 getAssetPathFromRoute from './../next-server/lib/router/utils/get-asset-path-from-route' function hasRel(rel, link) { try { @@ -38,14 +39,6 @@ function normalizeRoute(route) { return route.replace(/\/$/, '') } -export function getAssetPath(route) { - return route === '/' - ? '/index' - : /^\/index(\/|$)/.test(route) - ? `/index${route}` - : `${route}` -} - function appendLink(href, rel, as) { return new Promise((res, rej, link) => { link = document.createElement('link') @@ -118,8 +111,8 @@ export default class PageLoader { */ getDataHref(href, asPath) { const getHrefForSlug = (/** @type string */ path) => { - const dataRoute = getAssetPath(path) - return `${this.assetPrefix}/_next/data/${this.buildId}${dataRoute}.json` + const dataRoute = getAssetPathFromRoute(path, '.json') + return `${this.assetPrefix}/_next/data/${this.buildId}${dataRoute}` } const { pathname: hrefPathname, query } = parse(href, true) @@ -250,11 +243,11 @@ export default class PageLoader { } else { // Development only. In production the page file is part of the build manifest route = normalizeRoute(route) - let scriptRoute = getAssetPath(route) + let scriptRoute = getAssetPathFromRoute(route, '.js') const url = `${this.assetPrefix}/_next/static/pages${encodeURI( scriptRoute - )}.js` + )}` this.loadScript(url, route) } } @@ -331,13 +324,13 @@ export default class PageLoader { if (process.env.NODE_ENV !== 'production') { route = normalizeRoute(route) - const scriptRoute = getAssetPath(route) const ext = process.env.__NEXT_MODERN_BUILD && hasNoModule ? '.module.js' : '.js' + const scriptRoute = getAssetPathFromRoute(route, ext) url = `${this.assetPrefix}/_next/static/${encodeURIComponent( this.buildId - )}/pages${encodeURI(scriptRoute)}${ext}` + )}/pages${encodeURI(scriptRoute)}` } } diff --git a/packages/next/next-server/lib/router/router.ts b/packages/next/next-server/lib/router/router.ts index c694ff2450..f41367d71b 100644 --- a/packages/next/next-server/lib/router/router.ts +++ b/packages/next/next-server/lib/router/router.ts @@ -15,6 +15,7 @@ import { import { isDynamicRoute } from './utils/is-dynamic' import { getRouteMatcher } from './utils/route-matcher' import { getRouteRegex } from './utils/route-regex' +import getAssetPathFromRoute from './utils/get-asset-path-from-route' const basePath = (process.env.__NEXT_ROUTER_BASEPATH as string) || '' @@ -31,8 +32,7 @@ function toRoute(path: string): string { } function prepareRoute(path: string) { - path = delBasePath(path || '') - return toRoute(!path || path === '/' ? '/index' : path) + return toRoute(delBasePath(path || '') || '/') } type Url = UrlObject | string @@ -108,7 +108,10 @@ function fetchNextData( formatWithValidation({ pathname: addBasePath( // @ts-ignore __NEXT_DATA__ - `/_next/data/${__NEXT_DATA__.buildId}${pathname}.json` + `/_next/data/${__NEXT_DATA__.buildId}${getAssetPathFromRoute( + pathname, + '.json' + )}` ), query, }), diff --git a/packages/next/next-server/lib/router/utils/get-asset-path-from-route.ts b/packages/next/next-server/lib/router/utils/get-asset-path-from-route.ts new file mode 100644 index 0000000000..b1da430dd9 --- /dev/null +++ b/packages/next/next-server/lib/router/utils/get-asset-path-from-route.ts @@ -0,0 +1,14 @@ +// Translates a logical route into its pages asset path (relative from a common prefix) +// "asset path" being its javascript file, data file, prerendered html,... +export default function getAssetPathFromRoute( + route: string, + ext: string = '' +): string { + const path = + route === '/' + ? '/index' + : /^\/index(\/|$)/.test(route) + ? `/index${route}` + : `${route}` + return path + ext +} diff --git a/packages/next/next-server/lib/router/utils/get-route-from-asset-path.ts b/packages/next/next-server/lib/router/utils/get-route-from-asset-path.ts new file mode 100644 index 0000000000..495865e568 --- /dev/null +++ b/packages/next/next-server/lib/router/utils/get-route-from-asset-path.ts @@ -0,0 +1,16 @@ +// Translate a pages asset path (relative from a common prefix) back into its logical route +// "asset path" being its javascript file, data file, prerendered html,... +export default function getRouteFromAssetPath( + assetPath: string, + ext: string = '' +): string { + assetPath = assetPath.replace(/\\/g, '/') + assetPath = + ext && assetPath.endsWith(ext) ? assetPath.slice(0, -ext.length) : assetPath + if (assetPath.startsWith('/index/')) { + assetPath = assetPath.slice(6) + } else if (assetPath === '/index') { + assetPath = '/' + } + return assetPath +} diff --git a/packages/next/next-server/server/get-route-from-entrypoint.ts b/packages/next/next-server/server/get-route-from-entrypoint.ts index 4d9756da20..98efd1b6c6 100644 --- a/packages/next/next-server/server/get-route-from-entrypoint.ts +++ b/packages/next/next-server/server/get-route-from-entrypoint.ts @@ -1,4 +1,4 @@ -import { denormalizePagePath } from './normalize-page-path' +import getRouteFromAssetPath from '../lib/router/utils/get-route-from-asset-path' // matches static//pages/:page*.js // const SERVER_ROUTE_NAME_REGEX = /^static[/\\][^/\\]+[/\\]pages[/\\](.*)$/ @@ -14,7 +14,7 @@ function matchBundle(regex: RegExp, input: string): string | null { return null } - return denormalizePagePath(`/${result[1]}`) + return getRouteFromAssetPath(`/${result[1]}`) } export default function getRouteFromEntrypoint( diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index 9158099f8b..08a25c818e 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -67,6 +67,7 @@ import { compile as compilePathToRegex } from 'next/dist/compiled/path-to-regexp import { loadEnvConfig } from '../../lib/load-env-config' import './node-polyfill-fetch' import { PagesManifest } from '../../build/webpack/plugins/pages-manifest-plugin' +import getRouteFromAssetPath from '../lib/router/utils/get-route-from-asset-path' const getCustomRouteMatcher = pathMatch(true) @@ -411,13 +412,14 @@ export default class Server { } // re-create page's pathname - const pathname = `/${params.path - // we need to re-encode the params since they are decoded - // by path-match and we are re-building the URL - .map((param: string) => encodeURIComponent(param)) - .join('/')}` - .replace(/\.json$/, '') - .replace(/\/index$/, '/') + const pathname = getRouteFromAssetPath( + `/${params.path + // we need to re-encode the params since they are decoded + // by path-match and we are re-building the URL + .map((param: string) => encodeURIComponent(param)) + .join('/')}`, + '.json' + ) const parsedUrl = parseUrl(pathname, true) diff --git a/test/integration/basepath/pages/hello.js b/test/integration/basepath/pages/hello.js index 2df0ba7850..aad0239dc0 100644 --- a/test/integration/basepath/pages/hello.js +++ b/test/integration/basepath/pages/hello.js @@ -38,6 +38,12 @@ export default () => (

index getStaticProps

+
+ + +

nested index getStaticProps

+ +
{useRouter().basePath}
{useRouter().pathname}
diff --git a/test/integration/basepath/pages/index.js b/test/integration/basepath/pages/index.js index c479bede45..59d45cd776 100644 --- a/test/integration/basepath/pages/index.js +++ b/test/integration/basepath/pages/index.js @@ -3,15 +3,17 @@ import { useRouter } from 'next/router' export const getStaticProps = () => { return { props: { + nested: false, hello: 'hello', }, } } -export default function Index({ hello }) { +export default function Index({ hello, nested }) { const { query, pathname } = useRouter() return ( <> +

{nested ? 'yes' : 'no'}

{hello} world

{JSON.stringify(query)}

{pathname}

diff --git a/test/integration/basepath/pages/index/index.js b/test/integration/basepath/pages/index/index.js new file mode 100644 index 0000000000..c9b946b00e --- /dev/null +++ b/test/integration/basepath/pages/index/index.js @@ -0,0 +1,22 @@ +import { useRouter } from 'next/router' + +export const getStaticProps = () => { + return { + props: { + nested: true, + hello: 'hello', + }, + } +} + +export default function Index({ hello, nested }) { + const { query, pathname } = useRouter() + return ( + <> +

{nested ? 'yes' : 'no'}

+

{hello} world

+

{JSON.stringify(query)}

+

{pathname}

+ + ) +} diff --git a/test/integration/basepath/test/index.test.js b/test/integration/basepath/test/index.test.js index b3b7f6874d..157e0df700 100644 --- a/test/integration/basepath/test/index.test.js +++ b/test/integration/basepath/test/index.test.js @@ -101,11 +101,9 @@ const runTests = (context, dev = false) => { await browser.waitForElementByCss('#prop') expect(await browser.eval('window.beforeNavigate')).toBe('hi') - expect(await browser.elementByCss('#prop').text()).toBe('hello world') - + expect(await browser.elementByCss('#nested').text()).toBe('no') expect(JSON.parse(await browser.elementByCss('#query').text())).toEqual({}) - expect(await browser.elementByCss('#pathname').text()).toBe('/') if (!dev) { @@ -118,7 +116,41 @@ const runTests = (context, dev = false) => { if ( href.startsWith('/docs/_next/data') && - href.endsWith('index.json') + href.endsWith('index.json') && + !href.endsWith('index/index.json') + ) { + found = true + } + } + + expect(found).toBe(true) + } + }) + + it('should navigate to nested index page with getStaticProps', async () => { + const browser = await webdriver(context.appPort, '/docs/hello') + await browser.eval('window.beforeNavigate = "hi"') + + await browser.elementByCss('#nested-index-gsp').click() + await browser.waitForElementByCss('#prop') + + expect(await browser.eval('window.beforeNavigate')).toBe('hi') + expect(await browser.elementByCss('#prop').text()).toBe('hello world') + expect(await browser.elementByCss('#nested').text()).toBe('yes') + expect(JSON.parse(await browser.elementByCss('#query').text())).toEqual({}) + expect(await browser.elementByCss('#pathname').text()).toBe('/index') + + if (!dev) { + const prefetches = await browser.elementsByCss('link[rel="prefetch"]') + let found = false + + for (const prefetch of prefetches) { + const fullHref = await prefetch.getAttribute('href') + const href = url.parse(fullHref).pathname + + if ( + href.startsWith('/docs/_next/data') && + href.endsWith('index/index.json') ) { found = true } diff --git a/test/integration/prerender/pages/index.js b/test/integration/prerender/pages/index.js index e826c7c2da..240a501bef 100644 --- a/test/integration/prerender/pages/index.js +++ b/test/integration/prerender/pages/index.js @@ -48,6 +48,10 @@ const Page = ({ world, time }) => { to catchall
+ + to nested index + +
to optional catchall root diff --git a/test/integration/prerender/pages/index/index.js b/test/integration/prerender/pages/index/index.js new file mode 100644 index 0000000000..99eb242939 --- /dev/null +++ b/test/integration/prerender/pages/index/index.js @@ -0,0 +1,18 @@ +import Link from 'next/link' + +export async function getStaticProps() { + return { + props: { world: 'nested index' }, + } +} + +export default ({ world }) => { + return ( + <> +

hello {world}

+ + to home + + + ) +} diff --git a/test/integration/prerender/test/index.test.js b/test/integration/prerender/test/index.test.js index ebd2548509..e697e9fcab 100644 --- a/test/integration/prerender/test/index.test.js +++ b/test/integration/prerender/test/index.test.js @@ -141,6 +141,11 @@ const expectedManifestRoutes = () => ({ initialRevalidateSeconds: false, srcRoute: null, }, + '/index': { + dataRoute: `/_next/data/${buildId}/index/index.json`, + initialRevalidateSeconds: false, + srcRoute: null, + }, '/lang/de/about': { dataRoute: `/_next/data/${buildId}/lang/de/about.json`, initialRevalidateSeconds: false, @@ -280,6 +285,16 @@ const navigateTest = (dev = false) => { await browser.elementByCss('#home').click() await browser.waitForElementByCss('#comment-1') + // go to /index + await browser.elementByCss('#to-nested-index').click() + await browser.waitForElementByCss('#home') + text = await browser.elementByCss('p').text() + expect(text).toMatch(/hello nested index/) + + // go to / + await browser.elementByCss('#home').click() + await browser.waitForElementByCss('#comment-1') + // go to /catchall-optional await browser.elementByCss('#catchall-optional-root').click() await browser.waitForElementByCss('#home') @@ -975,6 +990,12 @@ const runTests = (dev = false, isEmulatedServerless = false) => { slug: 'slug', }, }, + { + dataRouteRegex: normalizeRegEx( + `^\\/_next\\/data\\/${escapeRegex(buildId)}\\/index\\/index.json$` + ), + page: '/index', + }, { namedDataRouteRegex: `^/_next/data/${escapeRegex( buildId -- GitLab