Skip to content

Commit

Permalink
Fix page checking failing with trailingSlash (#16362)
Browse files Browse the repository at this point in the history
This fixes page checking failing due to the trailing slash being present which causes pages to proxied by a rewrite when they shouldn't be. This also adds additional tests to ensure rewriting to an external resource is working correctly with `trailingSlash: true`

Fixes: #15700
  • Loading branch information
ijjk authored Aug 20, 2020
1 parent 681fbbd commit e1e6cfa
Show file tree
Hide file tree
Showing 8 changed files with 145 additions and 1 deletion.
5 changes: 4 additions & 1 deletion packages/next/next-server/server/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { IncomingMessage, ServerResponse } from 'http'
import { UrlWithParsedQuery } from 'url'

import pathMatch from './lib/path-match'
import { removePathTrailingSlash } from '../../client/normalize-trailing-slash'

export const route = pathMatch()

Expand Down Expand Up @@ -132,11 +133,13 @@ export default class Router {
requireBasePath: false,
match: route('/:path*'),
fn: async (checkerReq, checkerRes, params, parsedCheckerUrl) => {
const { pathname } = parsedCheckerUrl
let { pathname } = parsedCheckerUrl
pathname = removePathTrailingSlash(pathname || '/')

if (!pathname) {
return { finished: false }
}

if (await memoizedPageChecker(pathname)) {
return this.catchAllRoute.fn(
checkerReq,
Expand Down
16 changes: 16 additions & 0 deletions test/integration/trailing-slashes-rewrite/next.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
module.exports = {
trailingSlash: true,

async rewrites() {
return [
{
source: '/:path*/',
destination: '/:path*/',
},
{
source: '/:path*/',
destination: 'http://localhost:__EXTERNAL_PORT__/:path*',
},
]
},
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <p>catch-all</p>
}
3 changes: 3 additions & 0 deletions test/integration/trailing-slashes-rewrite/pages/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Index() {
return <p>index page</p>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Products() {
return <p>a product</p>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Products() {
return <p>some products</p>
}
10 changes: 10 additions & 0 deletions test/integration/trailing-slashes-rewrite/server.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
const http = require('http')
const server = http.createServer((req, res) => {
console.log('got request', req.url)
res.end(req.url)
})
const port = process.env.PORT || 3000

server.listen(port, () => {
console.log('Ready on http://localhost:' + port)
})
103 changes: 103 additions & 0 deletions test/integration/trailing-slashes-rewrite/test/index.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/* eslint-env jest */
import { join } from 'path'
import {
findPort,
killApp,
nextBuild,
nextStart,
File,
initNextServerScript,
renderViaHTTP,
launchApp,
} from 'next-test-utils'

jest.setTimeout(1000 * 60 * 2)

let app
let appPort
let proxyPort
let proxyServer
const appDir = join(__dirname, '../')
const nextConfig = new File(join(appDir, 'next.config.js'))

const runTests = () => {
it('should resolve index page correctly', async () => {
const html = await renderViaHTTP(appPort, '/')
expect(html).toContain('index page')
})

it('should resolve products page correctly', async () => {
const html = await renderViaHTTP(appPort, '/products')
expect(html).toContain('some products')
})

it('should resolve a dynamic page correctly', async () => {
const html = await renderViaHTTP(appPort, '/products/first')
expect(html).toContain('a product')
})

it('should resolve a catch-all page correctly', async () => {
const html = await renderViaHTTP(appPort, '/catch-all/hello')
expect(html).toContain('catch-all')
})

it('should proxy non-existent page correctly', async () => {
const html = await renderViaHTTP(appPort, '/non-existent')
expect(html).toBe('/non-existent')
})
}

describe('Trailing Slash Rewrite Proxying', () => {
describe('production mode', () => {
beforeAll(async () => {
proxyPort = await findPort()
proxyServer = await initNextServerScript(
join(appDir, 'server.js'),
/ready on/i,
{
...process.env,
PORT: proxyPort,
}
)

nextConfig.replace('__EXTERNAL_PORT__', proxyPort)

await nextBuild(appDir)
appPort = await findPort()
app = await nextStart(appDir, appPort)
})
afterAll(async () => {
nextConfig.restore()
await killApp(app)
await killApp(proxyServer)
})

runTests()
})

describe('dev mode', () => {
beforeAll(async () => {
proxyPort = await findPort()
proxyServer = await initNextServerScript(
join(appDir, 'server.js'),
/ready on/i,
{
...process.env,
PORT: proxyPort,
}
)

nextConfig.replace('__EXTERNAL_PORT__', proxyPort)

appPort = await findPort()
app = await launchApp(appDir, appPort)
})
afterAll(async () => {
nextConfig.restore()
await killApp(app)
await killApp(proxyServer)
})

runTests()
})
})

0 comments on commit e1e6cfa

Please sign in to comment.