fix basepath trailing slash (#15200)

Fixes the link rewriting part of https://github.com/vercel/next.js/issues/15194

packages/next/client/normalize-trailing-slash.ts

-import { UrlObject } from 'url'
-
 /**
  * Removes the trailing slash of a path if there is one. Preserves the root path `/`.
  */
@@ -11,7 +9,7 @@ export function removePathTrailingSlash(path: string): string {
  * Normalizes the trailing slash of a path according to the `trailingSlash` option
  * in `next.config.js`.
  */
-const normalizePathTrailingSlash = process.env.__NEXT_TRAILING_SLASH
+export const normalizePathTrailingSlash = process.env.__NEXT_TRAILING_SLASH
   ? (path: string): string => {
       if (/\.[^/]+\/?$/.test(path)) {
         return removePathTrailingSlash(path)
@@ -22,18 +20,3 @@ const normalizePathTrailingSlash = process.env.__NEXT_TRAILING_SLASH
       }
     }
   : removePathTrailingSlash
-
-/**
- * Normalizes the trailing slash of the path of a parsed url. Non-destructive.
- */
-export function normalizeTrailingSlash(url: URL): URL
-export function normalizeTrailingSlash(url: UrlObject): UrlObject
-export function normalizeTrailingSlash(url: UrlObject | URL): UrlObject | URL {
-  const normalizedPath =
-    url.pathname && normalizePathTrailingSlash(url.pathname)
-  return url.pathname === normalizedPath
-    ? url
-    : url instanceof URL
-    ? Object.assign(new URL(url.href), { pathname: normalizedPath })
-    : Object.assign({}, url, { pathname: normalizedPath })
-}

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

@@ -18,14 +18,18 @@ import { getRouteRegex } from './utils/route-regex'
 import { searchParamsToUrlQuery } from './utils/search-params-to-url-query'
 import { parseRelativeUrl } from './utils/parse-relative-url'
 import {
-  normalizeTrailingSlash,
   removePathTrailingSlash,
+  normalizePathTrailingSlash,
 } from '../../../client/normalize-trailing-slash'
 
 const basePath = (process.env.__NEXT_ROUTER_BASEPATH as string) || ''
 
 export function addBasePath(path: string): string {
-  return basePath ? (path === '/' ? basePath : basePath + path) : path
+  return basePath
+    ? path === '/'
+      ? normalizePathTrailingSlash(basePath)
+      : basePath + path
+    : path
 }
 
 export function delBasePath(path: string): string {
@@ -47,7 +51,8 @@ export function resolveHref(currentPath: string, href: Url): string {
   const base = new URL(currentPath, 'http://n')
   const urlAsString =
     typeof href === 'string' ? href : formatWithValidation(href)
-  const finalUrl = normalizeTrailingSlash(new URL(urlAsString, base))
+  const finalUrl = new URL(urlAsString, base)
+  finalUrl.pathname = normalizePathTrailingSlash(finalUrl.pathname)
   // if the origin didn't change, it means we received a relative href
   return finalUrl.origin === base.origin
     ? finalUrl.href.slice(finalUrl.origin.length)

test/integration/trailing-slashes/next.config.js

@@ -2,4 +2,5 @@ module.exports = {
   experimental: {
     // <placeholder>
   },
+  // basePath: '/docs',
 }

test/integration/trailing-slashes/test/index.test.js

@@ -12,6 +12,7 @@ import {
   launchApp,
   nextBuild,
   nextStart,
+  File,
 } from 'next-test-utils'
 import { join } from 'path'
 
@@ -20,7 +21,7 @@ jest.setTimeout(1000 * 60 * 2)
 let app
 let appPort
 const appDir = join(__dirname, '../')
-const nextConfig = join(appDir, 'next.config.js')
+const nextConfig = new File(join(appDir, 'next.config.js'))
 
 function testShouldRedirect(expectations) {
   it.each(expectations)(
@@ -70,8 +71,8 @@ function testShouldResolve(expectations) {
 function testLinkShouldRewriteTo(expectations) {
   it.each(expectations)(
     '%s should have href %s',
-    async (href, expectedHref) => {
-      const content = await renderViaHTTP(appPort, `/linker?href=${href}`)
+    async (linkPage, expectedHref) => {
+      const content = await renderViaHTTP(appPort, linkPage)
       const $ = cheerio.load(content)
       expect($('#link').attr('href')).toBe(expectedHref)
     }
@@ -79,10 +80,10 @@ function testLinkShouldRewriteTo(expectations) {
 
   it.each(expectations)(
     '%s should navigate to %s',
-    async (href, expectedHref) => {
+    async (linkPage, expectedHref) => {
       let browser
       try {
-        browser = await webdriver(appPort, `/linker?href=${href}`)
+        browser = await webdriver(appPort, linkPage)
         await browser.elementByCss('#link').click()
 
         await browser.waitForElementByCss('#hydration-marker')
@@ -97,10 +98,10 @@ function testLinkShouldRewriteTo(expectations) {
 
   it.each(expectations)(
     '%s should push route to %s',
-    async (href, expectedHref) => {
+    async (linkPage, expectedHref) => {
       let browser
       try {
-        browser = await webdriver(appPort, `/linker?href=${href}`)
+        browser = await webdriver(appPort, linkPage)
         await browser.elementByCss('#route-pusher').click()
 
         await browser.waitForElementByCss('#hydration-marker')
@@ -134,13 +135,13 @@ function testWithoutTrailingSlash() {
   ])
 
   testLinkShouldRewriteTo([
-    ['/', '/'],
-    ['/about', '/about'],
-    ['/about/', '/about'],
-    ['/about?hello=world', '/about?hello=world'],
-    ['/about/?hello=world', '/about?hello=world'],
-    ['/catch-all/hello/', '/catch-all/hello'],
-    ['/catch-all/hello.world/', '/catch-all/hello.world'],
+    ['/linker?href=/', '/'],
+    ['/linker?href=/about', '/about'],
+    ['/linker?href=/about/', '/about'],
+    ['/linker?href=/about?hello=world', '/about?hello=world'],
+    ['/linker?href=/about/?hello=world', '/about?hello=world'],
+    ['/linker?href=/catch-all/hello/', '/catch-all/hello'],
+    ['/linker?href=/catch-all/hello.world/', '/catch-all/hello.world'],
   ])
 }
 
@@ -164,30 +165,25 @@ function testWithTrailingSlash() {
   ])
 
   testLinkShouldRewriteTo([
-    ['/', '/'],
-    ['/about', '/about/'],
-    ['/about/', '/about/'],
-    ['/about?hello=world', '/about/?hello=world'],
-    ['/about/?hello=world', '/about/?hello=world'],
-    ['/catch-all/hello/', '/catch-all/hello/'],
-    ['/catch-all/hello.world/', '/catch-all/hello.world'],
+    ['/linker?href=/', '/'],
+    ['/linker?href=/about', '/about/'],
+    ['/linker?href=/about/', '/about/'],
+    ['/linker?href=/about?hello=world', '/about/?hello=world'],
+    ['/linker?href=/about/?hello=world', '/about/?hello=world'],
+    ['/linker?href=/catch-all/hello/', '/catch-all/hello/'],
+    ['/linker?href=/catch-all/hello.world/', '/catch-all/hello.world'],
   ])
 }
 
 describe('Trailing slashes', () => {
   describe('dev mode, trailingSlash: false', () => {
-    let origNextConfig
     beforeAll(async () => {
-      origNextConfig = await fs.readFile(nextConfig, 'utf8')
-      await fs.writeFile(
-        nextConfig,
-        origNextConfig.replace('// <placeholder>', 'trailingSlash: false')
-      )
+      nextConfig.replace('// <placeholder>', 'trailingSlash: false')
       appPort = await findPort()
       app = await launchApp(appDir, appPort)
     })
     afterAll(async () => {
-      await fs.writeFile(nextConfig, origNextConfig)
+      nextConfig.restore()
       await killApp(app)
     })
 
@@ -195,18 +191,13 @@ describe('Trailing slashes', () => {
   })
 
   describe('dev mode, trailingSlash: true', () => {
-    let origNextConfig
     beforeAll(async () => {
-      origNextConfig = await fs.readFile(nextConfig, 'utf8')
-      await fs.writeFile(
-        nextConfig,
-        origNextConfig.replace('// <placeholder>', 'trailingSlash: true')
-      )
+      nextConfig.replace('// <placeholder>', 'trailingSlash: true')
       appPort = await findPort()
       app = await launchApp(appDir, appPort)
     })
     afterAll(async () => {
-      await fs.writeFile(nextConfig, origNextConfig)
+      nextConfig.restore()
       await killApp(app)
     })
 
@@ -214,20 +205,14 @@ describe('Trailing slashes', () => {
   })
 
   describe('production mode, trailingSlash: false', () => {
-    let origNextConfig
     beforeAll(async () => {
-      origNextConfig = await fs.readFile(nextConfig, 'utf8')
-      await fs.writeFile(
-        nextConfig,
-        origNextConfig.replace('// <placeholder>', 'trailingSlash: false')
-      )
+      nextConfig.replace('// <placeholder>', 'trailingSlash: false')
       await nextBuild(appDir)
-
       appPort = await findPort()
       app = await nextStart(appDir, appPort)
     })
     afterAll(async () => {
-      await fs.writeFile(nextConfig, origNextConfig)
+      nextConfig.restore()
       await killApp(app)
     })
 
@@ -252,20 +237,14 @@ describe('Trailing slashes', () => {
   })
 
   describe('production mode, trailingSlash: true', () => {
-    let origNextConfig
     beforeAll(async () => {
-      origNextConfig = await fs.readFile(nextConfig, 'utf8')
-      await fs.writeFile(
-        nextConfig,
-        origNextConfig.replace('// <placeholder>', 'trailingSlash: true')
-      )
+      nextConfig.replace('// <placeholder>', 'trailingSlash: true')
       await nextBuild(appDir)
-
       appPort = await findPort()
       app = await nextStart(appDir, appPort)
     })
     afterAll(async () => {
-      await fs.writeFile(nextConfig, origNextConfig)
+      nextConfig.restore()
       await killApp(app)
     })
 
@@ -295,20 +274,14 @@ describe('Trailing slashes', () => {
   })
 
   describe('dev mode, with basepath, trailingSlash: true', () => {
-    let origNextConfig
     beforeAll(async () => {
-      origNextConfig = await fs.readFile(nextConfig, 'utf8')
-      await fs.writeFile(
-        nextConfig,
-        origNextConfig
-          .replace('// <placeholder>', 'trailingSlash: true')
-          .replace('// basePath:', 'basePath:')
-      )
+      nextConfig.replace('// <placeholder>', 'trailingSlash: true')
+      nextConfig.replace('// basePath:', 'basePath:')
       appPort = await findPort()
       app = await launchApp(appDir, appPort)
     })
     afterAll(async () => {
-      await fs.writeFile(nextConfig, origNextConfig)
+      nextConfig.restore()
       await killApp(app)
     })
 
@@ -318,25 +291,23 @@ describe('Trailing slashes', () => {
       ['/docs/catch-all/hello/world', '/docs/catch-all/hello/world/'],
       ['/docs/catch-all/hello.world/', '/docs/catch-all/hello.world'],
     ])
+
+    testLinkShouldRewriteTo([
+      ['/docs/linker?href=/about', '/docs/about/'],
+      ['/docs/linker?href=/', '/docs/'],
+    ])
   })
 
   describe('production mode, with basepath, trailingSlash: true', () => {
-    let origNextConfig
     beforeAll(async () => {
-      origNextConfig = await fs.readFile(nextConfig, 'utf8')
-      await fs.writeFile(
-        nextConfig,
-        origNextConfig
-          .replace('// <placeholder>', 'trailingSlash: true')
-          .replace('// basePath:', 'basePath:')
-      )
+      nextConfig.replace('// <placeholder>', 'trailingSlash: true')
+      nextConfig.replace('// basePath:', 'basePath:')
       await nextBuild(appDir)
-
       appPort = await findPort()
       app = await nextStart(appDir, appPort)
     })
     afterAll(async () => {
-      await fs.writeFile(nextConfig, origNextConfig)
+      nextConfig.restore()
       await killApp(app)
     })
 
@@ -346,5 +317,10 @@ describe('Trailing slashes', () => {
       ['/docs/catch-all/hello/world', '/docs/catch-all/hello/world/'],
       ['/docs/catch-all/hello.world/', '/docs/catch-all/hello.world'],
     ])
+
+    testLinkShouldRewriteTo([
+      ['/docs/linker?href=/about', '/docs/about/'],
+      ['/docs/linker?href=/', '/docs/'],
+    ])
   })
 })

test/lib/next-test-utils.js

@@ -393,25 +393,24 @@ export class File {
   }
 
   replace(pattern, newValue) {
+    const currentContent = readFileSync(this.path, 'utf8')
     if (pattern instanceof RegExp) {
-      if (!pattern.test(this.originalContent)) {
+      if (!pattern.test(currentContent)) {
         throw new Error(
-          `Failed to replace content.\n\nPattern: ${pattern.toString()}\n\nContent: ${
-            this.originalContent
-          }`
+          `Failed to replace content.\n\nPattern: ${pattern.toString()}\n\nContent: ${currentContent}`
         )
       }
     } else if (typeof pattern === 'string') {
-      if (!this.originalContent.includes(pattern)) {
+      if (!currentContent.includes(pattern)) {
         throw new Error(
-          `Failed to replace content.\n\nPattern: ${pattern}\n\nContent: ${this.originalContent}`
+          `Failed to replace content.\n\nPattern: ${pattern}\n\nContent: ${currentContent}`
         )
       }
     } else {
       throw new Error(`Unknown replacement attempt type: ${pattern}`)
     }
 
-    const newContent = this.originalContent.replace(pattern, newValue)
+    const newContent = currentContent.replace(pattern, newValue)
     this.write(newContent)
   }