Skip to content

Commit

Permalink
fix basepath trailing slash (#15200)
Browse files Browse the repository at this point in the history 
Fixes the link rewriting part of #15194
  • Loading branch information
@Janpot
Janpot authored Jul 15, 2020
1 parent 1fe612e commit 54d991e
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 98 deletions.
19 changes: 1 addition & 18 deletions packages/next/client/normalize-trailing-slash.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import { UrlObject } from 'url'

/**
* Removes the trailing slash of a path if there is one. Preserves the root path `/`.
*/
Expand All @@ -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)
Expand All @@ -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 })
}
11 changes: 8 additions & 3 deletions packages/next/next-server/lib/router/router.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
Expand Down
1 change: 1 addition & 0 deletions test/integration/trailing-slashes/next.config.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@ module.exports = {
experimental: {
// <placeholder>
},
// basePath: '/docs',
}
116 changes: 46 additions & 70 deletions test/integration/trailing-slashes/test/index.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
launchApp,
nextBuild,
nextStart,
File,
} from 'next-test-utils'
import { join } from 'path'

Expand All @@ -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)(
Expand Down Expand Up @@ -70,19 +71,19 @@ 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)
}
)

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')
Expand All @@ -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')
Expand Down Expand Up @@ -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'],
])
}

Expand All @@ -164,70 +165,54 @@ 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)
})

testWithoutTrailingSlash()
})

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)
})

testWithTrailingSlash()
})

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)
})

Expand All @@ -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)
})

Expand Down Expand Up @@ -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)
})

Expand All @@ -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)
})

Expand All @@ -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/'],
])
})
})
13 changes: 6 additions & 7 deletions test/lib/next-test-utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down

0 comments on commit 54d991e

Please sign in to comment.