From 45e4e60fd6b8ae30a18368ac613f8f8ff4da3718 Mon Sep 17 00:00:00 2001 From: krzysztof-grzybek Date: Sun, 10 Oct 2021 11:54:16 +0200 Subject: [PATCH] fix(router): reuse route strategy fix (#43791) Currently, it's impossible to cache (detach/attach) parent route without caching child routes. This produces a bug, when navigating from a/b to c, then to a/d, where a route is cached. On the last navigation, we incorrectly restore a/b route instead of a/d. This change introduces new behavior: if the route should be detached/attached, we do so, but we check also child routes recursively. Fixes #17333 PR Close #43791 --- .../size-tracking/integration-payloads.json | 2 +- .../router/bundle.golden_symbols.json | 6 -- packages/router/src/create_router_state.ts | 17 +---- .../router/src/operators/activate_routes.ts | 16 +++-- packages/router/test/integration.spec.ts | 70 ++++++++++++++++++- 5 files changed, 80 insertions(+), 31 deletions(-) diff --git a/goldens/size-tracking/integration-payloads.json b/goldens/size-tracking/integration-payloads.json index 605a3035ab9..b4b1e73a9d5 100644 --- a/goldens/size-tracking/integration-payloads.json +++ b/goldens/size-tracking/integration-payloads.json @@ -42,7 +42,7 @@ "master": { "uncompressed": { "runtime": 2843, - "main": 238453, + "main": 237895, "polyfills": 37064, "src_app_lazy_lazy_module_ts": 795 } diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index 06559de6d0e..a96f9b9336d 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -941,9 +941,6 @@ { "name": "advanceActivatedRoute" }, - { - "name": "advanceActivatedRouteNodeAndItsChildren" - }, { "name": "allocExpando" }, @@ -1922,9 +1919,6 @@ { "name": "setDirectiveInputsWhichShadowsStyling" }, - { - "name": "setFutureSnapshotsOfActivatedRoutes" - }, { "name": "setIncludeViewProviders" }, diff --git a/packages/router/src/create_router_state.ts b/packages/router/src/create_router_state.ts index 8532faa045c..f00d40f0ca1 100644 --- a/packages/router/src/create_router_state.ts +++ b/packages/router/src/create_router_state.ts @@ -34,7 +34,8 @@ function createNode( const detachedRouteHandle = routeReuseStrategy.retrieve(curr.value); if (detachedRouteHandle !== null) { const tree = (detachedRouteHandle as DetachedRouteHandleInternal).route; - setFutureSnapshotsOfActivatedRoutes(curr, tree); + tree.value._futureSnapshot = curr.value; + tree.children = curr.children.map(c => createNode(routeReuseStrategy, c)); return tree; } } @@ -45,20 +46,6 @@ function createNode( } } -function setFutureSnapshotsOfActivatedRoutes( - curr: TreeNode, result: TreeNode): void { - if (curr.value.routeConfig !== result.value.routeConfig) { - throw new Error('Cannot reattach ActivatedRouteSnapshot created from a different route'); - } - if (curr.children.length !== result.children.length) { - throw new Error('Cannot reattach ActivatedRouteSnapshot with a different number of children'); - } - result.value._futureSnapshot = curr.value; - for (let i = 0; i < curr.children.length; ++i) { - setFutureSnapshotsOfActivatedRoutes(curr.children[i], result.children[i]); - } -} - function createOrReuseChildren( routeReuseStrategy: RouteReuseStrategy, curr: TreeNode, prevState: TreeNode) { diff --git a/packages/router/src/operators/activate_routes.ts b/packages/router/src/operators/activate_routes.ts index 8abe7300f17..3007441cb98 100644 --- a/packages/router/src/operators/activate_routes.ts +++ b/packages/router/src/operators/activate_routes.ts @@ -99,6 +99,13 @@ export class ActivateRoutes { private detachAndStoreRouteSubtree( route: TreeNode, parentContexts: ChildrenOutletContexts): void { const context = parentContexts.getContext(route.value.outlet); + const contexts = context && route.value.component ? context.children : parentContexts; + const children: {[outletName: string]: TreeNode} = nodeChildrenAsMap(route); + + for (const childOutlet of Object.keys(children)) { + this.deactivateRouteAndItsChildren(children[childOutlet], contexts); + } + if (context && context.outlet) { const componentRef = context.outlet.detach(); const contexts = context.children.onOutletDeactivated(); @@ -179,7 +186,9 @@ export class ActivateRoutes { // Otherwise attach from `RouterOutlet.ngOnInit` when it is instantiated context.outlet.attach(stored.componentRef, stored.route.value); } - advanceActivatedRouteNodeAndItsChildren(stored.route); + + advanceActivatedRoute(stored.route.value); + this.activateChildRoutes(futureNode, null, context.children); } else { const config = parentLoadedConfig(future.snapshot); const cmpFactoryResolver = config ? config.module.componentFactoryResolver : null; @@ -203,11 +212,6 @@ export class ActivateRoutes { } } -function advanceActivatedRouteNodeAndItsChildren(node: TreeNode): void { - advanceActivatedRoute(node.value); - node.children.forEach(advanceActivatedRouteNodeAndItsChildren); -} - function parentLoadedConfig(snapshot: ActivatedRouteSnapshot): LoadedRouterConfig|null { for (let s = snapshot.parent; s; s = s.parent) { const route = s.routeConfig; diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index f2c3cedfa00..4b03ab00f3f 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -8,7 +8,7 @@ import {APP_BASE_HREF, CommonModule, HashLocationStrategy, Location, LOCATION_INITIALIZED, LocationStrategy, PlatformLocation} from '@angular/common'; import {SpyLocation} from '@angular/common/testing'; -import {ChangeDetectionStrategy, Component, EventEmitter, Injectable, NgModule, NgModuleRef, NgZone, OnDestroy, ViewChild, ɵConsole as Console, ɵNoopNgZone as NoopNgZone} from '@angular/core'; +import {ChangeDetectionStrategy, Component, EventEmitter, Inject, Injectable, InjectionToken, NgModule, NgModuleRef, NgZone, OnDestroy, ViewChild, ɵConsole as Console, ɵNoopNgZone as NoopNgZone} from '@angular/core'; import {ComponentFixture, fakeAsync, inject, TestBed, tick} from '@angular/core/testing'; import {By} from '@angular/platform-browser/src/dom/debug/by'; import {expect} from '@angular/platform-browser/testing/src/matchers'; @@ -5711,9 +5711,11 @@ describe('Integration', () => { describe('Custom Route Reuse Strategy', () => { class AttachDetachReuseStrategy implements RouteReuseStrategy { stored: {[k: string]: DetachedRouteHandle} = {}; + pathsToDetach = ['a']; shouldDetach(route: ActivatedRouteSnapshot): boolean { - return route.routeConfig!.path === 'a'; + return typeof route.routeConfig!.path !== 'undefined' && + this.pathsToDetach.includes(route.routeConfig!.path); } store(route: ActivatedRouteSnapshot, detachedTree: DetachedRouteHandle): void { @@ -5827,6 +5829,7 @@ describe('Integration', () => { const fixture = createRoot(router, RootCmp); router.routeReuseStrategy = new AttachDetachReuseStrategy(); + (router.routeReuseStrategy as AttachDetachReuseStrategy).pathsToDetach = ['a', 'b']; spyOn(router.routeReuseStrategy, 'retrieve').and.callThrough(); router.resetConfig([ @@ -5857,7 +5860,7 @@ describe('Integration', () => { router.navigateByUrl('/a;p=1/b;p=2'); advance(fixture); // We retrieve both the stored route snapshots - expect(router.routeReuseStrategy.retrieve).toHaveBeenCalledTimes(2); + expect(router.routeReuseStrategy.retrieve).toHaveBeenCalledTimes(4); const teamCmp2 = fixture.debugElement.children[1].componentInstance; const simpleCmp2 = fixture.debugElement.children[1].children[1].componentInstance; expect(location.path()).toEqual('/a;p=1/b;p=2'); @@ -5868,6 +5871,7 @@ describe('Integration', () => { expect(teamCmp.route.snapshot).toBe(router.routerState.snapshot.root.firstChild); expect(teamCmp.route.snapshot.params).toEqual({p: '1'}); expect(teamCmp.route.firstChild.snapshot.params).toEqual({p: '2'}); + expect(teamCmp.recordedParams).toEqual([{}, {p: '1'}]); }))); it('should support shorter lifecycles', @@ -6019,6 +6023,66 @@ describe('Integration', () => { advance(fixture); expect(fixture.debugElement.query(By.directive(SimpleCmp))).toBeTruthy(); })); + + it('should allow to attach parent route with fresh child route', fakeAsync(() => { + const CREATED_COMPS = new InjectionToken('CREATED_COMPS'); + + @Component({selector: 'root', template: ``}) + class Root { + } + + @Component({selector: 'parent', template: ``}) + class Parent { + constructor(@Inject(CREATED_COMPS) createdComps: string[]) { + createdComps.push('parent'); + } + } + + @Component({selector: 'child', template: `child`}) + class Child { + constructor(@Inject(CREATED_COMPS) createdComps: string[]) { + createdComps.push('child'); + } + } + + @NgModule({ + declarations: [Root, Parent, Child], + imports: [ + CommonModule, + RouterTestingModule.withRoutes([ + {path: 'a', component: Parent, children: [{path: 'b', component: Child}]}, + {path: 'c', component: SimpleCmp} + ]), + ], + providers: [ + {provide: RouteReuseStrategy, useClass: AttachDetachReuseStrategy}, + {provide: CREATED_COMPS, useValue: []} + ] + }) + class TestModule { + } + TestBed.configureTestingModule({imports: [TestModule]}); + + const router = TestBed.inject(Router); + const fixture = createRoot(router, Root); + const createdComps = TestBed.inject(CREATED_COMPS); + + expect(createdComps).toEqual([]); + + router.navigateByUrl('/a/b'); + advance(fixture); + expect(createdComps).toEqual(['parent', 'child']); + + router.navigateByUrl('/c'); + advance(fixture); + expect(createdComps).toEqual(['parent', 'child']); + + // 'a' parent route will be reused by the `RouteReuseStrategy`, child 'b' should be + // recreated + router.navigateByUrl('/a/b'); + advance(fixture); + expect(createdComps).toEqual(['parent', 'child', 'child']); + })); }); });