fix(router): improve route matching priorities

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<string, Instruction>;
@@ -24,21 +27,21 @@ export class Instruction {
   // the part of the URL captured by this instruction and all children
   accumulatedUrl:string;
 
-  params:Map<string, string>;
+  params:StringMap<string, string>;
   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) => {

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<segments.length; i++) {
     var segment = segments[i],
@@ -70,17 +80,16 @@ function parsePathString(route:string) {
 
     if (isPresent(match = RegExpWrapper.firstMatch(paramMatcher, segment))) {
       ListWrapper.push(results, new DynamicSegment(match[1]));
-      cost += 100;
+      specificity += (100 - i);
     } else if (isPresent(match = RegExpWrapper.firstMatch(wildcardMatcher, segment))) {
       ListWrapper.push(results, new StarSegment(match[1]));
-      cost += 10000;
     } else if (segment.length > 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<string> {
@@ -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<string, string> {

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<string, PathRecognizer>;
   redirects:Map<string, string>;
@@ -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<StringMap> {
+
-    var solutions = [];
+  /**
+   * Given a URL, returns a list of `RouteMatch`es, which are partial recognitions for some route.
+   *
+   */
+  recognize(url:string):List<RouteMatch> {
+    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 == '/') {
+        var matchedUrl = '/';
-          StringMapWrapper.set(solution, 'matchedUrl', '/');
+        var unmatchedUrl = '';
-          StringMapWrapper.set(solution, 'unmatchedUrl', '');
+        if (url != '/') {
-        } else {
+          matchedUrl = match[0];
-          StringMapWrapper.set(solution, 'matchedUrl', match[0]);
+          unmatchedUrl = StringWrapper.substring(url, match[0].length);
-          var unmatchedUrl = StringWrapper.substring(url, match[0].length);
-          StringMapWrapper.set(solution, 'unmatchedUrl', unmatchedUrl);
         }
-        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<string, any>;
+  params:StringMap<string, string>;
+  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;
+  }
+}

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<any, RouteRecognizer>;
 
@@ -12,6 +16,9 @@ export class RouteRegistry {
     this._rules = MapWrapper.create();
   }
 
+  /**
+   * Given a component and a configuration object, add the route to this registry
+   */
   config(parentComponent, config:StringMap<string, any>): 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');
+    var components = config['components'];
-    StringMapWrapper.forEach(components, (component, _) => {
+    StringMapWrapper.forEach(components, (component, _) => this.configFromComponent(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) => {
+          ListWrapper.forEach(annotation.configs, (config) => this.config(component, 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);
+    // Matches some beginning part of the given URL
-    var fullSolutions = [];
+    var possibleMatches = componentRecognizer.recognize(url);
 
-    for(var i = 0; i < componentSolutions.length; i++) {
+    // A list of instructions that captures all of the given URL
-      var candidate = componentSolutions[i];
+    var fullSolutions = ListWrapper.create();
-      if (candidate['unmatchedUrl'].length == 0) {
+
-        ListWrapper.push(fullSolutions, handlerToLeafInstructions(candidate, parentComponent));
+    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 childInstruction = this.recognize(candidate.unmatchedUrl, component);
-          var name = componentNames[cmpIndex];
-          var component = components[name];
-          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'],
+            matchedUrl: candidate.matchedUrl,
-            parentCost: candidate['cost']
+            parentSpecificity: candidate.specificity
           }));
         }
       }
     }
 
     if (fullSolutions.length > 0) {
-      var lowCost = fullSolutions[0].cost;
+      var mostSpecificSolution = fullSolutions[0];
-      var lowIndex = 0;
+      for (var solutionIndex = 1; solutionIndex < fullSolutions.length; solutionIndex++) {
-      for (var solIdx = 1; solIdx < fullSolutions.length; solIdx++) {
+        var solution = fullSolutions[solutionIndex];
-        if (fullSolutions[solIdx].cost < lowCost) {
+        if (solution.specificity > mostSpecificSolution.specificity) {
-          lowCost = fullSolutions[solIdx].cost;
+          mostSpecificSolution = solution;
-          lowIndex = solIdx;
         }
       }
 
-      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'],
+      params: routeMatch.params,
-      parentCost: 0
+      parentSpecificity: 0
     });
   });
   return new Instruction({
     component: parentComponent,
     children: children,
-    matchedUrl: context['matchedUrl'],
+    matchedUrl: routeMatch.matchedUrl,
-    parentCost: context['cost']
+    parentSpecificity: routeMatch.specificity
   });
 }
 
-// given:
-// { component: Foo }
-// mutates the config to:
-// { components: { default: Foo } }
-function normalizeConfig(config:StringMap<string, any>): StringMap<string, any> {
-  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);
+ * 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<string, any>): StringMap<string, any> {
+  if (!StringMapWrapper.contains(config, 'component')) {
+    return config;
+  }
+  var newConfig = {
+    'components': {
+      'default': config['component']
+    }
+  };
 
   StringMapWrapper.forEach(config, (value, key) => {
-      if (!StringWrapper.equals(key, 'component') && !StringWrapper.equals(key, 'components')) {
+    if (key != 'component' && key != 'components') {
-        StringMapWrapper.set(newConfig, key, value);
+      newConfig[key] = value;
     }
   });
 
   return newConfig;
 }
-  return config;
-}

modules/angular2/src/router/router.js

@@ -37,7 +37,7 @@ export class Router {
 
   _pipeline:Pipeline;
   _registry:RouteRegistry;
-  _outlets:Map<any, Outlet>;
+  _outlets:Map<any, RouterOutlet>;
   _subject:EventEmitter;
 
 

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].handler).toEqual(handler);
-      expect(recognizer.recognize('/test')[0]).toEqual({
-        'cost' : 1,
-        'handler': { 'components': { 'a': 'b' } },
-        'params': {},
-        'matchedUrl': '/test',
-        'unmatchedUrl': ''
-      });
     });
 
-    it('should work with leading slash', () => {
+
+    it('should recognize a single slash', () => {
       recognizer.addConfig('/', handler);
-
+      var solution = recognizer.recognize('/')[0];
-      expect(recognizer.recognize('/')[0]).toEqual({
+      expect(solution.handler).toEqual(handler);
-        'cost': 0,
-        'handler': { 'components': { 'a': 'b' } },
-        'params': {},
-        'matchedUrl': '/',
-        'unmatchedUrl': ''
-      });
     });
 
-    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({
+      var solution = recognizer.recognize('/user/brian')[0];
-        'cost': 101,
+      expect(solution.handler).toEqual(handler);
-        'handler': handler,
+      expect(solution.params).toEqual({ 'name': 'brian' });
-        'params': { 'name': 'brian' },
-        'matchedUrl': '/user/brian',
-        'unmatchedUrl': ''
-      });
     });
 
-    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,
+      var solution = solutions[0];
-        'handler': handler,
+      expect(solution.handler).toEqual(handler);
-        'params': {},
+      expect(solution.matchedUrl).toEqual('/b');
-        'matchedUrl': '/b',
-        'unmatchedUrl': ''
-      });
     });
 
+
     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(

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});