Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[not for merge] Fix our route matching #9706

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions packages/next/next-server/server/next-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -581,9 +581,14 @@ export default class Server {
// this should not occur now that we check this during build
if (!this.pagesManifest![unixPath]) {
routes.push({
match: route(unixPath),
match: (pathname: string | undefined) => {
if (pathname === unixPath) {
return {}
}
return false
},
type: 'route',
name: 'public catchall',
name: 'public individual',
fn: async (req, res, _params, parsedUrl) => {
const p = join(this.publicDir, unixPath)
await this.serveStatic(req, res, p, parsedUrl)
Expand Down
6 changes: 6 additions & 0 deletions test/integration/public-dir/next.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module.exports = {
onDemandEntries: {
// Make sure entries are not getting disposed.
maxInactiveAge: 1000 * 60 * 60,
},
}
1 change: 1 addition & 0 deletions test/integration/public-dir/pages/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default () => <div>Hello World</div>
1 change: 1 addition & 0 deletions test/integration/public-dir/public/test space.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
i have a space
1 change: 1 addition & 0 deletions test/integration/public-dir/public/test+this.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
i have a plus
1 change: 1 addition & 0 deletions test/integration/public-dir/public/test:this.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
i have a :
86 changes: 86 additions & 0 deletions test/integration/public-dir/test/index.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/* eslint-env jest */
/* global jasmine */
import { join } from 'path'
import {
nextServer,
nextBuild,
startApp,
stopApp,
renderViaHTTP,
fetchViaHTTP,
} from 'next-test-utils'

const appDir = join(__dirname, '../')
let appPort
let server
let app
jasmine.DEFAULT_TIMEOUT_INTERVAL = 1000 * 60 * 2

describe('Public Files', () => {
describe('Production', () => {
beforeAll(async () => {
await nextBuild(appDir)
app = nextServer({
dir: join(__dirname, '../'),
dev: false,
quiet: true,
})

server = await startApp(app)
appPort = server.address().port
})
afterAll(() => stopApp(server))

describe('With basic usage', () => {
it('should render the page', async () => {
const html = await renderViaHTTP(appPort, '/')
expect(html).toMatch(/Hello World/)
})

it('should stream the file +', async () => {
const html = await renderViaHTTP(appPort, '/test+this.txt')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to handle /test%2Bthis.txt?

Copy link
Member Author

@Timer Timer Dec 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't handled previously. Also, see:

> require('path-to-regexp').match('/file/test\\:me.txt')('/file/test:me.txt')
{
  path: '/file/test:me.txt',
  index: 0,
  params: [Object: null prototype] {}
}
> encodeURIComponent(':')
'%3A'
> require('path-to-regexp').match('/file/test\\:me.txt')('/file/test%3Ame.txt')
false
> require('path-to-regexp').match('/file/test\\:me.txt')(decodeURI('/file/test%3Ame.txt'))
false

Not sure what we should do. I'll add tests that expect a 404 for the time being.

expect(html).toMatch(/i have a plus/)
})

it('should not stream the file + with trailing slash', async () => {
const res = await fetchViaHTTP(appPort, `/test+this.txt/`)
expect(res.status).toBe(404)
})

it('should not stream the file + encoded', async () => {
const res = await fetchViaHTTP(
appPort,
`/test${encodeURIComponent('+')}this.txt`
)
expect(res.status).toBe(404)
})

it('should stream the file :', async () => {
const html = await renderViaHTTP(appPort, '/test:this.txt')
expect(html).toMatch(/i have a :/)
})

it('should not stream the file : encoded', async () => {
const res = await fetchViaHTTP(
appPort,
`/test${encodeURIComponent(':')}this.txt`
)
expect(res.status).toBe(404)
})

xit('should stream the file space', async () => {
const html = await renderViaHTTP(appPort, '/test space.txt')
expect(html).toMatch(/i have a space/)
})

// Why is this one different? %20 is `encodeURI`.
xit('should stream the file space encoded', async () => {
const html = await renderViaHTTP(
appPort,
`/test${encodeURIComponent(' ')}space.txt`
)
expect(html).toMatch(/i have a space/)
})
})
})
})