From 13020f904fa96ed57af584c4c194f04f943d5e0e Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 18 Dec 2020 10:26:32 -0800 Subject: [PATCH] fix(router): correctly deactivate children with componentless parent (#40196) During route activation, a componentless route will not have a context created for it, but the logic continues to recurse so that children are still activated. This can be seen here: https://github.com/angular/angular/blob/362f45c4bf1bb49a90b014d2053f4c4474d132c0/packages/router/src/operators/activate_routes.ts#L151-L158 The current deactivation logic does not currently account for componentless routes. This commit adjusts the deactivation logic so that if a context cannot be retrieved for a given route (because it is componentless), we continue to recurse and deactivate the children using the same `parentContexts` in the same way that activation does. Fixes #20694 PR Close #40196 --- .../router/src/operators/activate_routes.ts | 25 ++-- packages/router/test/integration.spec.ts | 115 ++++++++++++++++-- 2 files changed, 120 insertions(+), 20 deletions(-) diff --git a/packages/router/src/operators/activate_routes.ts b/packages/router/src/operators/activate_routes.ts index a045bfe4a8..f62ec645a4 100644 --- a/packages/router/src/operators/activate_routes.ts +++ b/packages/router/src/operators/activate_routes.ts @@ -109,26 +109,27 @@ export class ActivateRoutes { private deactivateRouteAndOutlet( route: TreeNode, parentContexts: ChildrenOutletContexts): void { const context = parentContexts.getContext(route.value.outlet); + // The context could be `null` if we are on a componentless route but there may still be + // children that need deactivating. + const contexts = context && route.value.component ? context.children : parentContexts; + const children: {[outletName: string]: TreeNode} = nodeChildrenAsMap(route); - if (context) { - const children: {[outletName: string]: any} = nodeChildrenAsMap(route); - const contexts = route.value.component ? context.children : parentContexts; + for (const child of Object.values(children)) { + this.deactivateRouteAndItsChildren(child, contexts); + } - forEach(children, (v: any, k: string) => this.deactivateRouteAndItsChildren(v, contexts)); - - if (context.outlet) { - // Destroy the component - context.outlet.deactivate(); - // Destroy the contexts for all the outlets that were in the component - context.children.onOutletDeactivated(); - } + if (context && context.outlet) { + // Destroy the component + context.outlet.deactivate(); + // Destroy the contexts for all the outlets that were in the component + context.children.onOutletDeactivated(); } } private activateChildRoutes( futureNode: TreeNode, currNode: TreeNode|null, contexts: ChildrenOutletContexts): void { - const children: {[outlet: string]: any} = nodeChildrenAsMap(currNode); + const children: {[outlet: string]: TreeNode} = nodeChildrenAsMap(currNode); futureNode.children.forEach(c => { this.activateRoutes(c, children[c.value.outlet], contexts); this.forwardEvent(new ActivationEnd(c.value.snapshot)); diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index 038626f933..23041f1cf0 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -390,7 +390,7 @@ describe('Integration', () => { }); }); - describe('should advance the parent route after deactivating its children', () => { + describe('route activation', () => { @Component({template: ''}) class Parent { constructor(route: ActivatedRoute) { @@ -400,8 +400,24 @@ describe('Integration', () => { } } + @Component({ + template: ` + + + + ` + }) + class NamedOutletHost { + logDeactivate(route: string) { + log.push(route + ' deactivate'); + } + } + @Component({template: 'child1'}) class Child1 { + constructor() { + log.push('child1 constructor'); + } ngOnDestroy() { log.push('child1 destroy'); } @@ -412,20 +428,33 @@ describe('Integration', () => { constructor() { log.push('child2 constructor'); } + ngOnDestroy() { + log.push('child2 destroy'); + } + } + + @Component({template: 'child3'}) + class Child3 { + constructor() { + log.push('child3 constructor'); + } + ngOnDestroy() { + log.push('child3 destroy'); + } } @NgModule({ - declarations: [Parent, Child1, Child2], - entryComponents: [Parent, Child1, Child2], + declarations: [Parent, NamedOutletHost, Child1, Child2, Child3], + entryComponents: [Parent, NamedOutletHost, Child1, Child2, Child3], imports: [RouterModule] }) class TestModule { } - beforeEach(() => TestBed.configureTestingModule({imports: [TestModule]})); - - it('should work', - fakeAsync(inject([Router, Location], (router: Router, location: Location) => { + it('should advance the parent route after deactivating its children', fakeAsync(() => { + TestBed.configureTestingModule({imports: [TestModule]}); + const router = TestBed.inject(Router); + const location = TestBed.inject(Location); const fixture = createRoot(router, RootCmp); router.resetConfig([{ @@ -446,11 +475,81 @@ describe('Integration', () => { expect(location.path()).toEqual('/parent/2/child2'); expect(log).toEqual([ {id: '1'}, + 'child1 constructor', 'child1 destroy', {id: '2'}, 'child2 constructor', ]); - }))); + })); + + it('should deactivate outlet children with componentless parent', fakeAsync(() => { + TestBed.configureTestingModule({imports: [TestModule]}); + const router = TestBed.inject(Router); + const fixture = createRoot(router, RootCmp); + + router.resetConfig([ + { + path: 'named-outlets', + component: NamedOutletHost, + children: [ + { + path: 'home', + children: [ + {path: '', component: Child1, outlet: 'first'}, + {path: '', component: Child2, outlet: 'second'}, + {path: 'primary', component: Child3}, + ] + }, + { + path: 'about', + children: [ + {path: '', component: Child1, outlet: 'first'}, + {path: '', component: Child2, outlet: 'second'}, + ] + }, + + ] + }, + { + path: 'other', + component: Parent, + }, + ]); + + router.navigateByUrl('/named-outlets/home/primary'); + advance(fixture); + expect(log).toEqual([ + 'child3 constructor', // primary outlet always first + 'child1 constructor', + 'child2 constructor', + ]); + log.length = 0; + + router.navigateByUrl('/named-outlets/about'); + advance(fixture); + expect(log).toEqual([ + 'child3 destroy', + 'primary deactivate', + 'child1 destroy', + 'first deactivate', + 'child2 destroy', + 'second deactivate', + 'child1 constructor', + 'child2 constructor', + ]); + log.length = 0; + + router.navigateByUrl('/other'); + advance(fixture); + expect(log).toEqual([ + 'child1 destroy', + 'first deactivate', + 'child2 destroy', + 'second deactivate', + // route param subscription from 'Parent' component + {}, + ]); + })); }); it('should not wait for prior navigations to start a new navigation',