Handle trailing slashes, even when query parameters (vercel#14650)

Add tests and fix for when the url contains query parameters. `router` now uses the same method for formatting url+as pair as `Link`, will be able to share code after vercel#14633 is merged
rokinsky · Jul 11, 2020 · 7ba3e09 · 7ba3e09
1 parent 6b9d918
commit 7ba3e09
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 23 deletions.
diff --git a/packages/next/next-server/lib/router/router.ts b/packages/next/next-server/lib/router/router.ts
@@ -38,24 +38,28 @@ function prepareRoute(path: string) {
 
 type Url = UrlObject | string
 
+function formatTrailingSlash(url: UrlObject): UrlObject {
+  return Object.assign({}, url, {
+    pathname:
+      url.pathname &&
+      normalizeTrailingSlash(url.pathname, !!process.env.__NEXT_TRAILING_SLASH),
+  })
+}
+
+function formatUrl(url: Url): string {
+  return url
+    ? formatWithValidation(
+        formatTrailingSlash(typeof url === 'object' ? url : parse(url))
+      )
+    : url
+}
+
 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(
-    normalizeTrailingSlash(url, !!process.env.__NEXT_TRAILING_SLASH)
-  )
-  as = as
-    ? addBasePath(
-        normalizeTrailingSlash(as, !!process.env.__NEXT_TRAILING_SLASH)
-      )
-    : as
-
   return {
-    url,
-    as,
+    url: addBasePath(formatUrl(url)),
+    as: as ? addBasePath(formatUrl(as)) : as,
   }
 }
 
@@ -436,7 +440,9 @@ export default class Router implements BaseRouter {
       // 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
+      pathname = pathname
+        ? normalizeTrailingSlash(delBasePath(pathname), false)
+        : pathname
 
       if (!pathname || protocol) {
         if (process.env.NODE_ENV !== 'production') {

diff --git a/test/integration/trailing-slashes/test/index.test.js b/test/integration/trailing-slashes/test/index.test.js
@@ -86,7 +86,8 @@ function testLinkShouldRewriteTo(expectations) {
         await browser.elementByCss('#link').click()
 
         await browser.waitForElementByCss('#hydration-marker')
-        const { pathname } = new URL(await browser.eval('window.location.href'))
+        const url = new URL(await browser.eval('window.location.href'))
+        const pathname = url.href.slice(url.origin.length)
         expect(pathname).toBe(expectedHref)
       } finally {
         if (browser) await browser.close()
@@ -103,7 +104,8 @@ function testLinkShouldRewriteTo(expectations) {
         await browser.elementByCss('#route-pusher').click()
 
         await browser.waitForElementByCss('#hydration-marker')
-        const { pathname } = new URL(await browser.eval('window.location.href'))
+        const url = new URL(await browser.eval('window.location.href'))
+        const pathname = url.href.slice(url.origin.length)
         expect(pathname).toBe(expectedHref)
       } finally {
         if (browser) await browser.close()
@@ -112,7 +114,7 @@ function testLinkShouldRewriteTo(expectations) {
   )
 }
 
-function testWithTrailingSlash() {
+function testWithoutTrailingSlash() {
   testShouldRedirect([
     ['/about/', '/about'],
     ['/catch-all/hello/world/', '/catch-all/hello/world'],
@@ -127,16 +129,19 @@ function testWithTrailingSlash() {
       '/catch-all/[...slug].js',
       '/catch-all/[...slug]',
     ],
+    ['/about?hello=world', '/about.js', '/about'],
   ])
 
   testLinkShouldRewriteTo([
     ['/', '/'],
     ['/about', '/about'],
     ['/about/', '/about'],
+    ['/about?hello=world', '/about?hello=world'],
+    ['/about/?hello=world', '/about?hello=world'],
   ])
 }
 
-function testWithoutTrailingSlash() {
+function testWithTrailingSlash() {
   testShouldRedirect([
     ['/about', '/about/'],
     ['/catch-all/hello/world', '/catch-all/hello/world/'],
@@ -151,12 +156,15 @@ function testWithoutTrailingSlash() {
       '/catch-all/[...slug].js',
       '/catch-all/[...slug]',
     ],
+    ['/about/?hello=world', '/about.js', '/about'],
   ])
 
   testLinkShouldRewriteTo([
     ['/', '/'],
     ['/about', '/about/'],
     ['/about/', '/about/'],
+    ['/about?hello=world', '/about/?hello=world'],
+    ['/about/?hello=world', '/about/?hello=world'],
   ])
 }
 
@@ -177,7 +185,7 @@ describe('Trailing slashes', () => {
       await killApp(app)
     })
 
-    testWithTrailingSlash()
+    testWithoutTrailingSlash()
   })
 
   describe('dev mode, trailingSlash: true', () => {
@@ -196,7 +204,7 @@ describe('Trailing slashes', () => {
       await killApp(app)
     })
 
-    testWithoutTrailingSlash()
+    testWithTrailingSlash()
   })
 
   describe('production mode, trailingSlash: false', () => {
@@ -217,7 +225,7 @@ describe('Trailing slashes', () => {
       await killApp(app)
     })
 
-    testWithTrailingSlash()
+    testWithoutTrailingSlash()
 
     it('should have a redirect in the routesmanifest', async () => {
       const manifest = await fs.readJSON(
@@ -255,7 +263,7 @@ describe('Trailing slashes', () => {
       await killApp(app)
     })
 
-    testWithoutTrailingSlash()
+    testWithTrailingSlash()
 
     it('should have a redirect in the routesmanifest', async () => {
       const manifest = await fs.readJSON(