From 1f797faaa6933ab2b8311251a167613f5a75a901 Mon Sep 17 00:00:00 2001 From: Zack Tanner Date: Wed, 6 Sep 2023 11:03:37 -0700 Subject: [PATCH] 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 --- packages/next/src/client/link.tsx | 2 +- .../router/utils => client}/resolve-href.ts | 18 +- packages/next/src/shared/lib/router/router.ts | 2 +- .../app/app/layout.js | 7 + .../app/app/with-app-dir/another/page.js | 13 ++ .../app/app/with-app-dir/blog/[slug]/page.js | 13 ++ .../app/app/with-app-dir/page.js | 21 ++ .../index.test.ts | 211 +++++++++++------- 8 files changed, 192 insertions(+), 95 deletions(-) rename packages/next/src/{shared/lib/router/utils => client}/resolve-href.ts (81%) create mode 100644 test/e2e/skip-trailing-slash-redirect/app/app/layout.js create mode 100644 test/e2e/skip-trailing-slash-redirect/app/app/with-app-dir/another/page.js create mode 100644 test/e2e/skip-trailing-slash-redirect/app/app/with-app-dir/blog/[slug]/page.js create mode 100644 test/e2e/skip-trailing-slash-redirect/app/app/with-app-dir/page.js diff --git a/packages/next/src/client/link.tsx b/packages/next/src/client/link.tsx index 6048a87191c46..94226c8caa5c0 100644 --- 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/packages/next/src/shared/lib/router/utils/resolve-href.ts b/packages/next/src/client/resolve-href.ts similarity index 81% rename from packages/next/src/shared/lib/router/utils/resolve-href.ts rename to packages/next/src/client/resolve-href.ts index b4c33ae353da8..af4458f270e20 100644 --- a/packages/next/src/shared/lib/router/utils/resolve-href.ts +++ b/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 index 2074bc2b4441f..a0d1cf13ffd15 100644 --- a/packages/next/src/shared/lib/router/router.ts +++ b/packages/next/src/shared/lib/router/router.ts @@ -40,6 +40,7 @@ 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' @@ -47,7 +48,6 @@ 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 new file mode 100644 index 0000000000000..803f17d863c8a --- /dev/null +++ b/test/e2e/skip-trailing-slash-redirect/app/app/layout.js @@ -0,0 +1,7 @@ +export default function RootLayout({ children }) { + return ( + + {children} + + ) +} 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 new file mode 100644 index 0000000000000..d284d5354459f --- /dev/null +++ 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 ( + <> +

another page

+ + to index + +
+ + ) +} 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 new file mode 100644 index 0000000000000..d5e27f34586ef --- /dev/null +++ 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 ( + <> +

blog page

+ + to index + +
+ + ) +} 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 new file mode 100644 index 0000000000000..257550314cecb --- /dev/null +++ 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 ( + <> +

index page

+ + to another + +
+ + to another + +
+ + to /blog/first + +
+ + ) +} diff --git a/test/e2e/skip-trailing-slash-redirect/index.test.ts b/test/e2e/skip-trailing-slash-redirect/index.test.ts index a1fd62379b6a3..d9cf1e90adbed 100644 --- 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/') }) })