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.

Sign up for GitHub

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

Jump to bottom

Support trailing slashes, not extraneous ones #3158

Merged
merged 1 commit into from
Apr 1, 2016
Merged

Support trailing slashes, not extraneous ones #3158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
42 changes: 19 additions & 23 deletions modules/PatternUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,6 @@ function escapeRegExp(string) {
return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')
}

function escapeSource(string) {
return escapeRegExp(string).replace(/\/+/g, '/+')
}

function _compilePattern(pattern) {
let regexpSource = ''
const paramNames = []
Expand All @@ -17,7 +13,7 @@ function _compilePattern(pattern) {
while ((match = matcher.exec(pattern))) {
if (match.index !== lastIndex) {
tokens.push(pattern.slice(lastIndex, match.index))
regexpSource += escapeSource(pattern.slice(lastIndex, match.index))
regexpSource += escapeRegExp(pattern.slice(lastIndex, match.index))
}

if (match[1]) {
Expand All @@ -42,7 +38,7 @@ function _compilePattern(pattern) {

if (lastIndex !== pattern.length) {
tokens.push(pattern.slice(lastIndex, pattern.length))
regexpSource += escapeSource(pattern.slice(lastIndex, pattern.length))
regexpSource += escapeRegExp(pattern.slice(lastIndex, pattern.length))
}

return {
Expand Down Expand Up @@ -82,17 +78,15 @@ export function compilePattern(pattern) {
* - paramValues
*/
export function matchPattern(pattern, pathname) {
// Make leading slashes consistent between pattern and pathname.
// Ensure pattern starts with leading slash for consistency with pathname.
if (pattern.charAt(0) !== '/') {
pattern = `/${pattern}`
}
if (pathname.charAt(0) !== '/') {
pathname = `/${pathname}`
}

let { regexpSource, paramNames, tokens } = compilePattern(pattern)

regexpSource += '/*' // Capture path separators
if (pattern.charAt(pattern.length - 1) !== '/') {
regexpSource += '/?' // Allow optional path separator at end.
}

// Special-case patterns like '*' for catch-all routes.
if (tokens[tokens.length - 1] === '*') {
Expand All @@ -106,18 +100,20 @@ export function matchPattern(pattern, pathname) {
const matchedPath = match[0]
remainingPathname = pathname.substr(matchedPath.length)

// If we didn't match the entire pathname, then make sure that the match we
// did get ends at a path separator (potentially the one we added above at
// the beginning of the path, if the actual match was empty).
if (
remainingPathname &&
matchedPath.charAt(matchedPath.length - 1) !== '/'
) {
return {
remainingPathname: null,
paramNames,
paramValues: null
if (remainingPathname) {
// Require that the match ends at a path separator, if we didn't match
// the full path, so any remaining pathname is a new path segment.
if (matchedPath.charAt(matchedPath.length - 1) !== '/') {
return {
remainingPathname: null,
paramNames,
paramValues: null
}
}

// If there is a remaining pathname, treat the path separator as part of
// the remaining pathname for properly continuing the match.
remainingPathname = `/${remainingPathname}`
}

paramValues = match.slice(1).map(v => v && decodeURIComponent(v))
Expand Down
2 changes: 1 addition & 1 deletion modules/__tests__/_bc-Link-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ describe('v1 Link', function () {
Michael
</Link>
<Link
to="hello/ryan" query={{ the: 'query' }}
to="/hello/ryan" query={{ the: 'query' }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't supposed to work, and now in fact it actually doesn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To elaborate on "not supposed to work" – links like this will do random, dangerous things when used with e.g. browser history right now, given the lack of support there for relative links.

activeClassName="active"
>
Ryan
Expand Down
16 changes: 8 additions & 8 deletions modules/__tests__/_bc-isActive-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,15 @@ describe('v1 isActive', function () {
})
})

it('is active with extraneous slashes', function (done) {
it('is not active with extraneous slashes', function (done) {
render((
<Router history={createHistory('/parent/child')}>
<Route path="/parent">
<Route path="child" />
</Route>
</Router>
), node, function () {
expect(this.history.isActive('/parent////child////')).toBe(true)
expect(this.history.isActive('/parent////child////')).toBe(false)
done()
})
})
Expand Down Expand Up @@ -288,7 +288,7 @@ describe('v1 isActive', function () {
})
})

it('is active with extraneous slashes', function (done) {
it('is not active with extraneous slashes', function (done) {
render((
<Router history={createHistory('/parent/child')}>
<Route path="/parent">
Expand All @@ -298,8 +298,8 @@ describe('v1 isActive', function () {
</Route>
</Router>
), node, function () {
expect(this.history.isActive('/parent///child///', null)).toBe(true)
expect(this.history.isActive('/parent///child///', null, true)).toBe(true)
expect(this.history.isActive('/parent///child///', null)).toBe(false)
expect(this.history.isActive('/parent///child///', null, true)).toBe(false)
done()
})
})
Expand All @@ -324,7 +324,7 @@ describe('v1 isActive', function () {
})
})

it('is active with extraneous slashes', function (done) {
it('is not active with extraneous slashes', function (done) {
render((
<Router history={createHistory('/parent/child')}>
<Route path="/parent">
Expand All @@ -336,8 +336,8 @@ describe('v1 isActive', function () {
</Route>
</Router>
), node, function () {
expect(this.history.isActive('/parent///child///', null)).toBe(true)
expect(this.history.isActive('/parent///child///', null, true)).toBe(true)
expect(this.history.isActive('/parent///child///', null)).toBe(false)
expect(this.history.isActive('/parent///child///', null, true)).toBe(false)
done()
})
})
Expand Down
120 changes: 112 additions & 8 deletions modules/__tests__/isActive-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,15 +114,43 @@ describe('isActive', function () {
})
})

it('is active with extraneous slashes', function (done) {
it('is active with trailing slash on pattern', function (done) {
render((
<Router history={createHistory('/parent/child')}>
<Route path="/parent">
<Route path="child" />
</Route>
</Router>
), node, function () {
expect(this.router.isActive('/parent////child////')).toBe(true)
expect(this.router.isActive('/parent/child/')).toBe(true)
done()
})
})

it('is active with trailing slash on location', function (done) {
render((
<Router history={createHistory('/parent/child/')}>
<Route path="/parent">
<Route path="child" />
</Route>
</Router>
), node, function () {
expect(this.router.isActive('/parent/child')).toBe(true)
expect(this.router.isActive('/parent/child/')).toBe(true)
done()
})
})

it('is not active with extraneous slashes', function (done) {
render((
<Router history={createHistory('/parent/child')}>
<Route path="/parent">
<Route path="child" />
</Route>
</Router>
), node, function () {
expect(this.router.isActive('/parent//child')).toBe(false)
expect(this.router.isActive('/parent/child//')).toBe(false)
done()
})
})
Expand Down Expand Up @@ -336,7 +364,41 @@ describe('isActive', function () {
})
})

it('is active with extraneous slashes', function (done) {
it('is active with trailing slash on pattern', function (done) {
render((
<Router history={createHistory('/parent/child')}>
<Route path="/parent">
<Route path="child">
<IndexRoute />
</Route>
</Route>
</Router>
), node, function () {
expect(this.router.isActive('/parent/child/')).toBe(true)
expect(this.router.isActive('/parent/child/', true)).toBe(true)
done()
})
})

it('is active with trailing slash on location', function (done) {
render((
<Router history={createHistory('/parent/child/')}>
<Route path="/parent">
<Route path="child">
<IndexRoute />
</Route>
</Route>
</Router>
), node, function () {
expect(this.router.isActive('/parent/child')).toBe(true)
expect(this.router.isActive('/parent/child', true)).toBe(true)
expect(this.router.isActive('/parent/child/')).toBe(true)
expect(this.router.isActive('/parent/child/', true)).toBe(true)
done()
})
})

it('is not active with extraneous slashes', function (done) {
render((
<Router history={createHistory('/parent/child')}>
<Route path="/parent">
Expand All @@ -346,8 +408,10 @@ describe('isActive', function () {
</Route>
</Router>
), node, function () {
expect(this.router.isActive('/parent///child///')).toBe(true)
expect(this.router.isActive('/parent///child///', true)).toBe(true)
expect(this.router.isActive('/parent//child')).toBe(false)
expect(this.router.isActive('/parent/child//')).toBe(false)
expect(this.router.isActive('/parent//child', true)).toBe(false)
expect(this.router.isActive('/parent/child//', true)).toBe(false)
done()
})
})
Expand All @@ -372,7 +436,45 @@ describe('isActive', function () {
})
})

it('is active with extraneous slashes', function (done) {
it('is active with trailing slash on pattern', function (done) {
render((
<Router history={createHistory('/parent/child')}>
<Route path="/parent">
<Route path="child">
<Route>
<IndexRoute />
</Route>
</Route>
</Route>
</Router>
), node, function () {
expect(this.router.isActive('/parent/child/')).toBe(true)
expect(this.router.isActive('/parent/child/', true)).toBe(true)
done()
})
})

it('is active with trailing slash on location', function (done) {
render((
<Router history={createHistory('/parent/child/')}>
<Route path="/parent">
<Route path="child">
<Route>
<IndexRoute />
</Route>
</Route>
</Route>
</Router>
), node, function () {
expect(this.router.isActive('/parent/child')).toBe(true)
expect(this.router.isActive('/parent/child', true)).toBe(true)
expect(this.router.isActive('/parent/child/')).toBe(true)
expect(this.router.isActive('/parent/child/', true)).toBe(true)
done()
})
})

it('is not active with extraneous slashes', function (done) {
render((
<Router history={createHistory('/parent/child')}>
<Route path="/parent">
Expand All @@ -384,8 +486,10 @@ describe('isActive', function () {
</Route>
</Router>
), node, function () {
expect(this.router.isActive('/parent///child///')).toBe(true)
expect(this.router.isActive('/parent///child///', true)).toBe(true)
expect(this.router.isActive('/parent//child')).toBe(false)
expect(this.router.isActive('/parent/child//')).toBe(false)
expect(this.router.isActive('/parent//child', true)).toBe(false)
expect(this.router.isActive('/parent/child//', true)).toBe(false)
done()
})
})
Expand Down
2 changes: 1 addition & 1 deletion modules/__tests__/matchPattern-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ describe('matchPattern', function () {
}

it('works without params', function () {
assertMatch('/', '/path', 'path', [], [])
assertMatch('/', '/path', '/path', [], [])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case covers a purely internal function – matchRoutes is not part of our public API.

})

it('works with named params', function () {
Expand Down