Skip to content

Commit b2bc50d

Browse files
btfordbtford
committed
fix(router): correctly sort route matches with children by specificity
This changes the way we calculate specificity. Instead of using a number, we use a string, so that combining specificity across parent-child instructions becomes a matter of concatenating them Fixes #5848 Closes #6011
1 parent e748add commit b2bc50d

7 files changed

Lines changed: 80 additions & 26 deletions

File tree

‎modules/angular2/src/facade/lang.dart‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ class Math {
1111
static final _random = new math.Random();
1212
static int floor(num n) => n.floor();
1313
static double random() => _random.nextDouble();
14+
static num min(num a, num b) => math.min(a, b);
1415
}
1516

1617
class CONST {

‎modules/angular2/src/router/instruction.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,8 @@ export abstract class Instruction {
115115

116116
get urlParams(): string[] { return isPresent(this.component) ? this.component.urlParams : []; }
117117

118-
get specificity(): number {
119-
var total = 0;
118+
get specificity(): string {
119+
var total = '';
120120
if (isPresent(this.component)) {
121121
total += this.component.specificity;
122122
}
@@ -305,7 +305,7 @@ export class ComponentInstruction {
305305
public routeData: RouteData;
306306

307307
constructor(public urlPath: string, public urlParams: string[], data: RouteData,
308-
public componentType, public terminal: boolean, public specificity: number,
308+
public componentType, public terminal: boolean, public specificity: string,
309309
public params: {[key: string]: any} = null) {
310310
this.routeData = isPresent(data) ? data : BLANK_ROUTE_DATA;
311311
}

‎modules/angular2/src/router/path_recognizer.ts‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -96,45 +96,45 @@ function parsePathString(route: string): {[key: string]: any} {
9696

9797
var segments = splitBySlash(route);
9898
var results = [];
99-
var specificity = 0;
99+
100+
var specificity = '';
101+
102+
// a single slash (or "empty segment" is as specific as a static segment
103+
if (segments.length == 0) {
104+
specificity += '2';
105+
}
100106

101107
// The "specificity" of a path is used to determine which route is used when multiple routes match
102-
// a URL.
103-
// Static segments (like "/foo") are the most specific, followed by dynamic segments (like
104-
// "/:id"). Star segments
105-
// add no specificity. Segments at the start of the path are more specific than proceeding ones.
108+
// a URL. Static segments (like "/foo") are the most specific, followed by dynamic segments (like
109+
// "/:id"). Star segments add no specificity. Segments at the start of the path are more specific
110+
// than proceeding ones.
111+
//
106112
// The code below uses place values to combine the different types of segments into a single
107-
// integer that we can
108-
// sort later. Each static segment is worth hundreds of points of specificity (10000, 9900, ...,
109-
// 200), and each
110-
// dynamic segment is worth single points of specificity (100, 99, ... 2).
111-
if (segments.length > 98) {
112-
throw new BaseException(`'${route}' has more than the maximum supported number of segments.`);
113-
}
113+
// string that we can sort later. Each static segment is marked as a specificity of "2," each
114+
// dynamic segment is worth "1" specificity, and stars are worth "0" specificity.
114115

115116
var limit = segments.length - 1;
116117
for (var i = 0; i <= limit; i++) {
117118
var segment = segments[i], match;
118119

119120
if (isPresent(match = RegExpWrapper.firstMatch(paramMatcher, segment))) {
120121
results.push(new DynamicSegment(match[1]));
121-
specificity += (100 - i);
122+
specificity += '1';
122123
} else if (isPresent(match = RegExpWrapper.firstMatch(wildcardMatcher, segment))) {
123124
results.push(new StarSegment(match[1]));
125+
specificity += '0';
124126
} else if (segment == '...') {
125127
if (i < limit) {
126128
throw new BaseException(`Unexpected "..." before the end of the path for "${route}".`);
127129
}
128130
results.push(new ContinuationSegment());
129131
} else {
130132
results.push(new StaticSegment(segment));
131-
specificity += 100 * (100 - i);
133+
specificity += '2';
132134
}
133135
}
134-
var result = StringMapWrapper.create();
135-
StringMapWrapper.set(result, 'segments', results);
136-
StringMapWrapper.set(result, 'specificity', specificity);
137-
return result;
136+
137+
return {'segments': results, 'specificity': specificity};
138138
}
139139

140140
// this function is used to determine whether a route config path like `/foo/:id` collides with
@@ -177,7 +177,7 @@ function assertPath(path: string) {
177177
*/
178178
export class PathRecognizer {
179179
private _segments: Segment[];
180-
specificity: number;
180+
specificity: string;
181181
terminal: boolean = true;
182182
hash: string;
183183

‎modules/angular2/src/router/route_recognizer.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ export class RedirectRecognizer implements AbstractRecognizer {
5959

6060
// represents something like '/foo/:bar'
6161
export class RouteRecognizer implements AbstractRecognizer {
62-
specificity: number;
62+
specificity: string;
6363
terminal: boolean = true;
6464
hash: string;
6565

‎modules/angular2/src/router/route_registry.ts‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import {
88
isString,
99
isStringMap,
1010
Type,
11+
StringWrapper,
12+
Math,
1113
getTypeNameForDebugging,
1214
CONST_EXPR
1315
} from 'angular2/src/facade/lang';
@@ -492,7 +494,39 @@ function splitAndFlattenLinkParams(linkParams: any[]): any[] {
492494
* Given a list of instructions, returns the most specific instruction
493495
*/
494496
function mostSpecific(instructions: Instruction[]): Instruction {
495-
return ListWrapper.maximum(instructions, (instruction: Instruction) => instruction.specificity);
497+
instructions = instructions.filter((instruction) => isPresent(instruction));
498+
if (instructions.length == 0) {
499+
return null;
500+
}
501+
if (instructions.length == 1) {
502+
return instructions[0];
503+
}
504+
var first = instructions[0];
505+
var rest = instructions.slice(1);
506+
return rest.reduce((instruction: Instruction, contender: Instruction) => {
507+
if (compareSpecificityStrings(contender.specificity, instruction.specificity) == -1) {
508+
return contender;
509+
}
510+
return instruction;
511+
}, first);
512+
}
513+
514+
/*
515+
* Expects strings to be in the form of "[0-2]+"
516+
* Returns -1 if string A should be sorted above string B, 1 if it should be sorted after,
517+
* or 0 if they are the same.
518+
*/
519+
function compareSpecificityStrings(a: string, b: string): number {
520+
var l = Math.min(a.length, b.length);
521+
for (var i = 0; i < l; i += 1) {
522+
var ai = StringWrapper.charCodeAt(a, i);
523+
var bi = StringWrapper.charCodeAt(b, i);
524+
var difference = bi - ai;
525+
if (difference != 0) {
526+
return difference;
527+
}
528+
}
529+
return a.length - b.length;
496530
}
497531

498532
function assertTerminalComponent(component, path) {

‎modules/angular2/test/router/route_registry_spec.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,21 @@ export function main() {
181181
});
182182
}));
183183

184+
it('should prefer routes with high specificity over routes with children with lower specificity',
185+
inject([AsyncTestCompleter], (async) => {
186+
registry.config(RootHostCmp, new Route({path: '/first', component: DummyCmpA}));
187+
188+
// terminates to DummyCmpB
189+
registry.config(RootHostCmp,
190+
new Route({path: '/:second/...', component: SingleSlashChildCmp}));
191+
192+
registry.recognize('/first', [])
193+
.then((instruction) => {
194+
expect(instruction.component.componentType).toBe(DummyCmpA);
195+
async.done();
196+
});
197+
}));
198+
184199
it('should match the full URL using child components', inject([AsyncTestCompleter], (async) => {
185200
registry.config(RootHostCmp, new Route({path: '/first/...', component: DummyParentCmp}));
186201

@@ -322,6 +337,10 @@ class DummyCmpB {}
322337
class DefaultRouteCmp {
323338
}
324339

340+
@RouteConfig([new Route({path: '/', component: DummyCmpB, name: 'ThirdCmp'})])
341+
class SingleSlashChildCmp {
342+
}
343+
325344

326345
@RouteConfig([
327346
new Route(

‎modules/angular2/test/router/router_link_spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@ import {
3333
import {DOM} from 'angular2/src/platform/dom/dom_adapter';
3434
import {ResolvedInstruction} from 'angular2/src/router/instruction';
3535

36-
let dummyInstruction =
37-
new ResolvedInstruction(new ComponentInstruction('detail', [], null, null, true, 0), null, {});
36+
let dummyInstruction = new ResolvedInstruction(
37+
new ComponentInstruction('detail', [], null, null, true, '0'), null, {});
3838

3939
export function main() {
4040
describe('routerLink directive', function() {

0 commit comments

Comments
 (0)