From acfce4a805c8bae4a278048fe6c45b5da857196c Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Mon, 26 Oct 2020 19:10:16 -0700 Subject: [PATCH 1/3] refactor(router): Small refactor of createUrlTree and extra tests This commit has a small refactor of some methods in create_url_tree.ts and adds some test cases, including two that will fail at the moment but should pass. A follow-up commit will make use of the refactorings to fix the test with minimal changes. --- packages/router/src/create_url_tree.ts | 22 +++-- packages/router/test/create_url_tree.spec.ts | 94 ++++++++++++++++++++ 2 files changed, 109 insertions(+), 7 deletions(-) diff --git a/packages/router/src/create_url_tree.ts b/packages/router/src/create_url_tree.ts index cec351c7cc52..c7fcb3126002 100644 --- a/packages/router/src/create_url_tree.ts +++ b/packages/router/src/create_url_tree.ts @@ -37,6 +37,14 @@ function isMatrixParams(command: any): boolean { return typeof command === 'object' && command != null && !command.outlets && !command.segmentPath; } +/** + * Determines if a given command has an `outlets` map. When we encounter a command + * with an outlets k/v map, we need to apply each outlet individually to the existing segment. + */ +function isCommandWithOutlets(command: any): command is {outlets: {[key: string]: any}} { + return typeof command === 'object' && command != null && command.outlets; +} + function tree( oldSegmentGroup: UrlSegmentGroup, newSegmentGroup: UrlSegmentGroup, urlTree: UrlTree, queryParams: Params, fragment: string): UrlTree { @@ -75,7 +83,7 @@ class Navigation { throw new Error('Root segment cannot have matrix parameters'); } - const cmdWithOutlet = commands.find(c => typeof c === 'object' && c != null && c.outlets); + const cmdWithOutlet = commands.find(isCommandWithOutlets); if (cmdWithOutlet && cmdWithOutlet !== last(commands)) { throw new Error('{outlets:{}} has to be the last command'); } @@ -179,14 +187,14 @@ function createPositionApplyingDoubleDots( } function getPath(command: any): any { - if (typeof command === 'object' && command != null && command.outlets) { + if (isCommandWithOutlets(command)) { return command.outlets[PRIMARY_OUTLET]; } return `${command}`; } function getOutlets(commands: any[]): {[k: string]: any[]} { - if (typeof commands[0] === 'object' && commands[0] !== null && commands[0].outlets) { + if (isCommandWithOutlets(commands[0])) { return commands[0].outlets; } @@ -276,9 +284,9 @@ function createNewSegmentGroup( let i = 0; while (i < commands.length) { - if (typeof commands[i] === 'object' && commands[i] !== null && - commands[i].outlets !== undefined) { - const children = createNewSegmentChildren(commands[i].outlets); + const command = commands[i]; + if (isCommandWithOutlets(command)) { + const children = createNewSegmentChildren(command.outlets); return new UrlSegmentGroup(paths, children); } @@ -290,7 +298,7 @@ function createNewSegmentGroup( continue; } - const curr = getPath(commands[i]); + const curr = getPath(command); const next = (i < commands.length - 1) ? commands[i + 1] : null; if (curr && next && isMatrixParams(next)) { paths.push(new UrlSegment(curr, stringify(next))); diff --git a/packages/router/test/create_url_tree.spec.ts b/packages/router/test/create_url_tree.spec.ts index 8603c1062a3b..75e256449262 100644 --- a/packages/router/test/create_url_tree.spec.ts +++ b/packages/router/test/create_url_tree.spec.ts @@ -112,6 +112,100 @@ describe('createUrlTree', () => { expect(serializer.serialize(t)).toEqual('/a/(b//right:d/11/e)'); }); + describe('', () => { + /** + * In this group of scenarios, imagine a config like: + * { + * path: 'parent', + * children: [ + * { + * path: 'child', + * component: AnyCmp + * }, + * { + * path: 'popup', + * outlet: 'secondary', + * component: AnyCmp + * } + * ] + * }, + * { + * path: 'other', + * component: AnyCmp + * }, + * { + * path: 'rootPopup', + * outlet: 'rootSecondary', + * } + */ + + it('should support removing secondary outlet with prefix', () => { + const p = serializer.parse('/parent/(child//secondary:popup)'); + const t = createRoot(p, ['parent', {outlets: {secondary: null}}]); + // - Segment index 0: + // * match and keep existing 'parent' + // - Segment index 1: + // * 'secondary' outlet cleared with `null` + // * 'primary' outlet not provided in the commands list, so the existing value is kept + expect(serializer.serialize(t)).toEqual('/parent/child'); + }); + + xit('should support updating secondary and primary outlets with prefix', () => { + const p = serializer.parse('/parent/child'); + const t = createRoot(p, ['parent', {outlets: {primary: 'child', secondary: 'popup'}}]); + expect(serializer.serialize(t)).toEqual('/parent/(child//secondary:popup)'); + }); + + xit('should support updating two outlets at the same time relative to non-root segment', () => { + const p = serializer.parse('/parent/child'); + const t = create( + p.root.children[PRIMARY_OUTLET], 0 /* relativeTo: 'parent' */, p, + [{outlets: {primary: 'child', secondary: 'popup'}}]); + expect(serializer.serialize(t)).toEqual('/parent/(child//secondary:popup)'); + }); + + it('should support adding multiple outlets with prefix', () => { + const p = serializer.parse(''); + const t = createRoot(p, ['parent', {outlets: {primary: 'child', secondary: 'popup'}}]); + expect(serializer.serialize(t)).toEqual('/parent/(child//secondary:popup)'); + }); + + it('should support updating clearing primary and secondary with prefix', () => { + const p = serializer.parse('/parent/(child//secondary:popup)'); + const t = createRoot(p, ['other']); + // Because we navigate away from the 'parent' route, the children of that route are cleared + // because they are note valid for the 'other' path. + expect(serializer.serialize(t)).toEqual('/other'); + }); + + it('should not clear secondary outlet when at root and prefix is used', () => { + const p = serializer.parse('/other(rootSecondary:rootPopup)'); + const t = createRoot(p, ['parent', {outlets: {primary: 'child', rootSecondary: null}}]); + // We prefixed the navigation with 'parent' so we cannot clear the "rootSecondary" outlet + // because once the outlets object is consumed, traversal is beyond the root segment. + expect(serializer.serialize(t)).toEqual('/parent/child(rootSecondary:rootPopup)'); + }); + + it('should not clear non-root secondary outlet when command is targeting root', () => { + const p = serializer.parse('/parent/(child//secondary:popup)'); + const t = createRoot(p, [{outlets: {secondary: null}}]); + // The start segment index for the command is at 0, but the outlet lives at index 1 + // so we cannot clear the outlet from processing segment index 0. + expect(serializer.serialize(t)).toEqual('/parent/(child//secondary:popup)'); + }); + + it('can clear an auxiliary outlet at the correct segment level', () => { + const p = serializer.parse('/parent/(child//secondary:popup)(rootSecondary:rootPopup)'); + // ^^^^^^^^^^^^^^^^^^^^^^ + // The parens here show that 'child' and 'secondary:popup' appear at the same 'level' in the + // config, i.e. are part of the same children list. You can also imagine an implicit paren + // group around the whole URL to visualize how 'parent' and 'rootSecondary:rootPopup' are also + // defined at the same level. + const t = createRoot(p, ['parent', {outlets: {primary: 'child', secondary: null}}]); + expect(serializer.serialize(t)).toEqual('/parent/child(rootSecondary:rootPopup)'); + }); + }); + it('should throw when outlets is not the last command', () => { const p = serializer.parse('/a'); expect(() => createRoot(p, ['a', {outlets: {right: ['c']}}, 'c'])) From c54f9e1feb40001ab6837c4f2d7f4e87382a6eab Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Mon, 26 Oct 2020 20:38:28 -0700 Subject: [PATCH 2/3] fix(router): Ensure all outlets are used when commands have a prefix When there is a primary outlet present in the outlets map and the object is also prefixed with some other commands, the current logic only uses the primary outlet and ignores the others. This change ensures that all outlets are respected at the segment level when prefixed with other commands. --- packages/router/src/create_url_tree.ts | 18 +++++++++--------- packages/router/test/create_url_tree.spec.ts | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/router/src/create_url_tree.ts b/packages/router/src/create_url_tree.ts index c7fcb3126002..2a70e23e2c8d 100644 --- a/packages/router/src/create_url_tree.ts +++ b/packages/router/src/create_url_tree.ts @@ -186,13 +186,6 @@ function createPositionApplyingDoubleDots( return new Position(g, false, ci - dd); } -function getPath(command: any): any { - if (isCommandWithOutlets(command)) { - return command.outlets[PRIMARY_OUTLET]; - } - return `${command}`; -} - function getOutlets(commands: any[]): {[k: string]: any[]} { if (isCommandWithOutlets(commands[0])) { return commands[0].outlets; @@ -259,7 +252,14 @@ function prefixedWith(segmentGroup: UrlSegmentGroup, startIndex: number, command while (currentPathIndex < segmentGroup.segments.length) { if (currentCommandIndex >= commands.length) return noMatch; const path = segmentGroup.segments[currentPathIndex]; - const curr = getPath(commands[currentCommandIndex]); + const command = commands[currentCommandIndex]; + // Do not try to consume command as part of the prefixing if it has outlets because it can + // contain outlets other than the one being processed. Consuming the outlets command would + // result in other outlets being ignored. + if (isCommandWithOutlets(command)) { + break; + } + const curr = `${command}`; const next = currentCommandIndex < commands.length - 1 ? commands[currentCommandIndex + 1] : null; @@ -298,7 +298,7 @@ function createNewSegmentGroup( continue; } - const curr = getPath(command); + const curr = isCommandWithOutlets(command) ? command.outlets[PRIMARY_OUTLET] : `${command}`; const next = (i < commands.length - 1) ? commands[i + 1] : null; if (curr && next && isMatrixParams(next)) { paths.push(new UrlSegment(curr, stringify(next))); diff --git a/packages/router/test/create_url_tree.spec.ts b/packages/router/test/create_url_tree.spec.ts index 75e256449262..d51fa48d9570 100644 --- a/packages/router/test/create_url_tree.spec.ts +++ b/packages/router/test/create_url_tree.spec.ts @@ -150,13 +150,13 @@ describe('createUrlTree', () => { expect(serializer.serialize(t)).toEqual('/parent/child'); }); - xit('should support updating secondary and primary outlets with prefix', () => { + it('should support updating secondary and primary outlets with prefix', () => { const p = serializer.parse('/parent/child'); const t = createRoot(p, ['parent', {outlets: {primary: 'child', secondary: 'popup'}}]); expect(serializer.serialize(t)).toEqual('/parent/(child//secondary:popup)'); }); - xit('should support updating two outlets at the same time relative to non-root segment', () => { + it('should support updating two outlets at the same time relative to non-root segment', () => { const p = serializer.parse('/parent/child'); const t = create( p.root.children[PRIMARY_OUTLET], 0 /* relativeTo: 'parent' */, p, From 5346092b1670337e86e6453edb465dbd67f9ed49 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 29 Oct 2020 13:01:33 -0700 Subject: [PATCH 3/3] fixup! refactor(router): Small refactor of createUrlTree and extra tests --- .../core/test/bundling/router/bundle.golden_symbols.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index 3757fd55ed8d..40f40ecf0159 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -1376,9 +1376,6 @@ { "name": "getParentInjectorView" }, - { - "name": "getPath" - }, { "name": "getPathIndexShift" }, @@ -1490,6 +1487,9 @@ { "name": "isArrayLike" }, + { + "name": "isCommandWithOutlets" + }, { "name": "isComponentDef" },