Skip to content

Commit ebd9cf5

Browse files
ryanflorenceryanflorence
committed
Merge pull request #2434 from taion/index-route
[fixed] Index routes with extraneous slashes
2 parents 7c129f5 + 437fd29 commit ebd9cf5

File tree

6 files changed

+225
-27
lines changed

6 files changed

+225
-27
lines changed

‎modules/PatternUtils.js‎

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ export function matchPattern(pattern, pathname) {
9292

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

95-
regexpSource += '/*' // Ignore trailing slashes
95+
regexpSource += '/*' // Capture path separators
9696

9797
// Special-case patterns like '*' for catch-all routes.
9898
const captureRemaining = tokens[tokens.length - 1] !== '*'
@@ -106,28 +106,27 @@ export function matchPattern(pattern, pathname) {
106106

107107
let remainingPathname, paramValues
108108
if (match != null) {
109-
let matchedPath
110109
if (captureRemaining) {
111110
remainingPathname = match.pop()
112-
matchedPath =
111+
const matchedPath =
113112
match[0].substr(0, match[0].length - remainingPathname.length)
114-
} else {
115-
// If this matched at all, then the match was the entire pathname.
116-
matchedPath = match[0]
117-
remainingPathname = ''
118-
}
119113

120-
// Ensure we actually match at a path boundary.
121-
if (remainingPathname && remainingPathname.charAt(0) !== '/') {
122-
// This depends on the leading slash getting added to pathname above to
123-
// work in all cases.
124-
if (!matchedPath || matchedPath.charAt(matchedPath.length - 1) !== '/') {
114+
// If we didn't match the entire pathname, then make sure that the match
115+
// we did get ends at a path separator (potentially the one we added
116+
// above at the beginning of the path, if the actual match was empty).
117+
if (
118+
remainingPathname &&
119+
matchedPath.charAt(matchedPath.length - 1) !== '/'
120+
) {
125121
return {
126122
remainingPathname: null,
127123
paramNames,
128124
paramValues: null
129125
}
130126
}
127+
} else {
128+
// If this matched at all, then the match was the entire pathname.
129+
remainingPathname = ''
131130
}
132131

133132
paramValues = match.slice(1).map(

‎modules/PropTypes.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ import { PropTypes } from 'react'
33
const { func, object, arrayOf, oneOfType, element, shape, string } = PropTypes
44

55
export function falsy(props, propName, componentName) {
6-
/* istanbul ignore if: sanity check */
76
if (props[propName])
87
return new Error(`<${componentName}> should not have a "${propName}" prop`)
98
}

‎modules/__tests__/isActive-test.js‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,70 @@ describe('isActive', function () {
7272
})
7373
})
7474

75+
describe('nested routes', function () {
76+
describe('on the child', function () {
77+
it('is active for the child and the parent', function (done) {
78+
render((
79+
<Router history={createHistory('/parent/child')}>
80+
<Route path="/parent">
81+
<Route path="child" />
82+
</Route>
83+
</Router>
84+
), node, function () {
85+
expect(this.history.isActive('/parent/child')).toBe(true)
86+
expect(this.history.isActive('/parent/child', null, true)).toBe(true)
87+
expect(this.history.isActive('/parent')).toBe(true)
88+
expect(this.history.isActive('/parent', null, true)).toBe(false)
89+
done()
90+
})
91+
})
92+
93+
it('is active with extraneous slashes', function (done) {
94+
render((
95+
<Router history={createHistory('/parent/child')}>
96+
<Route path="/parent">
97+
<Route path="child" />
98+
</Route>
99+
</Router>
100+
), node, function () {
101+
expect(this.history.isActive('/parent////child////')).toBe(true)
102+
done()
103+
})
104+
})
105+
106+
it('is not active with missing slashes', function (done) {
107+
render((
108+
<Router history={createHistory('/parent/child')}>
109+
<Route path="/parent">
110+
<Route path="child" />
111+
</Route>
112+
</Router>
113+
), node, function () {
114+
expect(this.history.isActive('/parentchild')).toBe(false)
115+
done()
116+
})
117+
})
118+
})
119+
120+
describe('on the parent', function () {
121+
it('is active for the parent', function (done) {
122+
render((
123+
<Router history={createHistory('/parent')}>
124+
<Route path="/parent">
125+
<Route path="child" />
126+
</Route>
127+
</Router>
128+
), node, function () {
129+
expect(this.history.isActive('/parent/child')).toBe(false)
130+
expect(this.history.isActive('/parent/child', null, true)).toBe(false)
131+
expect(this.history.isActive('/parent')).toBe(true)
132+
expect(this.history.isActive('/parent', null, true)).toBe(true)
133+
done()
134+
})
135+
})
136+
})
137+
})
138+
75139
describe('a pathname that matches a parent route, but not the URL directly', function () {
76140
describe('with no query', function () {
77141
it('is active', function (done) {
@@ -188,6 +252,96 @@ describe('isActive', function () {
188252
})
189253
})
190254
})
255+
256+
describe('with the index route nested under a pathless route', function () {
257+
it('is active', function (done) {
258+
render((
259+
<Router history={createHistory('/home')}>
260+
<Route path="/home">
261+
<Route>
262+
<IndexRoute />
263+
</Route>
264+
</Route>
265+
</Router>
266+
), node, function () {
267+
expect(this.history.isActive('/home', null)).toBe(true)
268+
expect(this.history.isActive('/home', null, true)).toBe(true)
269+
done()
270+
})
271+
})
272+
})
273+
274+
describe('with a nested index route', function () {
275+
it('is active', function (done) {
276+
render((
277+
<Router history={createHistory('/parent/child')}>
278+
<Route path="/parent">
279+
<Route path="child">
280+
<IndexRoute />
281+
</Route>
282+
</Route>
283+
</Router>
284+
), node, function () {
285+
expect(this.history.isActive('/parent/child', null)).toBe(true)
286+
expect(this.history.isActive('/parent/child', null, true)).toBe(true)
287+
done()
288+
})
289+
})
290+
291+
it('is active with extraneous slashes', function (done) {
292+
render((
293+
<Router history={createHistory('/parent/child')}>
294+
<Route path="/parent">
295+
<Route path="child">
296+
<IndexRoute />
297+
</Route>
298+
</Route>
299+
</Router>
300+
), node, function () {
301+
expect(this.history.isActive('/parent///child///', null)).toBe(true)
302+
expect(this.history.isActive('/parent///child///', null, true)).toBe(true)
303+
done()
304+
})
305+
})
306+
})
307+
308+
describe('with a nested index route under a pathless route', function () {
309+
it('is active', function (done) {
310+
render((
311+
<Router history={createHistory('/parent/child')}>
312+
<Route path="/parent">
313+
<Route path="child">
314+
<Route>
315+
<IndexRoute />
316+
</Route>
317+
</Route>
318+
</Route>
319+
</Router>
320+
), node, function () {
321+
expect(this.history.isActive('/parent/child', null)).toBe(true)
322+
expect(this.history.isActive('/parent/child', null, true)).toBe(true)
323+
done()
324+
})
325+
})
326+
327+
it('is active with extraneous slashes', function (done) {
328+
render((
329+
<Router history={createHistory('/parent/child')}>
330+
<Route path="/parent">
331+
<Route path="child">
332+
<Route>
333+
<IndexRoute />
334+
</Route>
335+
</Route>
336+
</Route>
337+
</Router>
338+
), node, function () {
339+
expect(this.history.isActive('/parent///child///', null)).toBe(true)
340+
expect(this.history.isActive('/parent///child///', null, true)).toBe(true)
341+
done()
342+
})
343+
})
344+
})
191345
})
192346

193347
describe('a pathname that matches only the beginning of the URL', function () {

‎modules/__tests__/matchRoutes-test.js‎

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,9 @@ import { createMemoryHistory } from 'history'
44
import { createRoutes } from '../RouteUtils'
55
import matchRoutes from '../matchRoutes'
66
import Route from '../Route'
7+
import IndexRoute from '../IndexRoute'
78

89
describe('matchRoutes', function () {
9-
1010
let routes
1111
let
1212
RootRoute, UsersRoute, UsersIndexRoute, UserRoute, PostRoute, FilesRoute,
@@ -331,4 +331,33 @@ describe('matchRoutes', function () {
331331
})
332332
})
333333

334+
describe('invalid route configs', function () {
335+
let invalidRoutes, errorSpy
336+
337+
beforeEach(function () {
338+
errorSpy = expect.spyOn(console, 'error')
339+
})
340+
341+
afterEach(function () {
342+
errorSpy.restore()
343+
})
344+
345+
describe('index route with path', function () {
346+
beforeEach(function () {
347+
invalidRoutes = createRoutes(
348+
<Route path="/">
349+
<IndexRoute path="foo" />
350+
</Route>
351+
)
352+
})
353+
354+
it('complains when matching', function (done) {
355+
matchRoutes(invalidRoutes, createLocation('/'), function (error, match) {
356+
expect(match).toExist()
357+
expect(errorSpy).toHaveBeenCalledWith('Warning: Index routes should not have paths')
358+
done()
359+
})
360+
})
361+
})
362+
})
334363
})

‎modules/isActive.js‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -43,13 +43,12 @@ function paramsAreActive(paramNames, paramValues, activeParams) {
4343
})
4444
}
4545

46-
function getMatchingRoute(pathname, activeRoutes, activeParams) {
47-
let route, pattern
46+
function getMatchingRouteIndex(pathname, activeRoutes, activeParams) {
4847
let remainingPathname = pathname, paramNames = [], paramValues = []
4948

5049
for (let i = 0, len = activeRoutes.length; i < len; ++i) {
51-
route = activeRoutes[i]
52-
pattern = route.path || ''
50+
const route = activeRoutes[i]
51+
const pattern = route.path || ''
5352

5453
if (pattern.charAt(0) === '/') {
5554
remainingPathname = pathname
@@ -69,7 +68,7 @@ function getMatchingRoute(pathname, activeRoutes, activeParams) {
6968
route.path &&
7069
paramsAreActive(paramNames, paramValues, activeParams)
7170
)
72-
return route
71+
return i
7372
}
7473

7574
return null
@@ -79,12 +78,20 @@ function getMatchingRoute(pathname, activeRoutes, activeParams) {
7978
* Returns true if the given pathname matches the active routes
8079
* and params.
8180
*/
82-
function routeIsActive(pathname, location, routes, params, indexOnly) {
83-
if (indexOnly) {
84-
return location.pathname.replace(/\/*$/) === pathname.replace(/\/*$/)
81+
function routeIsActive(pathname, routes, params, indexOnly) {
82+
const i = getMatchingRouteIndex(pathname, routes, params)
83+
84+
if (i === null) {
85+
// No match.
86+
return false
87+
} else if (!indexOnly) {
88+
// Any match is good enough.
89+
return true
8590
}
8691

87-
return getMatchingRoute(pathname, routes, params) != null
92+
// If any remaining routes past the match index have paths, then we can't
93+
// be on the index route.
94+
return routes.slice(i + 1).every(route => !route.path)
8895
}
8996

9097
/**
@@ -109,7 +116,7 @@ function isActive(pathname, query, indexOnly, location, routes, params) {
109116
if (location == null)
110117
return false
111118

112-
if (!routeIsActive(pathname, location, routes, params, indexOnly))
119+
if (!routeIsActive(pathname, routes, params, indexOnly))
113120
return false
114121

115122
return queryIsActive(query, location.query)

‎modules/matchRoutes.js‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import warning from 'warning'
12
import { loopAsync } from './AsyncUtils'
23
import { matchPattern } from './PatternUtils'
34
import { createRoutes } from './RouteUtils'
@@ -90,10 +91,19 @@ function matchRouteDeep(
9091
if (error) {
9192
callback(error)
9293
} else {
93-
if (Array.isArray(indexRoute))
94+
if (Array.isArray(indexRoute)) {
95+
warning(
96+
indexRoute.every(route => !route.path),
97+
'Index routes should not have paths'
98+
)
9499
match.routes.push(...indexRoute)
95-
else if (indexRoute)
100+
} else if (indexRoute) {
101+
warning(
102+
!indexRoute.path,
103+
'Index routes should not have paths'
104+
)
96105
match.routes.push(indexRoute)
106+
}
97107

98108
callback(null, match)
99109
}

0 commit comments

Comments
 (0)