Update handling for basePath to only automatically add (vercel#13817)

As discussed, this streamlines the handling for `basePath` to not automatically strip and add the `basePath` when provided to `next/link` or `router.push/replace` and only automatically adds the `basePath` and when it is manually provided it will cause a 404 which ensures `href` still matches to the pages directory 1-to-1.

This also adds additional test cases that we discussed to ensure this behavior is working as intended

---

Fixes vercel#13902

packages/next/client/link.tsx

@@ -162,8 +162,8 @@ class Link extends Component<LinkProps> {
   // as per https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html
   formatUrls = memoizedFormatUrl((href, asHref) => {
     return {
-      href: addBasePath(formatUrl(href)),
-      as: asHref ? addBasePath(formatUrl(asHref)) : asHref,
+      href: formatUrl(href),
+      as: asHref ? formatUrl(asHref) : asHref,
     }
   })
 
@@ -237,7 +237,10 @@ class Link extends Component<LinkProps> {
 
   render() {
     let { children } = this.props
-    const { href, as } = this.formatUrls(this.props.href, this.props.as)
+    let { href, as } = this.formatUrls(this.props.href, this.props.as)
+    as = as ? addBasePath(as) : as
+    href = addBasePath(href)
+
     // Deprecated. Warning shown by propType check. If the children provided is a string (<Link>example</Link>) we wrap it in an <a> tag
     if (typeof children === 'string') {
       children = <a>{children}</a>

packages/next/client/page-loader.js

@@ -3,7 +3,6 @@ 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 { delBasePath } from './../next-server/lib/router/router'
 
 function hasRel(rel, link) {
   try {
@@ -105,7 +104,6 @@ export default class PageLoader {
    */
   getDataHref(href, asPath) {
     const getHrefForSlug = (/** @type string */ path) => {
-      path = delBasePath(path)
       const dataRoute = getAssetPath(path)
       return `${this.assetPrefix}/_next/data/${this.buildId}${dataRoute}.json`
     }

packages/next/next-server/lib/router/router.ts

@@ -19,13 +19,11 @@ import { getRouteRegex } from './utils/route-regex'
 const basePath = (process.env.__NEXT_ROUTER_BASEPATH as string) || ''
 
 export function addBasePath(path: string): string {
-  return path.indexOf(basePath) !== 0 ? basePath + path : path
+  return `${basePath}${path}`
 }
 
 export function delBasePath(path: string): string {
-  return path.indexOf(basePath) === 0
-    ? path.substr(basePath.length) || '/'
-    : path
+  return path.substr(basePath.length)
 }
 
 function toRoute(path: string): string {
@@ -37,6 +35,21 @@ const prepareRoute = (path: string) =>
 
 type Url = UrlObject | string
 
+function prepareUrlAs(url: Url, as: Url) {
+  // If url and as provided as an object representation,
+  // we'll format them into the string version here.
+  url = typeof url === 'object' ? formatWithValidation(url) : url
+  as = typeof as === 'object' ? formatWithValidation(as) : as
+
+  url = addBasePath(url)
+  as = as ? addBasePath(as) : as
+
+  return {
+    url,
+    as,
+  }
+}
+
 type ComponentRes = { page: ComponentType; mod: any }
 
 export type BaseRouter = {
@@ -235,7 +248,7 @@ export default class Router implements BaseRouter {
         // we have to register the initial route upon initialization
         this.changeState(
           'replaceState',
-          formatWithValidation({ pathname, query }),
+          formatWithValidation({ pathname: addBasePath(pathname), query }),
           as
         )
       }
@@ -269,7 +282,7 @@ export default class Router implements BaseRouter {
       const { pathname, query } = this
       this.changeState(
         'replaceState',
-        formatWithValidation({ pathname, query }),
+        formatWithValidation({ pathname: addBasePath(pathname), query }),
         getURL()
       )
       return
@@ -300,7 +313,7 @@ export default class Router implements BaseRouter {
         )
       }
     }
-    this.replace(url, as, options)
+    this.change('replaceState', url, as, options)
   }
 
   update(route: string, mod: any) {
@@ -346,6 +359,7 @@ export default class Router implements BaseRouter {
    * @param options object you can define `shallow` and other options
    */
   push(url: Url, as: Url = url, options = {}) {
+    ;({ url, as } = prepareUrlAs(url, as))
     return this.change('pushState', url, as, options)
   }
 
@@ -356,13 +370,14 @@ export default class Router implements BaseRouter {
    * @param options object you can define `shallow` and other options
    */
   replace(url: Url, as: Url = url, options = {}) {
+    ;({ url, as } = prepareUrlAs(url, as))
     return this.change('replaceState', url, as, options)
   }
 
   change(
     method: HistoryMethod,
-    _url: Url,
-    _as: Url,
+    url: string,
+    as: string,
     options: any
   ): Promise<boolean> {
     return new Promise((resolve, reject) => {
@@ -374,18 +389,6 @@ export default class Router implements BaseRouter {
         performance.mark('routeChange')
       }
 
-      // If url and as provided as an object representation,
-      // we'll format them into the string version here.
-      let url = typeof _url === 'object' ? formatWithValidation(_url) : _url
-      let as = typeof _as === 'object' ? formatWithValidation(_as) : _as
-
-      // parse url parts without basePath since pathname should map 1-1 with
-      // pages dir
-      const { pathname, query, protocol } = parse(delBasePath(url), true)
-
-      url = addBasePath(url)
-      as = addBasePath(as)
-
       // Add the ending slash to the paths. So, we can serve the
       // "<page>/index.html" directly for the SSR page.
       if (process.env.__NEXT_EXPORT_TRAILING_SLASH) {
@@ -414,6 +417,13 @@ export default class Router implements BaseRouter {
         return resolve(true)
       }
 
+      let { pathname, query, protocol } = parse(url, true)
+
+      // url and as should always be prefixed with basePath by this
+      // point by either next/link or router.push/replace so strip the
+      // basePath from the pathname to match the pages dir 1-to-1
+      pathname = pathname ? delBasePath(pathname) : pathname
+
       if (!pathname || protocol) {
         if (process.env.NODE_ENV !== 'production') {
           throw new Error(
@@ -488,7 +498,7 @@ export default class Router implements BaseRouter {
               !(routeInfo.Component as any).getInitialProps
           }
 
-          this.set(route, pathname, query, as, routeInfo).then(() => {
+          this.set(route, pathname!, query, as, routeInfo).then(() => {
             if (error) {
               Router.events.emit('routeChangeError', error, as)
               throw error
@@ -760,9 +770,9 @@ export default class Router implements BaseRouter {
       if (process.env.NODE_ENV !== 'production') {
         return
       }
-      const route = delBasePath(toRoute(pathname))
+      const route = toRoute(pathname)
       Promise.all([
-        this.pageLoader.prefetchData(url, delBasePath(asPath)),
+        this.pageLoader.prefetchData(url, asPath),
         this.pageLoader[options.priority ? 'loadPage' : 'prefetch'](route),
       ]).then(() => resolve(), reject)
     })
@@ -773,7 +783,6 @@ export default class Router implements BaseRouter {
     const cancel = (this.clc = () => {
       cancelled = true
     })
-    route = delBasePath(route)
 
     const componentResult = await this.pageLoader.loadPage(route)
 

test/integration/basepath/pages/docs/another.js

@@ -0,0 +1 @@
+export default () => <p>hello from another</p>

test/integration/basepath/pages/invalid-manual-basepath.js

@@ -0,0 +1,11 @@
+import Link from 'next/link'
+
+export default () => (
+  <>
+    <Link href="/docs/other-page">
+      <a id="other-page-link">
+        <h1>Hello World</h1>
+      </a>
+    </Link>
+  </>
+)

test/integration/basepath/test/index.test.js

@@ -38,8 +38,118 @@ const runTests = (context, dev = false) => {
       )
       expect(routesManifest.basePath).toBe('/docs')
     })
+
+    it('should prefetch pages correctly when manually called', async () => {
+      const browser = await webdriver(context.appPort, '/docs/other-page')
+      await browser.eval('window.next.router.prefetch("/gssp")')
+
+      await check(
+        async () => {
+          const links = await browser.elementsByCss('link[rel=prefetch]')
+
+          for (const link of links) {
+            const href = await link.getAttribute('href')
+            if (href.includes('gssp')) {
+              return true
+            }
+          }
+          return false
+        },
+        {
+          test(result) {
+            return result === true
+          },
+        }
+      )
+    })
+
+    it('should prefetch pages correctly in viewport with <Link>', async () => {
+      const browser = await webdriver(context.appPort, '/docs/hello')
+      await browser.eval('window.next.router.prefetch("/gssp")')
+
+      await check(
+        async () => {
+          const links = await browser.elementsByCss('link[rel=prefetch]')
+          let found = new Set()
+
+          for (const link of links) {
+            const href = await link.getAttribute('href')
+            if (href.match(/(gsp|gssp|other-page)\.js$/)) {
+              found.add(href)
+            }
+            if (href.match(/gsp\.json$/)) {
+              found.add(href)
+            }
+          }
+          return found
+        },
+        {
+          test(result) {
+            return result.size === 4
+          },
+        }
+      )
+    })
   }
 
+  it('should work with nested folder with same name as basePath', async () => {
+    const html = await renderViaHTTP(context.appPort, '/docs/docs/another')
+    expect(html).toContain('hello from another')
+
+    const browser = await webdriver(context.appPort, '/docs/hello')
+    await browser.eval('window.next.router.push("/docs/another")')
+
+    await check(() => browser.elementByCss('p').text(), /hello from another/)
+  })
+
+  it('should 404 when manually adding basePath with <Link>', async () => {
+    const browser = await webdriver(
+      context.appPort,
+      '/docs/invalid-manual-basepath'
+    )
+    await browser.eval('window.beforeNav = "hi"')
+    await browser.elementByCss('#other-page-link').click()
+
+    await check(() => browser.eval('window.beforeNav'), {
+      test(content) {
+        return content !== 'hi'
+      },
+    })
+
+    const html = await browser.eval('document.documentElement.innerHTML')
+    expect(html).toContain('This page could not be found')
+  })
+
+  it('should 404 when manually adding basePath with router.push', async () => {
+    const browser = await webdriver(context.appPort, '/docs/hello')
+    await browser.eval('window.beforeNav = "hi"')
+    await browser.eval('window.next.router.push("/docs/other-page")')
+
+    await check(() => browser.eval('window.beforeNav'), {
+      test(content) {
+        return content !== 'hi'
+      },
+    })
+
+    const html = await browser.eval('document.documentElement.innerHTML')
+    expect(html).toContain('This page could not be found')
+  })
+
+  it('should 404 when manually adding basePath with router.replace', async () => {
+    const browser = await webdriver(context.appPort, '/docs/hello')
+    await browser.eval('window.beforeNav = "hi"')
+    await browser.eval('window.next.router.replace("/docs/other-page")')
+
+    await check(() => browser.eval('window.beforeNav'), {
+      test(content) {
+        return content !== 'hi'
+      },
+    })
+
+    const html = await browser.eval('document.documentElement.innerHTML')
+    expect(html).toContain('This page could not be found')
+  })
+
   it('should show the hello page under the /docs prefix', async () => {
     const browser = await webdriver(context.appPort, '/docs/hello')
     try {

test/integration/preload-viewport/test/index.test.js

@@ -247,7 +247,7 @@ describe('Prefetching Links in viewport', () => {
     expect(calledPrefetch).toBe(true)
   })
 
-  it('should correctly omit pre-generaged dynamic pages from SSG manifest', async () => {
+  it('should correctly omit pre-generated dynamic pages from SSG manifest', async () => {
     const content = await readFile(
       join(appDir, '.next', 'static', 'test-build', '_ssgManifest.js'),
       'utf8'