From b2bc50dbd14ba2db7a640d8c3c3c149c13db87a1 Mon Sep 17 00:00:00 2001 From: Brian Ford Date: Fri, 18 Dec 2015 10:05:55 -0800 Subject: [PATCH 1/7] 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 --- modules/angular2/src/facade/lang.dart | 1 + modules/angular2/src/router/instruction.ts | 6 +-- .../angular2/src/router/path_recognizer.ts | 38 +++++++++---------- .../angular2/src/router/route_recognizer.ts | 2 +- modules/angular2/src/router/route_registry.ts | 36 +++++++++++++++++- .../test/router/route_registry_spec.ts | 19 ++++++++++ .../angular2/test/router/router_link_spec.ts | 4 +- 7 files changed, 80 insertions(+), 26 deletions(-) diff --git a/modules/angular2/src/facade/lang.dart b/modules/angular2/src/facade/lang.dart index 96b2c01e2bda..c8067485f69f 100644 --- a/modules/angular2/src/facade/lang.dart +++ b/modules/angular2/src/facade/lang.dart @@ -11,6 +11,7 @@ class Math { static final _random = new math.Random(); static int floor(num n) => n.floor(); static double random() => _random.nextDouble(); + static num min(num a, num b) => math.min(a, b); } class CONST { diff --git a/modules/angular2/src/router/instruction.ts b/modules/angular2/src/router/instruction.ts index 7178b4b74284..815745bd920d 100644 --- a/modules/angular2/src/router/instruction.ts +++ b/modules/angular2/src/router/instruction.ts @@ -115,8 +115,8 @@ export abstract class Instruction { get urlParams(): string[] { return isPresent(this.component) ? this.component.urlParams : []; } - get specificity(): number { - var total = 0; + get specificity(): string { + var total = ''; if (isPresent(this.component)) { total += this.component.specificity; } @@ -305,7 +305,7 @@ export class ComponentInstruction { public routeData: RouteData; constructor(public urlPath: string, public urlParams: string[], data: RouteData, - public componentType, public terminal: boolean, public specificity: number, + public componentType, public terminal: boolean, public specificity: string, public params: {[key: string]: any} = null) { this.routeData = isPresent(data) ? data : BLANK_ROUTE_DATA; } diff --git a/modules/angular2/src/router/path_recognizer.ts b/modules/angular2/src/router/path_recognizer.ts index 1e03e1271d84..263ae9446d4a 100644 --- a/modules/angular2/src/router/path_recognizer.ts +++ b/modules/angular2/src/router/path_recognizer.ts @@ -96,21 +96,22 @@ function parsePathString(route: string): {[key: string]: any} { var segments = splitBySlash(route); var results = []; - var specificity = 0; + + var specificity = ''; + + // a single slash (or "empty segment" is as specific as a static segment + if (segments.length == 0) { + specificity += '2'; + } // 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. + // 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.`); - } + // string that we can sort later. Each static segment is marked as a specificity of "2," each + // dynamic segment is worth "1" specificity, and stars are worth "0" specificity. var limit = segments.length - 1; for (var i = 0; i <= limit; i++) { @@ -118,9 +119,10 @@ function parsePathString(route: string): {[key: string]: any} { if (isPresent(match = RegExpWrapper.firstMatch(paramMatcher, segment))) { results.push(new DynamicSegment(match[1])); - specificity += (100 - i); + specificity += '1'; } else if (isPresent(match = RegExpWrapper.firstMatch(wildcardMatcher, segment))) { results.push(new StarSegment(match[1])); + specificity += '0'; } else if (segment == '...') { if (i < limit) { throw new BaseException(`Unexpected "..." before the end of the path for "${route}".`); @@ -128,13 +130,11 @@ function parsePathString(route: string): {[key: string]: any} { results.push(new ContinuationSegment()); } else { results.push(new StaticSegment(segment)); - specificity += 100 * (100 - i); + specificity += '2'; } } - var result = StringMapWrapper.create(); - StringMapWrapper.set(result, 'segments', results); - StringMapWrapper.set(result, 'specificity', specificity); - return result; + + return {'segments': results, 'specificity': specificity}; } // this function is used to determine whether a route config path like `/foo/:id` collides with @@ -177,7 +177,7 @@ function assertPath(path: string) { */ export class PathRecognizer { private _segments: Segment[]; - specificity: number; + specificity: string; terminal: boolean = true; hash: string; diff --git a/modules/angular2/src/router/route_recognizer.ts b/modules/angular2/src/router/route_recognizer.ts index fb0b8973e90f..7f44fffa8612 100644 --- a/modules/angular2/src/router/route_recognizer.ts +++ b/modules/angular2/src/router/route_recognizer.ts @@ -59,7 +59,7 @@ export class RedirectRecognizer implements AbstractRecognizer { // represents something like '/foo/:bar' export class RouteRecognizer implements AbstractRecognizer { - specificity: number; + specificity: string; terminal: boolean = true; hash: string; diff --git a/modules/angular2/src/router/route_registry.ts b/modules/angular2/src/router/route_registry.ts index a897d4dcaa2c..dbf757f4a779 100644 --- a/modules/angular2/src/router/route_registry.ts +++ b/modules/angular2/src/router/route_registry.ts @@ -8,6 +8,8 @@ import { isString, isStringMap, Type, + StringWrapper, + Math, getTypeNameForDebugging, CONST_EXPR } from 'angular2/src/facade/lang'; @@ -492,7 +494,39 @@ function splitAndFlattenLinkParams(linkParams: any[]): any[] { * Given a list of instructions, returns the most specific instruction */ function mostSpecific(instructions: Instruction[]): Instruction { - return ListWrapper.maximum(instructions, (instruction: Instruction) => instruction.specificity); + instructions = instructions.filter((instruction) => isPresent(instruction)); + if (instructions.length == 0) { + return null; + } + if (instructions.length == 1) { + return instructions[0]; + } + var first = instructions[0]; + var rest = instructions.slice(1); + return rest.reduce((instruction: Instruction, contender: Instruction) => { + if (compareSpecificityStrings(contender.specificity, instruction.specificity) == -1) { + return contender; + } + return instruction; + }, first); +} + +/* + * Expects strings to be in the form of "[0-2]+" + * Returns -1 if string A should be sorted above string B, 1 if it should be sorted after, + * or 0 if they are the same. + */ +function compareSpecificityStrings(a: string, b: string): number { + var l = Math.min(a.length, b.length); + for (var i = 0; i < l; i += 1) { + var ai = StringWrapper.charCodeAt(a, i); + var bi = StringWrapper.charCodeAt(b, i); + var difference = bi - ai; + if (difference != 0) { + return difference; + } + } + return a.length - b.length; } function assertTerminalComponent(component, path) { diff --git a/modules/angular2/test/router/route_registry_spec.ts b/modules/angular2/test/router/route_registry_spec.ts index 5097afbdebac..972c78cdfb65 100644 --- a/modules/angular2/test/router/route_registry_spec.ts +++ b/modules/angular2/test/router/route_registry_spec.ts @@ -181,6 +181,21 @@ export function main() { }); })); + it('should prefer routes with high specificity over routes with children with lower specificity', + inject([AsyncTestCompleter], (async) => { + registry.config(RootHostCmp, new Route({path: '/first', component: DummyCmpA})); + + // terminates to DummyCmpB + registry.config(RootHostCmp, + new Route({path: '/:second/...', component: SingleSlashChildCmp})); + + registry.recognize('/first', []) + .then((instruction) => { + expect(instruction.component.componentType).toBe(DummyCmpA); + async.done(); + }); + })); + it('should match the full URL using child components', inject([AsyncTestCompleter], (async) => { registry.config(RootHostCmp, new Route({path: '/first/...', component: DummyParentCmp})); @@ -322,6 +337,10 @@ class DummyCmpB {} class DefaultRouteCmp { } +@RouteConfig([new Route({path: '/', component: DummyCmpB, name: 'ThirdCmp'})]) +class SingleSlashChildCmp { +} + @RouteConfig([ new Route( diff --git a/modules/angular2/test/router/router_link_spec.ts b/modules/angular2/test/router/router_link_spec.ts index 228750341de4..cf20732deb36 100644 --- a/modules/angular2/test/router/router_link_spec.ts +++ b/modules/angular2/test/router/router_link_spec.ts @@ -33,8 +33,8 @@ import { import {DOM} from 'angular2/src/platform/dom/dom_adapter'; import {ResolvedInstruction} from 'angular2/src/router/instruction'; -let dummyInstruction = - new ResolvedInstruction(new ComponentInstruction('detail', [], null, null, true, 0), null, {}); +let dummyInstruction = new ResolvedInstruction( + new ComponentInstruction('detail', [], null, null, true, '0'), null, {}); export function main() { describe('routerLink directive', function() { From 822e83ebb09f3e44e653226304b0ff5820f73a23 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Mon, 21 Dec 2015 10:02:09 -0800 Subject: [PATCH 2/7] chore(docs): update the merge process docs use caretaker rather than "on-duty" Closes #6058 --- COMMITTER.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/COMMITTER.md b/COMMITTER.md index 851237eb6ebf..ef408a6c6a47 100644 --- a/COMMITTER.md +++ b/COMMITTER.md @@ -11,8 +11,11 @@ Someone with committer access will do the rest. We have automated the process for merging pull requests into master. Our goal is to minimize the disruption for Angular committers and also prevent breakages on master. -When a PR is ready to merge, a project member in the CoreTeamMember list (see below) can add the special label, -`PR: merge`. +When a PR has `pr_state: LGTM` and is ready to merge, you should add the `pr_action: merge` label. +Currently (late 2015), we need to ensure that each PR will cleanly merge into the Google-internal version control, +so the caretaker reviews the changes manually. + +After this review, the caretaker adds `zomg_admin: do_merge` which is restricted to admins only. A robot running as [mary-poppins](https://github.com/mary-poppins) is notified that the label was added by an authorized person, and will create a new branch in the angular project, using the convention `presubmit-{username}-pr-{number}`. @@ -26,6 +29,6 @@ Finally, after merge `mary-poppins` removes the presubmit branch. ## Administration -The list of users who can trigger a merge by adding the label is stored in our appengine app datastore. +The list of users who can trigger a merge by adding the `zomg_admin: do_merge` label is stored in our appengine app datastore. Edit the contents of the [CoreTeamMember Table]( https://console.developers.google.com/project/angular2-automation/datastore/query?queryType=KindQuery&namespace=&kind=CoreTeamMember) From cab69f689f74ea4f7f50dc521971b417728a4d3e Mon Sep 17 00:00:00 2001 From: Vladislav Zarakovsky Date: Mon, 21 Dec 2015 14:40:35 +0300 Subject: [PATCH 3/7] docs(cheatsheet): fix typo in