From 54d991e6420d5493242a933037a8192ade0b53b8 Mon Sep 17 00:00:00 2001 From: Jan Potoms <2109932+Janpot@users.noreply.github.com> Date: Thu, 16 Jul 2020 01:53:31 +0200 Subject: [PATCH] fix basepath trailing slash (#15200) Fixes the link rewriting part of https://github.com/vercel/next.js/issues/15194 --- .../next/client/normalize-trailing-slash.ts | 19 +-- .../next/next-server/lib/router/router.ts | 11 +- .../trailing-slashes/next.config.js | 1 + .../trailing-slashes/test/index.test.js | 116 +++++++----------- test/lib/next-test-utils.js | 13 +- 5 files changed, 62 insertions(+), 98 deletions(-) diff --git a/packages/next/client/normalize-trailing-slash.ts b/packages/next/client/normalize-trailing-slash.ts index fdb22d9fad..6cbd6f72e0 100644 --- a/packages/next/client/normalize-trailing-slash.ts +++ b/packages/next/client/normalize-trailing-slash.ts @@ -1,5 +1,3 @@ -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 }) -} diff --git a/packages/next/next-server/lib/router/router.ts b/packages/next/next-server/lib/router/router.ts index 629f865893..182a28145f 100644 --- a/packages/next/next-server/lib/router/router.ts +++ b/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) diff --git a/test/integration/trailing-slashes/next.config.js b/test/integration/trailing-slashes/next.config.js index 77feb6c409..9da32c0d7a 100644 --- a/test/integration/trailing-slashes/next.config.js +++ b/test/integration/trailing-slashes/next.config.js @@ -2,4 +2,5 @@ module.exports = { experimental: { // }, + // basePath: '/docs', } diff --git a/test/integration/trailing-slashes/test/index.test.js b/test/integration/trailing-slashes/test/index.test.js index 97682630de..44bc308c71 100644 --- a/test/integration/trailing-slashes/test/index.test.js +++ b/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('// ', 'trailingSlash: false') - ) + nextConfig.replace('// ', '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('// ', 'trailingSlash: true') - ) + nextConfig.replace('// ', '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('// ', 'trailingSlash: false') - ) + nextConfig.replace('// ', '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('// ', 'trailingSlash: true') - ) + nextConfig.replace('// ', '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('// ', 'trailingSlash: true') - .replace('// basePath:', 'basePath:') - ) + nextConfig.replace('// ', '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('// ', 'trailingSlash: true') - .replace('// basePath:', 'basePath:') - ) + nextConfig.replace('// ', '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/'], + ]) }) }) diff --git a/test/lib/next-test-utils.js b/test/lib/next-test-utils.js index ba201e981b..131e9f6f92 100644 --- a/test/lib/next-test-utils.js +++ b/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) } -- GitLab