Fix prerendered nested index handling (vercel#14383)

Noticed this while reviewing vercel#14376. After having done vercel#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 vercel#14376, the issue was already there, I just noticed it while reading that PR
rokinsky · Jul 11, 2020 · b900a4a · b900a4a
1 parent f9a5736
commit b900a4a
Show file tree

Hide file tree

Showing 13 changed files with 164 additions and 31 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -1,4 +1,4 @@
-import { denormalizePagePath } from './normalize-page-path'
+import getRouteFromAssetPath from '../lib/router/utils/get-route-from-asset-path'
 
 // matches static/<buildid>/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
@@ -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
@@ -38,6 +38,12 @@ export default () => (
         <h1>index getStaticProps</h1>
       </a>
     </Link>
+    <br />
+    <Link href="/index">
+      <a id="nested-index-gsp">
+        <h1>nested index getStaticProps</h1>
+      </a>
+    </Link>
     <div id="base-path">{useRouter().basePath}</div>
     <div id="pathname">{useRouter().pathname}</div>
   </>

diff --git 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 (
     <>
+      <p id="nested">{nested ? 'yes' : 'no'}</p>
       <p id="prop">{hello} world</p>
       <p id="query">{JSON.stringify(query)}</p>
       <p id="pathname">{pathname}</p>

diff --git a/test/integration/basepath/pages/index/index.js 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 (
+    <>
+      <p id="nested">{nested ? 'yes' : 'no'}</p>
+      <p id="prop">{hello} world</p>
+      <p id="query">{JSON.stringify(query)}</p>
+      <p id="pathname">{pathname}</p>
+    </>
+  )
+}
diff --git 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
@@ -48,6 +48,10 @@ const Page = ({ world, time }) => {
         <a id="to-catchall">to catchall</a>
       </Link>
       <br />
+      <Link href="/index">
+        <a id="to-nested-index">to nested index</a>
+      </Link>
+      <br />
       <Link href="/catchall-optional/[[...slug]]" as="/catchall-optional">
         <a id="catchall-optional-root">to optional catchall root</a>
       </Link>

diff --git a/test/integration/prerender/pages/index/index.js 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 (
+    <>
+      <p>hello {world}</p>
+      <Link href="/">
+        <a id="home">to home</a>
+      </Link>
+    </>
+  )
+}
diff --git 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