From a5ffca0576bc67df8e4076741963a6e61eeefb2a Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Tue, 2 Jun 2020 15:55:38 -0700 Subject: [PATCH] fix(router): fix navigation ignoring logic to compare to the browser url (#37716) This PR changes the logic for determining when to skip route processing from using the URL of the last attempted navigation to the actual resulting URL after that transition. Because guards may prevent navigation and reset the browser URL, the raw URL of the previous transition may not match the actual URL of the browser at the end of the navigation process. For that reason, we need to use `urlAfterRedirects` instead. Other notes: These checks in scheduleNavigation were added in https://github.com/angular/angular/commit/eb2ceff4ba19a09a5f400f81e371ec176878cd37 The test still passes and, more surprisingly, passes if the checks are removed completely. There have likely been changes to the navigation handling that handle the test in a different way. That said, it still appears to be important to keep the checks there in some capacity because it does affect how many navigation events occur. This addresses an issue that came up in #16710: https://github.com/angular/angular/issues/16710#issuecomment-634869739 This also partially addresses #13586 in fixing history for imperative navigations that are cancelled by guards. PR Close #37716 --- packages/router/src/router.ts | 26 +++- packages/router/test/integration.spec.ts | 187 ++++++++++++++++++++++- 2 files changed, 203 insertions(+), 10 deletions(-) diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index 6ccfd0bf9f..f8508655c2 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -1126,12 +1126,28 @@ export class Router { rawUrl: UrlTree, source: NavigationTrigger, restoredState: RestoredState|null, extras: NavigationExtras, priorPromise?: {resolve: any, reject: any, promise: Promise}): Promise { + // * Imperative navigations (router.navigate) might trigger additional navigations to the same + // URL via a popstate event and the locationChangeListener. We should skip these duplicate + // navs. Duplicates may also be triggered by attempts to sync AngularJS and Angular router + // states. + // * Imperative navigations can be cancelled by router guards, meaning the URL won't change. If + // the user follows that with a navigation using the back/forward button or manual URL change, + // the destination may be the same as the previous imperative attempt. We should not skip + // these navigations because it's a separate case from the one above -- it's not a duplicate + // navigation. const lastNavigation = this.getTransition(); - // If the user triggers a navigation imperatively (e.g., by using navigateByUrl), - // and that navigation results in 'replaceState' that leads to the same URL, - // we should skip those. - if (lastNavigation && source !== 'imperative' && lastNavigation.source === 'imperative' && - lastNavigation.rawUrl.toString() === rawUrl.toString()) { + // We don't want to skip duplicate successful navs if they're imperative because + // onSameUrlNavigation could be 'reload' (so the duplicate is intended). + const browserNavPrecededByRouterNav = + source !== 'imperative' && lastNavigation?.source === 'imperative'; + const lastNavigationSucceeded = this.lastSuccessfulId === lastNavigation.id; + // If the last navigation succeeded or is in flight, we can use the rawUrl as the comparison. + // However, if it failed, we should compare to the final result (urlAfterRedirects). + const lastNavigationUrl = (lastNavigationSucceeded || this.currentNavigation) ? + lastNavigation.rawUrl : + lastNavigation.urlAfterRedirects; + const duplicateNav = lastNavigationUrl.toString() === rawUrl.toString(); + if (browserNavPrecededByRouterNav && duplicateNav) { return Promise.resolve(true); // return value is not used } diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index 9aa3775aca..8b3d5d23c1 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -909,7 +909,7 @@ describe('Integration', () => { expect(cmp.recordedUrls()).toEqual(['one/two', 'three/four']); }))); - describe('should reset location if a navigation by location is successful', () => { + describe('duplicate in-flight navigations', () => { beforeEach(() => { TestBed.configureTestingModule({ providers: [{ @@ -924,7 +924,29 @@ describe('Integration', () => { }); }); - it('work', fakeAsync(inject([Router, Location], (router: Router, location: Location) => { + it('should ignore the duplicate resulting from a location sync', fakeAsync(() => { + const router = TestBed.inject(Router); + const fixture = createRoot(router, RootCmp); + const location = TestBed.inject(Location) as SpyLocation; + router.resetConfig([{path: 'simple', component: SimpleCmp, canActivate: ['in1Second']}]); + + const recordedEvents: any[] = []; + router.events.forEach(e => onlyNavigationStartAndEnd(e) && recordedEvents.push(e)); + + // setTimeout used so this navigation resolves at the same time as the one that results + // from the location PopStateEvent (see Router#setUpLocationChangeListener). + setTimeout(() => { + router.navigateByUrl('/simple'); + }, 0); + location.simulateUrlPop('/simple'); + tick(1000); + advance(fixture); + expectEvents(recordedEvents, [[NavigationStart, '/simple'], [NavigationEnd, '/simple']]); + })); + + it('should reset location if a navigation by location is successful', fakeAsync(() => { + const router = TestBed.inject(Router); + const location = TestBed.inject(Location) as SpyLocation; const fixture = createRoot(router, RootCmp); router.resetConfig([{path: 'simple', component: SimpleCmp, canActivate: ['in1Second']}]); @@ -936,14 +958,14 @@ describe('Integration', () => { // - location change 'simple' // - the first location change gets canceled, the URL gets reset to '/' // - the second location change gets finished, the URL should be reset to '/simple' - (location).simulateUrlPop('/simple'); - (location).simulateUrlPop('/simple'); + location.simulateUrlPop('/simple'); + location.simulateUrlPop('/simple'); tick(2000); advance(fixture); expect(location.path()).toEqual('/simple'); - }))); + })); }); it('should support secondary routes', fakeAsync(inject([Router], (router: Router) => { @@ -2569,6 +2591,161 @@ describe('Integration', () => { }))); }); + describe('should not break the history', () => { + @Injectable({providedIn: 'root'}) + class MyGuard implements CanDeactivate { + allow: boolean = true; + canDeactivate(): boolean { + return this.allow; + } + } + + @Component({selector: 'parent', template: ''}) + class Parent { + } + + @Component({selector: 'home', template: 'home'}) + class Home { + } + + @Component({selector: 'child1', template: 'child1'}) + class Child1 { + } + + @Component({selector: 'child2', template: 'child2'}) + class Child2 { + } + + @Component({selector: 'child3', template: 'child3'}) + class Child3 { + } + + @Component({selector: 'child4', template: 'child4'}) + class Child4 { + } + + @Component({selector: 'child5', template: 'child5'}) + class Child5 { + } + + @NgModule({ + declarations: [Parent, Home, Child1, Child2, Child3, Child4, Child5], + entryComponents: [Child1, Child2, Child3, Child4, Child5], + imports: [RouterModule] + }) + class TestModule { + } + + let fixture: ComponentFixture; + + beforeEach(fakeAsync(() => { + TestBed.configureTestingModule({imports: [TestModule]}); + const router = TestBed.get(Router); + const location = TestBed.get(Location); + fixture = createRoot(router, Parent); + + router.resetConfig([ + {path: '', component: Home}, + {path: 'first', component: Child1}, + {path: 'second', component: Child2}, + {path: 'third', component: Child3, canDeactivate: [MyGuard]}, + {path: 'fourth', component: Child4}, + {path: 'fifth', component: Child5}, + ]); + + // Create a navigation history of pages 1-5, and go back to 3 so that there is both + // back and forward history. + router.navigateByUrl('/first'); + advance(fixture); + router.navigateByUrl('/second'); + advance(fixture); + router.navigateByUrl('/third'); + advance(fixture); + router.navigateByUrl('/fourth'); + advance(fixture); + router.navigateByUrl('/fifth'); + advance(fixture); + location.back(); + advance(fixture); + location.back(); + advance(fixture); + })); + + // TODO(https://github.com/angular/angular/issues/13586) + // A fix to this requires much more design + xit('when navigate back using Back button', fakeAsync(() => { + const location = TestBed.get(Location); + expect(location.path()).toEqual('/third'); + + TestBed.get(MyGuard).allow = false; + location.back(); + advance(fixture); + expect(location.path()).toEqual('/third'); + expect(fixture.nativeElement).toHaveText('child3'); + + TestBed.get(MyGuard).allow = true; + location.back(); + advance(fixture); + expect(location.path()).toEqual('/second'); + expect(fixture.nativeElement).toHaveText('child2'); + })); + + it('when navigate back imperatively', fakeAsync(() => { + const router = TestBed.get(Router); + const location = TestBed.get(Location); + expect(location.path()).toEqual('/third'); + + TestBed.get(MyGuard).allow = false; + router.navigateByUrl('/second'); + advance(fixture); + expect(location.path()).toEqual('/third'); + expect(fixture.nativeElement).toHaveText('child3'); + + TestBed.get(MyGuard).allow = true; + location.back(); + advance(fixture); + expect(location.path()).toEqual('/second'); + expect(fixture.nativeElement).toHaveText('child2'); + })); + + // TODO(https://github.com/angular/angular/issues/13586) + // A fix to this requires much more design + xit('when navigate back using Foward button', fakeAsync(() => { + const location = TestBed.get(Location); + expect(location.path()).toEqual('/third'); + + TestBed.get(MyGuard).allow = false; + location.forward(); + advance(fixture); + expect(location.path()).toEqual('/third'); + expect(fixture.nativeElement).toHaveText('child3'); + + TestBed.get(MyGuard).allow = true; + location.forward(); + advance(fixture); + expect(location.path()).toEqual('/fourth'); + expect(fixture.nativeElement).toHaveText('child4'); + })); + + it('when navigate forward imperatively', fakeAsync(() => { + const router = TestBed.get(Router); + const location = TestBed.get(Location); + expect(location.path()).toEqual('/third'); + + TestBed.get(MyGuard).allow = false; + router.navigateByUrl('/fourth'); + advance(fixture); + expect(location.path()).toEqual('/third'); + expect(fixture.nativeElement).toHaveText('child3'); + + TestBed.get(MyGuard).allow = true; + location.forward(); + advance(fixture); + expect(location.path()).toEqual('/fourth'); + expect(fixture.nativeElement).toHaveText('child4'); + })); + }); + describe('should redirect when guard returns UrlTree', () => { beforeEach(() => TestBed.configureTestingModule({ providers: [