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: [