fix: skipTrailingSlashRedirect being ignored in pages (#55067)

This moves `resolve-href` into `next/src/client` to make sure that when it calls `normalizeTrailingSlash`, that function has access to `process.env.__NEXT_MANUAL_TRAILING_SLASH` (inlined via `DefinePlugin`). Closes NEXT-1599 Fixes #54984
vercel · Sep 6, 2023 · 1f797fa · 1f797fa
1 parent 15980a6
commit 1f797fa
Show file tree

Hide file tree

Showing 8 changed files with 192 additions and 95 deletions.
diff --git a/packages/next/src/client/link.tsx b/packages/next/src/client/link.tsx
@@ -7,7 +7,7 @@ import type {
 
 import React from 'react'
 import { UrlObject } from 'url'
-import { resolveHref } from '../shared/lib/router/utils/resolve-href'
+import { resolveHref } from './resolve-href'
 import { isLocalURL } from '../shared/lib/router/utils/is-local-url'
 import { formatUrl } from '../shared/lib/router/utils/format-url'
 import { isAbsoluteUrl } from '../shared/lib/utils'

diff --git a/...c/shared/lib/router/utils/resolve-href.ts → packages/next/src/client/resolve-href.ts b/...c/shared/lib/router/utils/resolve-href.ts → packages/next/src/client/resolve-href.ts
@@ -1,13 +1,13 @@
-import type { NextRouter, Url } from '../router'
+import type { NextRouter, Url } from '../shared/lib/router/router'
 
-import { searchParamsToUrlQuery } from './querystring'
-import { formatWithValidation } from './format-url'
-import { omit } from './omit'
-import { normalizeRepeatedSlashes } from '../../utils'
-import { normalizePathTrailingSlash } from '../../../../client/normalize-trailing-slash'
-import { isLocalURL } from './is-local-url'
-import { isDynamicRoute } from './is-dynamic'
-import { interpolateAs } from './interpolate-as'
+import { searchParamsToUrlQuery } from '../shared/lib/router/utils/querystring'
+import { formatWithValidation } from '../shared/lib/router/utils/format-url'
+import { omit } from '../shared/lib/router/utils/omit'
+import { normalizeRepeatedSlashes } from '../shared/lib/utils'
+import { normalizePathTrailingSlash } from './normalize-trailing-slash'
+import { isLocalURL } from '../shared/lib/router/utils/is-local-url'
+import { isDynamicRoute } from '../shared/lib/router/utils'
+import { interpolateAs } from '../shared/lib/router/utils/interpolate-as'
 
 /**
  * Resolves a given hyperlink with a certain router state (basePath not included).

diff --git a/packages/next/src/shared/lib/router/router.ts b/packages/next/src/shared/lib/router/router.ts
@@ -40,14 +40,14 @@ import { removeLocale } from '../../../client/remove-locale'
 import { removeBasePath } from '../../../client/remove-base-path'
 import { addBasePath } from '../../../client/add-base-path'
 import { hasBasePath } from '../../../client/has-base-path'
+import { resolveHref } from '../../../client/resolve-href'
 import { isAPIRoute } from '../../../lib/is-api-route'
 import { getNextPathnameInfo } from './utils/get-next-pathname-info'
 import { formatNextPathnameInfo } from './utils/format-next-pathname-info'
 import { compareRouterStates } from './utils/compare-states'
 import { isLocalURL } from './utils/is-local-url'
 import { isBot } from './utils/is-bot'
 import { omit } from './utils/omit'
-import { resolveHref } from './utils/resolve-href'
 import { interpolateAs } from './utils/interpolate-as'
 import { handleSmoothScroll } from './utils/handle-smooth-scroll'
 

diff --git a/test/e2e/skip-trailing-slash-redirect/app/app/layout.js b/test/e2e/skip-trailing-slash-redirect/app/app/layout.js
@@ -0,0 +1,7 @@
+export default function RootLayout({ children }) {
+  return (
+    <html>
+      <body>{children}</body>
+    </html>
+  )
+}
diff --git a/test/e2e/skip-trailing-slash-redirect/app/app/with-app-dir/another/page.js b/test/e2e/skip-trailing-slash-redirect/app/app/with-app-dir/another/page.js
@@ -0,0 +1,13 @@
+import Link from 'next/link'
+
+export default function Page(props) {
+  return (
+    <>
+      <p id="another">another page</p>
+      <Link href="/with-app-dir" id="to-index">
+        to index
+      </Link>
+      <br />
+    </>
+  )
+}
diff --git a/test/e2e/skip-trailing-slash-redirect/app/app/with-app-dir/blog/[slug]/page.js b/test/e2e/skip-trailing-slash-redirect/app/app/with-app-dir/blog/[slug]/page.js
@@ -0,0 +1,13 @@
+import Link from 'next/link'
+
+export default function Page(props) {
+  return (
+    <>
+      <p id="blog">blog page</p>
+      <Link href="/with-app-dir" id="to-index">
+        to index
+      </Link>
+      <br />
+    </>
+  )
+}
diff --git a/test/e2e/skip-trailing-slash-redirect/app/app/with-app-dir/page.js b/test/e2e/skip-trailing-slash-redirect/app/app/with-app-dir/page.js
@@ -0,0 +1,21 @@
+import Link from 'next/link'
+
+export default function Page(props) {
+  return (
+    <>
+      <p id="index">index page</p>
+      <Link href="/with-app-dir/another" id="to-another">
+        to another
+      </Link>
+      <br />
+      <Link href="/with-app-dir/another/" id="to-another-with-slash">
+        to another
+      </Link>
+      <br />
+      <Link href="/with-app-dir/blog/first" id="to-blog-first">
+        to /blog/first
+      </Link>
+      <br />
+    </>
+  )
+}
diff --git a/test/e2e/skip-trailing-slash-redirect/index.test.ts b/test/e2e/skip-trailing-slash-redirect/index.test.ts
@@ -16,6 +16,129 @@ describe('skip-trailing-slash-redirect', () => {
   })
   afterAll(() => next.destroy())
 
+  // the tests below are run in both pages and app dir to ensure the behavior is the same
+  // the other cases aren't added to this block since they are either testing pages-specific behavior
+  // or aren't specific to either router implementation
+  async function runSharedTests(basePath: string) {
+    it('should not apply trailing slash redirect (with slash)', async () => {
+      const res = await fetchViaHTTP(
+        next.url,
+        `${basePath}another/`,
+        undefined,
+        {
+          redirect: 'manual',
+        }
+      )
+      expect(res.status).toBe(200)
+      expect(await res.text()).toContain('another page')
+    })
+
+    it('should not apply trailing slash redirect (without slash)', async () => {
+      const res = await fetchViaHTTP(
+        next.url,
+        `${basePath}another`,
+        undefined,
+        {
+          redirect: 'manual',
+        }
+      )
+      expect(res.status).toBe(200)
+      expect(await res.text()).toContain('another page')
+    })
+
+    it('should preserve original trailing slashes to links on client', async () => {
+      const browser = await webdriver(next.url, basePath)
+      await browser.eval('window.beforeNav = 1')
+
+      expect(
+        new URL(
+          await browser.elementByCss('#to-another').getAttribute('href'),
+          'http://n'
+        ).pathname
+      ).toBe(`${basePath}another`)
+
+      expect(
+        new URL(
+          await browser
+            .elementByCss('#to-another-with-slash')
+            .getAttribute('href'),
+          'http://n'
+        ).pathname
+      ).toBe(`${basePath}another/`)
+
+      await browser.elementByCss('#to-another').click()
+      await browser.waitForElementByCss('#another')
+
+      expect(await browser.eval('window.location.pathname')).toBe(
+        `${basePath}another`
+      )
+
+      await browser.back().waitForElementByCss('#to-another')
+
+      expect(
+        new URL(
+          await browser
+            .elementByCss('#to-another-with-slash')
+            .getAttribute('href'),
+          'http://n'
+        ).pathname
+      ).toBe(`${basePath}another/`)
+
+      await browser.elementByCss('#to-another-with-slash').click()
+      await browser.waitForElementByCss('#another')
+
+      expect(await browser.eval('window.location.pathname')).toBe(
+        `${basePath}another/`
+      )
+
+      await browser.back().waitForElementByCss('#to-another')
+      expect(await browser.eval('window.beforeNav')).toBe(1)
+    })
+
+    it('should respond to index correctly', async () => {
+      const res = await fetchViaHTTP(next.url, basePath, undefined, {
+        redirect: 'manual',
+      })
+      expect(res.status).toBe(200)
+      expect(await res.text()).toContain('index page')
+    })
+
+    it('should respond to dynamic route correctly', async () => {
+      const res = await fetchViaHTTP(
+        next.url,
+        `${basePath}blog/first`,
+        undefined,
+        {
+          redirect: 'manual',
+        }
+      )
+      expect(res.status).toBe(200)
+      expect(await res.text()).toContain('blog page')
+    })
+
+    it('should navigate client side correctly', async () => {
+      const browser = await webdriver(next.url, basePath)
+
+      expect(await browser.eval('location.pathname')).toBe(basePath)
+
+      await browser.elementByCss('#to-another').click()
+      await browser.waitForElementByCss('#another')
+
+      expect(await browser.eval('location.pathname')).toBe(`${basePath}another`)
+      await browser.back()
+      await browser.waitForElementByCss('#index')
+
+      expect(await browser.eval('location.pathname')).toBe(basePath)
+
+      await browser.elementByCss('#to-blog-first').click()
+      await browser.waitForElementByCss('#blog')
+
+      expect(await browser.eval('location.pathname')).toBe(
+        `${basePath}blog/first`
+      )
+    })
+  }
+
   it('should parse locale info for data request correctly', async () => {
     const pathname = `/_next/data/${next.buildId}/ja-jp/locale-test.json`
     const res = await next.fetch(pathname)
@@ -228,58 +351,6 @@ describe('skip-trailing-slash-redirect', () => {
     expect(await res.text()).toContain('another page')
   })
 
-  it('should not apply trailing slash redirect (with slash)', async () => {
-    const res = await fetchViaHTTP(next.url, '/another/', undefined, {
-      redirect: 'manual',
-    })
-    expect(res.status).toBe(200)
-    expect(await res.text()).toContain('another page')
-  })
-
-  it('should not apply trailing slash redirect (without slash)', async () => {
-    const res = await fetchViaHTTP(next.url, '/another', undefined, {
-      redirect: 'manual',
-    })
-    expect(res.status).toBe(200)
-    expect(await res.text()).toContain('another page')
-  })
-
-  it('should not apply trailing slash to links on client', async () => {
-    const browser = await webdriver(next.url, '/')
-    await browser.eval('window.beforeNav = 1')
-
-    expect(
-      new URL(
-        await browser.elementByCss('#to-another').getAttribute('href'),
-        'http://n'
-      ).pathname
-    ).toBe('/another')
-
-    await browser.elementByCss('#to-another').click()
-    await browser.waitForElementByCss('#another')
-
-    expect(await browser.eval('window.location.pathname')).toBe('/another')
-
-    await browser.back().waitForElementByCss('#to-another')
-
-    expect(
-      new URL(
-        await browser
-          .elementByCss('#to-another-with-slash')
-          .getAttribute('href'),
-        'http://n'
-      ).pathname
-    ).toBe('/another/')
-
-    await browser.elementByCss('#to-another-with-slash').click()
-    await browser.waitForElementByCss('#another')
-
-    expect(await browser.eval('window.location.pathname')).toBe('/another/')
-
-    await browser.back().waitForElementByCss('#to-another')
-    expect(await browser.eval('window.beforeNav')).toBe(1)
-  })
-
   it('should not apply trailing slash on load on client', async () => {
     let browser = await webdriver(next.url, '/another')
     await check(() => browser.eval('next.router.isReady ? "yes": "no"'), 'yes')
@@ -292,39 +363,11 @@ describe('skip-trailing-slash-redirect', () => {
     expect(await browser.eval('location.pathname')).toBe('/another/')
   })
 
-  it('should respond to index correctly', async () => {
-    const res = await fetchViaHTTP(next.url, '/', undefined, {
-      redirect: 'manual',
-    })
-    expect(res.status).toBe(200)
-    expect(await res.text()).toContain('index page')
+  describe('pages dir', () => {
+    runSharedTests('/')
   })
 
-  it('should respond to dynamic route correctly', async () => {
-    const res = await fetchViaHTTP(next.url, '/blog/first', undefined, {
-      redirect: 'manual',
-    })
-    expect(res.status).toBe(200)
-    expect(await res.text()).toContain('blog page')
-  })
-
-  it('should navigate client side correctly', async () => {
-    const browser = await webdriver(next.url, '/')
-
-    expect(await browser.eval('location.pathname')).toBe('/')
-
-    await browser.elementByCss('#to-another').click()
-    await browser.waitForElementByCss('#another')
-
-    expect(await browser.eval('location.pathname')).toBe('/another')
-    await browser.back()
-    await browser.waitForElementByCss('#index')
-
-    expect(await browser.eval('location.pathname')).toBe('/')
-
-    await browser.elementByCss('#to-blog-first').click()
-    await browser.waitForElementByCss('#blog')
-
-    expect(await browser.eval('location.pathname')).toBe('/blog/first')
+  describe('app dir', () => {
+    runSharedTests('/with-app-dir/')
   })
 })