From 5db89071d408c2e5b2e26b3d7b00e02e164c197f Mon Sep 17 00:00:00 2001 From: Brian Ford Date: Fri, 15 May 2015 02:05:57 -0700 Subject: [PATCH] fix(router): improve route matching priorities --- modules/angular2/src/router/instruction.js | 25 ++- .../angular2/src/router/path_recognizer.js | 27 +++- .../angular2/src/router/route_recognizer.js | 63 ++++++-- modules/angular2/src/router/route_registry.js | 142 ++++++++++-------- modules/angular2/src/router/router.js | 2 +- .../test/router/route_recognizer_spec.js | 81 +++++----- .../test/router/route_registry_spec.js | 27 ++++ 7 files changed, 240 insertions(+), 127 deletions(-) diff --git a/modules/angular2/src/router/instruction.js b/modules/angular2/src/router/instruction.js index 3c7d98559b..a8243c6aa2 100644 --- a/modules/angular2/src/router/instruction.js +++ b/modules/angular2/src/router/instruction.js @@ -14,6 +14,9 @@ export class RouteParams { } } +/** + * An `Instruction` represents the component hierarchy of the application based on a given route + */ export class Instruction { component:any; _children:Map; @@ -24,21 +27,21 @@ export class Instruction { // the part of the URL captured by this instruction and all children accumulatedUrl:string; - params:Map; + params:StringMap; reuse:boolean; - cost:number; + specificity:number; - constructor({params, component, children, matchedUrl, parentCost}:{params:StringMap, component:any, children:StringMap, matchedUrl:string, cost:number} = {}) { + constructor({params, component, children, matchedUrl, parentSpecificity}:{params:StringMap, component:any, children:Map, matchedUrl:string, parentSpecificity:number} = {}) { this.reuse = false; this.capturedUrl = matchedUrl; this.accumulatedUrl = matchedUrl; - this.cost = parentCost; + this.specificity = parentSpecificity; if (isPresent(children)) { this._children = children; var childUrl; StringMapWrapper.forEach(this._children, (child, _) => { childUrl = child.accumulatedUrl; - this.cost += child.cost; + this.specificity += child.specificity; }); if (isPresent(childUrl)) { this.accumulatedUrl += childUrl; @@ -50,14 +53,20 @@ export class Instruction { this.params = params; } - hasChild(outletName:string):Instruction { + hasChild(outletName:string):boolean { return StringMapWrapper.contains(this._children, outletName); } + /** + * Returns the child instruction with the given outlet name + */ getChild(outletName:string):Instruction { return StringMapWrapper.get(this._children, outletName); } + /** + * (child:Instruction, outletName:string) => {} + */ forEachChild(fn:Function): void { StringMapWrapper.forEach(this._children, fn); } @@ -65,7 +74,7 @@ export class Instruction { /** * Does a synchronous, breadth-first traversal of the graph of instructions. * Takes a function with signature: - * (parent:Instruction, child:Instruction) => {} + * (child:Instruction, outletName:string) => {} */ traverseSync(fn:Function): void { this.forEachChild(fn); @@ -74,7 +83,7 @@ export class Instruction { /** - * Takes a currently active instruction and sets a reuse flag on this instruction + * Takes a currently active instruction and sets a reuse flag on each of this instruction's children */ reuseComponentsFrom(oldInstruction:Instruction): void { this.traverseSync((childInstruction, outletName) => { diff --git a/modules/angular2/src/router/path_recognizer.js b/modules/angular2/src/router/path_recognizer.js index 12a26c8f41..cc06405a7c 100644 --- a/modules/angular2/src/router/path_recognizer.js +++ b/modules/angular2/src/router/path_recognizer.js @@ -62,7 +62,17 @@ function parsePathString(route:string) { var segments = splitBySlash(route); var results = ListWrapper.create(); - var cost = 0; + var specificity = 0; + + // The "specificity" of a path is used to determine which route is used when multiple routes match a URL. + // Static segments (like "/foo") are the most specific, followed by dynamic segments (like "/:id"). Star segments + // add no specificity. Segments at the start of the path are more specific than proceeding ones. + // The code below uses place values to combine the different types of segments into a single integer that we can + // sort later. Each static segment is worth hundreds of points of specificity (10000, 9900, ..., 200), and each + // dynamic segment is worth single points of specificity (100, 99, ... 2). + if (segments.length > 98) { + throw new BaseException(`'${route}' has more than the maximum supported number of segments.`); + } for (var i=0; i 0) { ListWrapper.push(results, new StaticSegment(segment)); - cost += 1; + specificity += 100 * (100 - i); } } - return {segments: results, cost}; + return {segments: results, specificity}; } function splitBySlash (url:string):List { @@ -93,16 +102,18 @@ export class PathRecognizer { segments:List; regex:RegExp; handler:any; - cost:number; + specificity:number; + path:string; constructor(path:string, handler:any) { + this.path = path; this.handler = handler; this.segments = []; // TODO: use destructuring assignment // see https://github.com/angular/ts2dart/issues/158 var parsed = parsePathString(path); - var cost = parsed['cost']; + var specificity = parsed['specificity']; var segments = parsed['segments']; var regexString = '^'; @@ -112,7 +123,7 @@ export class PathRecognizer { this.regex = RegExpWrapper.create(regexString); this.segments = segments; - this.cost = cost; + this.specificity = specificity; } parseParams(url:string):StringMap { diff --git a/modules/angular2/src/router/route_recognizer.js b/modules/angular2/src/router/route_recognizer.js index a8990c52b0..d9b2475ffe 100644 --- a/modules/angular2/src/router/route_recognizer.js +++ b/modules/angular2/src/router/route_recognizer.js @@ -1,8 +1,12 @@ -import {RegExp, RegExpWrapper, StringWrapper, isPresent} from 'angular2/src/facade/lang'; +import {RegExp, RegExpWrapper, StringWrapper, isPresent, BaseException} from 'angular2/src/facade/lang'; import {Map, MapWrapper, List, ListWrapper, StringMap, StringMapWrapper} from 'angular2/src/facade/collection'; import {PathRecognizer} from './path_recognizer'; +/** + * `RouteRecognizer` is responsible for recognizing routes for a single component. + * It is consumed by `RouteRegistry`, which knows how to recognize an entire hierarchy of components. + */ export class RouteRecognizer { names:Map; redirects:Map; @@ -20,14 +24,25 @@ export class RouteRecognizer { addConfig(path:string, handler:any, alias:string = null): void { var recognizer = new PathRecognizer(path, handler); + MapWrapper.forEach(this.matchers, (matcher, _) => { + if (recognizer.regex.toString() == matcher.regex.toString()) { + throw new BaseException(`Configuration '${path}' conflicts with existing route '${matcher.path}'`); + } + }); MapWrapper.set(this.matchers, recognizer.regex, recognizer); if (isPresent(alias)) { MapWrapper.set(this.names, alias, recognizer); } } - recognize(url:string):List { - var solutions = []; + + /** + * Given a URL, returns a list of `RouteMatch`es, which are partial recognitions for some route. + * + */ + recognize(url:string):List { + var solutions = ListWrapper.create(); + MapWrapper.forEach(this.redirects, (target, path) => { //TODO: "/" redirect case if (StringWrapper.startsWith(url, path)) { @@ -38,21 +53,20 @@ export class RouteRecognizer { MapWrapper.forEach(this.matchers, (pathRecognizer, regex) => { var match; if (isPresent(match = RegExpWrapper.firstMatch(regex, url))) { - var solution = StringMapWrapper.create(); - StringMapWrapper.set(solution, 'cost', pathRecognizer.cost); - StringMapWrapper.set(solution, 'handler', pathRecognizer.handler); - StringMapWrapper.set(solution, 'params', pathRecognizer.parseParams(url)); - //TODO(btford): determine a good generic way to deal with terminal matches - if (url == '/') { - StringMapWrapper.set(solution, 'matchedUrl', '/'); - StringMapWrapper.set(solution, 'unmatchedUrl', ''); - } else { - StringMapWrapper.set(solution, 'matchedUrl', match[0]); - var unmatchedUrl = StringWrapper.substring(url, match[0].length); - StringMapWrapper.set(solution, 'unmatchedUrl', unmatchedUrl); + var matchedUrl = '/'; + var unmatchedUrl = ''; + if (url != '/') { + matchedUrl = match[0]; + unmatchedUrl = StringWrapper.substring(url, match[0].length); } - ListWrapper.push(solutions, solution); + ListWrapper.push(solutions, new RouteMatch({ + specificity: pathRecognizer.specificity, + handler: pathRecognizer.handler, + params: pathRecognizer.parseParams(url), + matchedUrl: matchedUrl, + unmatchedUrl: unmatchedUrl + })); } }); @@ -68,3 +82,20 @@ export class RouteRecognizer { return isPresent(pathRecognizer) ? pathRecognizer.generate(params) : null; } } + +export class RouteMatch { + specificity:number; + handler:StringMap; + params:StringMap; + matchedUrl:string; + unmatchedUrl:string; + constructor({specificity, handler, params, matchedUrl, unmatchedUrl}: + {specificity:number, handler:StringMap, params:StringMap, matchedUrl:string, unmatchedUrl:string} = {}) { + + this.specificity = specificity; + this.handler = handler; + this.params = params; + this.matchedUrl = matchedUrl; + this.unmatchedUrl = unmatchedUrl; + } +} diff --git a/modules/angular2/src/router/route_registry.js b/modules/angular2/src/router/route_registry.js index ae9f426e74..1b4548c021 100644 --- a/modules/angular2/src/router/route_registry.js +++ b/modules/angular2/src/router/route_registry.js @@ -1,10 +1,14 @@ -import {RouteRecognizer} from './route_recognizer'; +import {RouteRecognizer, RouteMatch} from './route_recognizer'; import {Instruction, noopInstruction} from './instruction'; import {List, ListWrapper, Map, MapWrapper, StringMap, StringMapWrapper} from 'angular2/src/facade/collection'; import {isPresent, isBlank, isType, StringWrapper, BaseException} from 'angular2/src/facade/lang'; import {RouteConfig} from './route_config_impl'; import {reflector} from 'angular2/src/reflection/reflection'; +/** + * The RouteRegistry holds route configurations for each component in an Angular app. + * It is responsible for creating Instructions from URLs, and generating URLs based on route and parameters. + */ export class RouteRegistry { _rules:Map; @@ -12,7 +16,10 @@ export class RouteRegistry { this._rules = MapWrapper.create(); } - config(parentComponent, config: StringMap): void { + /** + * Given a component and a configuration object, add the route to this registry + */ + config(parentComponent, config:StringMap): void { if (!StringMapWrapper.contains(config, 'path')) { throw new BaseException('Route config does not contain "path"'); } @@ -33,18 +40,19 @@ export class RouteRegistry { config = normalizeConfig(config); if (StringMapWrapper.contains(config, 'redirectTo')) { - recognizer.addRedirect(StringMapWrapper.get(config, 'path'), StringMapWrapper.get(config, 'redirectTo')); + recognizer.addRedirect(config['path'], config['redirectTo']); return; } - var components = StringMapWrapper.get(config, 'components'); - StringMapWrapper.forEach(components, (component, _) => { - this.configFromComponent(component); - }); + var components = config['components']; + StringMapWrapper.forEach(components, (component, _) => this.configFromComponent(component)); recognizer.addConfig(config['path'], config, config['as']); } + /** + * Reads the annotations of a component and configures the registry based on them + */ configFromComponent(component): void { if (!isType(component)) { return; @@ -61,70 +69,78 @@ export class RouteRegistry { var annotation = annotations[i]; if (annotation instanceof RouteConfig) { - ListWrapper.forEach(annotation.configs, (config) => { - this.config(component, config); - }) + ListWrapper.forEach(annotation.configs, (config) => this.config(component, config)); } } } } + /** + * Given a URL and a parent component, return the most specific instruction for navigating + * the application into the state specified by the + */ recognize(url:string, parentComponent): Instruction { var componentRecognizer = MapWrapper.get(this._rules, parentComponent); if (isBlank(componentRecognizer)) { return null; } - var componentSolutions = componentRecognizer.recognize(url); - var fullSolutions = []; + // Matches some beginning part of the given URL + var possibleMatches = componentRecognizer.recognize(url); - for(var i = 0; i < componentSolutions.length; i++) { - var candidate = componentSolutions[i]; - if (candidate['unmatchedUrl'].length == 0) { - ListWrapper.push(fullSolutions, handlerToLeafInstructions(candidate, parentComponent)); + // A list of instructions that captures all of the given URL + var fullSolutions = ListWrapper.create(); + + for (var i = 0; i < possibleMatches.length; i++) { + var candidate : RouteMatch = possibleMatches[i]; + + // if the candidate captures all of the URL, add it to our list of solutions + if (candidate.unmatchedUrl.length == 0) { + ListWrapper.push(fullSolutions, routeMatchToInstruction(candidate, parentComponent)); } else { + + // otherwise, recursively match the remaining part of the URL against the component's children var children = StringMapWrapper.create(), - allMapped = true; + allChildrenMatch = true, + components = StringMapWrapper.get(candidate.handler, 'components'); - var components = candidate['handler']['components']; var componentNames = StringMapWrapper.keys(components); + for (var nameIndex = 0; nameIndex < componentNames.length; nameIndex++) { + var name = componentNames[nameIndex]; + var component = StringMapWrapper.get(components, name); - for (var cmpIndex = 0; cmpIndex < componentNames.length; cmpIndex++) { - var name = componentNames[cmpIndex]; - var component = components[name]; - var childInstruction = this.recognize(candidate['unmatchedUrl'], component); + var childInstruction = this.recognize(candidate.unmatchedUrl, component); if (isPresent(childInstruction)) { - childInstruction.params = candidate['params']; + childInstruction.params = candidate.params; children[name] = childInstruction; } else { - allMapped = false; + allChildrenMatch = false; break; } } - if (allMapped) { + if (allChildrenMatch) { ListWrapper.push(fullSolutions, new Instruction({ component: parentComponent, children: children, - matchedUrl: candidate['matchedUrl'], - parentCost: candidate['cost'] + matchedUrl: candidate.matchedUrl, + parentSpecificity: candidate.specificity })); } } } if (fullSolutions.length > 0) { - var lowCost = fullSolutions[0].cost; - var lowIndex = 0; - for (var solIdx = 1; solIdx < fullSolutions.length; solIdx++) { - if (fullSolutions[solIdx].cost < lowCost) { - lowCost = fullSolutions[solIdx].cost; - lowIndex = solIdx; + var mostSpecificSolution = fullSolutions[0]; + for (var solutionIndex = 1; solutionIndex < fullSolutions.length; solutionIndex++) { + var solution = fullSolutions[solutionIndex]; + if (solution.specificity > mostSpecificSolution.specificity) { + mostSpecificSolution = solution; } } - return fullSolutions[lowIndex]; + return mostSpecificSolution; } return null; @@ -137,43 +153,49 @@ export class RouteRegistry { } } -function handlerToLeafInstructions(context, parentComponent): Instruction { +function routeMatchToInstruction(routeMatch:RouteMatch, parentComponent): Instruction { var children = StringMapWrapper.create(); - StringMapWrapper.forEach(context['handler']['components'], (component, outletName) => { + var components = StringMapWrapper.get(routeMatch.handler, 'components'); + StringMapWrapper.forEach(components, (component, outletName) => { children[outletName] = new Instruction({ component: component, - params: context['params'], - parentCost: 0 + params: routeMatch.params, + parentSpecificity: 0 }); }); return new Instruction({ component: parentComponent, children: children, - matchedUrl: context['matchedUrl'], - parentCost: context['cost'] + matchedUrl: routeMatch.matchedUrl, + parentSpecificity: routeMatch.specificity }); } -// given: -// { component: Foo } -// mutates the config to: -// { components: { default: Foo } } + +/* + * Given a config object: + * { 'component': Foo } + * Returns a new config object: + * { components: { default: Foo } } + * + * If the config object does not contain a `component` key, the original + * config object is returned. + */ function normalizeConfig(config:StringMap): StringMap { - if (StringMapWrapper.contains(config, 'component')) { - var component = StringMapWrapper.get(config, 'component'); - var components = StringMapWrapper.create(); - StringMapWrapper.set(components, 'default', component); - - var newConfig = StringMapWrapper.create(); - StringMapWrapper.set(newConfig, 'components', components); - - StringMapWrapper.forEach(config, (value, key) => { - if (!StringWrapper.equals(key, 'component') && !StringWrapper.equals(key, 'components')) { - StringMapWrapper.set(newConfig, key, value); - } - }); - - return newConfig; + if (!StringMapWrapper.contains(config, 'component')) { + return config; } - return config; + var newConfig = { + 'components': { + 'default': config['component'] + } + }; + + StringMapWrapper.forEach(config, (value, key) => { + if (key != 'component' && key != 'components') { + newConfig[key] = value; + } + }); + + return newConfig; } diff --git a/modules/angular2/src/router/router.js b/modules/angular2/src/router/router.js index 12c515cd6c..e8df794e32 100644 --- a/modules/angular2/src/router/router.js +++ b/modules/angular2/src/router/router.js @@ -37,7 +37,7 @@ export class Router { _pipeline:Pipeline; _registry:RouteRegistry; - _outlets:Map; + _outlets:Map; _subject:EventEmitter; diff --git a/modules/angular2/test/router/route_recognizer_spec.js b/modules/angular2/test/router/route_recognizer_spec.js index 96ddf24cda..f0198aa9d1 100644 --- a/modules/angular2/test/router/route_recognizer_spec.js +++ b/modules/angular2/test/router/route_recognizer_spec.js @@ -14,65 +14,78 @@ export function main() { var handler = { 'components': { 'a': 'b' } }; + var handler2 = { + 'components': { 'b': 'c' } + }; beforeEach(() => { recognizer = new RouteRecognizer(); }); - it('should work with a static segment', () => { + + it('should recognize a static segment', () => { recognizer.addConfig('/test', handler); - - expect(recognizer.recognize('/test')[0]).toEqual({ - 'cost' : 1, - 'handler': { 'components': { 'a': 'b' } }, - 'params': {}, - 'matchedUrl': '/test', - 'unmatchedUrl': '' - }); + expect(recognizer.recognize('/test')[0].handler).toEqual(handler); }); - it('should work with leading slash', () => { + + it('should recognize a single slash', () => { recognizer.addConfig('/', handler); - - expect(recognizer.recognize('/')[0]).toEqual({ - 'cost': 0, - 'handler': { 'components': { 'a': 'b' } }, - 'params': {}, - 'matchedUrl': '/', - 'unmatchedUrl': '' - }); + var solution = recognizer.recognize('/')[0]; + expect(solution.handler).toEqual(handler); }); - it('should work with a dynamic segment', () => { + + it('should recognize a dynamic segment', () => { recognizer.addConfig('/user/:name', handler); - expect(recognizer.recognize('/user/brian')[0]).toEqual({ - 'cost': 101, - 'handler': handler, - 'params': { 'name': 'brian' }, - 'matchedUrl': '/user/brian', - 'unmatchedUrl': '' - }); + var solution = recognizer.recognize('/user/brian')[0]; + expect(solution.handler).toEqual(handler); + expect(solution.params).toEqual({ 'name': 'brian' }); }); - it('should allow redirects', () => { + + it('should recognize a star segment', () => { + recognizer.addConfig('/first/*rest', handler); + var solution = recognizer.recognize('/first/second/third')[0]; + expect(solution.handler).toEqual(handler); + expect(solution.params).toEqual({ 'rest': 'second/third' }); + }); + + + it('should throw when given two routes that start with the same static segment', () => { + recognizer.addConfig('/hello', handler); + expect(() => recognizer.addConfig('/hello', handler2)).toThrowError( + 'Configuration \'/hello\' conflicts with existing route \'/hello\'' + ); + }); + + + it('should throw when given two routes that have dynamic segments in the same order', () => { + recognizer.addConfig('/hello/:person/how/:doyoudou', handler); + expect(() => recognizer.addConfig('/hello/:friend/how/:areyou', handler2)).toThrowError( + 'Configuration \'/hello/:friend/how/:areyou\' conflicts with existing route \'/hello/:person/how/:doyoudou\'' + ); + }); + + + it('should recognize redirects', () => { recognizer.addRedirect('/a', '/b'); recognizer.addConfig('/b', handler); var solutions = recognizer.recognize('/a'); expect(solutions.length).toBe(1); - expect(solutions[0]).toEqual({ - 'cost': 1, - 'handler': handler, - 'params': {}, - 'matchedUrl': '/b', - 'unmatchedUrl': '' - }); + + var solution = solutions[0]; + expect(solution.handler).toEqual(handler); + expect(solution.matchedUrl).toEqual('/b'); }); + it('should generate URLs', () => { recognizer.addConfig('/app/user/:name', handler, 'user'); expect(recognizer.generate('user', {'name' : 'misko'})).toEqual('/app/user/misko'); }); + it('should throw in the absence of required params URLs', () => { recognizer.addConfig('/app/user/:name', handler, 'user'); expect(() => recognizer.generate('user', {})).toThrowError( diff --git a/modules/angular2/test/router/route_registry_spec.js b/modules/angular2/test/router/route_registry_spec.js index 1b21bca264..72fe53bc86 100644 --- a/modules/angular2/test/router/route_registry_spec.js +++ b/modules/angular2/test/router/route_registry_spec.js @@ -45,6 +45,33 @@ export function main() { expect(instruction.getChild('default').component).toBe(DummyCompA); }); + it('should prefer routes with more dynamic segments', () => { + registry.config(rootHostComponent, {'path': '/:first/*rest', 'component': DummyCompA}); + registry.config(rootHostComponent, {'path': '/*all', 'component': DummyCompB}); + + var instruction = registry.recognize('/some/path', rootHostComponent); + + expect(instruction.getChild('default').component).toBe(DummyCompA); + }); + + it('should prefer routes with more static segments', () => { + registry.config(rootHostComponent, {'path': '/first/:second', 'component': DummyCompA}); + registry.config(rootHostComponent, {'path': '/:first/:second', 'component': DummyCompB}); + + var instruction = registry.recognize('/first/second', rootHostComponent); + + expect(instruction.getChild('default').component).toBe(DummyCompA); + }); + + it('should prefer routes with static segments before dynamic segments', () => { + registry.config(rootHostComponent, {'path': '/first/second/:third', 'component': DummyCompB}); + registry.config(rootHostComponent, {'path': '/first/:second/third', 'component': DummyCompA}); + + var instruction = registry.recognize('/first/second/third', rootHostComponent); + + expect(instruction.getChild('default').component).toBe(DummyCompB); + }); + it('should match the full URL recursively', () => { registry.config(rootHostComponent, {'path': '/first', 'component': DummyParentComp});