Browse code
Fix route overlap detection, refactor specificity calculation
Ed Langley authored on 16/06/2017 05:41:47
Showing 2 changed files
Showing 2 changed files
... | ... |
@@ -12,28 +12,16 @@ function mostSpecificRouteMatch(match1, match2) { |
12 | 12 |
|
13 | 13 |
const paramLength1 = match1.routeParams.length; |
14 | 14 |
const paramLength2 = match2.routeParams.length; |
15 |
+ let findWildcard = R.compose(R.findIndex.bind(R, isWildcard), pathSplit) |
|
15 | 16 |
|
16 |
- let result = null; |
|
17 |
+ let result = (paramLength1 > paramLength2) ? match2 : match1; |
|
17 | 18 |
|
18 | 19 |
if (paramLength1 === paramLength2) { |
19 |
- let path1Parts = pathSplit(match1.path); |
|
20 |
- let path2Parts = pathSplit(match2.path); |
|
21 |
- |
|
22 |
- for (let [segment1, segment2] of R.zip(path1Parts, path2Parts)) { |
|
23 |
- if (isWildcard(segment1)) { |
|
24 |
- result = match2; |
|
25 |
- } else if (isWildcard(segment2)) { |
|
26 |
- result = match1; |
|
27 |
- } |
|
28 | 20 |
|
29 |
- if (result !== null) { |
|
30 |
- break; |
|
31 |
- } |
|
32 |
- } |
|
33 |
- } else if (paramLength1 > paramLength2) { |
|
34 |
- result = match2; |
|
35 |
- } else if (paramLength2 > paramLength1) { |
|
36 |
- result = match1; |
|
21 |
+ let path1WildcardIdx = findWildcard(match1.path); |
|
22 |
+ let path2WildcardIdx = findWildcard(match2.path); |
|
23 |
+ |
|
24 |
+ result = (path1WildcardIdx !== -1 && path1WildcardIdx < path2WildcardIdx) ? match2 : match1; |
|
37 | 25 |
} |
38 | 26 |
|
39 | 27 |
if (result === null) { |
... | ... |
@@ -210,6 +198,35 @@ function getRouteByPath(pattern, matchers) { |
210 | 198 |
return matchers.compiledRouteMatchers[pattern]; |
211 | 199 |
} |
212 | 200 |
|
201 |
+function normalizeWildcards(path) { |
|
202 |
+ let curIdx = 0; |
|
203 |
+ return path.map((el) => { |
|
204 |
+ if (isWildcard(el)) { |
|
205 |
+ return `:wildcard${curIdx}` |
|
206 |
+ } else { |
|
207 |
+ return el; |
|
208 |
+ } |
|
209 |
+ }); |
|
210 |
+} |
|
211 |
+ |
|
212 |
+function routeAlreadyExists(compiledRouteMatchers, path) { |
|
213 |
+ let result = compiledRouteMatchers.hasOwnProperty(path) |
|
214 |
+ |
|
215 |
+ if (!result) { |
|
216 |
+ const normalizingSplit = R.compose(normalizeWildcards, pathSplit); |
|
217 |
+ const pathParts = normalizingSplit(path); |
|
218 |
+ |
|
219 |
+ for (const otherPath of Object.keys(compiledRouteMatchers)) { |
|
220 |
+ const otherPathParts = normalizingSplit(otherPath); |
|
221 |
+ if (R.equals(pathParts, otherPathParts)) { |
|
222 |
+ throw new Error(`invalid routing configuration — route ${path} overlaps with route ${otherPath}`) |
|
223 |
+ } |
|
224 |
+ } |
|
225 |
+ } |
|
226 |
+ |
|
227 |
+ return result; |
|
228 |
+} |
|
229 |
+ |
|
213 | 230 |
function compileRoutes(routesConfig) { |
214 | 231 |
let compiledActionMatchers = {}; |
215 | 232 |
let compiledRouteMatchers = {}; |
... | ... |
@@ -227,7 +244,7 @@ function compileRoutes(routesConfig) { |
227 | 244 |
const route = makeRoute(path, action, extraParams); |
228 | 245 |
compiledActionMatchers[action].push(route); |
229 | 246 |
|
230 |
- if (compiledRouteMatchers.hasOwnProperty(path)) { |
|
247 |
+ if (routeAlreadyExists(compiledRouteMatchers, path)) { |
|
231 | 248 |
throw new Error("overlapping paths"); |
232 | 249 |
} |
233 | 250 |
|
... | ... |
@@ -188,19 +188,44 @@ it("router should match non-wildcard route in preference to wildcard route", () |
188 | 188 |
expect(actionsDispatched()).toEqual([{type: 'ACTION_NAME', id: 1}]); |
189 | 189 |
}); |
190 | 190 |
|
191 |
+it("router should throw on duplicate paths", () => { |
|
192 |
+ // given |
|
193 |
+ const routesConfig = [ |
|
194 |
+ ['/somewhere/:id', 'ACTION_NAME', {}], |
|
195 |
+ ["/somewhere/:id", 'ACTION_NAME', {}], |
|
196 |
+ ]; |
|
197 |
+ |
|
198 |
+ expect( () => { |
|
199 |
+ setupTest(routesConfig); |
|
200 |
+ }).toThrow(); |
|
201 |
+}); |
|
202 |
+ |
|
203 |
+it("router should throw on equally specific routes", () => { |
|
204 |
+ // given |
|
205 |
+ const routesConfig = [ |
|
206 |
+ ['/somewhere/:id', 'ACTION_NAME', {}], |
|
207 |
+ ["/somewhere/:specific", 'ACTION_NAME', {}], |
|
208 |
+ ]; |
|
209 |
+ |
|
210 |
+ expect( () => { |
|
211 |
+ const {actionsDispatched, fireUrlChange} = setupTest(routesConfig); |
|
212 |
+ fireUrlChange('/somewhere/specific'); |
|
213 |
+ }).toThrow(); |
|
214 |
+}); |
|
215 |
+ |
|
191 | 216 |
it("router should match less-wildcarded routes in preference to more wildcarded routes", () => { |
192 | 217 |
//given |
193 | 218 |
const routesConfig = [ |
194 |
- ["/somewhere/specific/:view", "ACTION_NAME", {id: 1}], |
|
195 |
- ["/somewhere/:id/:view", "ACTION_NAME", {}], |
|
219 |
+ ["/somewhere/:id/:view/:bar", "ACTION_NAME", {}], |
|
220 |
+ ["/somewhere/:foo/:id/:view/:baz", "ACTION_NAME", {}], |
|
196 | 221 |
]; |
197 | 222 |
const {actionsDispatched, fireUrlChange} = setupTest(routesConfig); |
198 | 223 |
|
199 | 224 |
// when |
200 |
- fireUrlChange('/somewhere/specific/etc'); |
|
225 |
+ fireUrlChange('/somewhere/specific/etc/bar'); |
|
201 | 226 |
|
202 | 227 |
// then |
203 |
- expect(actionsDispatched()).toEqual([{type:'ACTION_NAME', id: 1, view: "etc"}]); |
|
228 |
+ expect(actionsDispatched()).toEqual([{type:'ACTION_NAME', id: "specific", view: "etc", bar: "bar"}]); |
|
204 | 229 |
|
205 | 230 |
}); |
206 | 231 |
|