Skip to content

Commit

Permalink
Merge pull request #3158 from taion/isActive-extraneous-slashes
Browse files Browse the repository at this point in the history 
Support trailing slashes, not extraneous ones
  • Loading branch information
@timdorr
timdorr committed Apr 1, 2016
2 parents 1267766 + 9cd747f commit 7d04245
Show file tree
Hide file tree
Showing 5 changed files with 141 additions and 41 deletions.
42 changes: 19 additions & 23 deletions modules/PatternUtils.js
View file
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
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' }}
activeClassName="active"
>
Ryan
Expand Down
16 changes: 8 additions & 8 deletions modules/__tests__/_bc-isActive-test.js
View file
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
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
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', [], [])
})

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

0 comments on commit 7d04245

Please sign in to comment.