From eb2ceff4ba19a09a5f400f81e371ec176878cd37 Mon Sep 17 00:00:00 2001 From: Victor Savkin Date: Wed, 21 Dec 2016 15:47:58 -0500 Subject: [PATCH] fix(router): should reset location if a navigation by location is successful (#13545) Closes #13491 --- modules/@angular/router/src/router.ts | 46 +++++++++++-------- .../@angular/router/test/integration.spec.ts | 39 +++++++++++++++- 2 files changed, 65 insertions(+), 20 deletions(-) diff --git a/modules/@angular/router/src/router.ts b/modules/@angular/router/src/router.ts index 48c7506c9c..9750bff5b2 100644 --- a/modules/@angular/router/src/router.ts +++ b/modules/@angular/router/src/router.ts @@ -266,6 +266,8 @@ function defaultErrorHandler(error: any): any { throw error; } +type NavigationSource = 'imperative' | 'popstate' | 'hashchange'; + type NavigationParams = { id: number, rawUrl: UrlTree, @@ -273,7 +275,7 @@ type NavigationParams = { resolve: any, reject: any, promise: Promise, - imperative: boolean, + source: NavigationSource, }; @@ -374,20 +376,8 @@ export class Router { if (!this.locationSubscription) { this.locationSubscription = this.location.subscribe(Zone.current.wrap((change: any) => { const rawUrlTree = this.urlSerializer.parse(change['url']); - const lastNavigation = this.navigations.value; - - // 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 && lastNavigation.imperative && - lastNavigation.rawUrl.toString() === rawUrlTree.toString()) { - return; - } - - setTimeout(() => { - this.scheduleNavigation( - rawUrlTree, false, {skipLocationChange: change['pop'], replaceUrl: true}); - }, 0); + const source: NavigationSource = change['type'] === 'popstate' ? 'popstate' : 'hashchange'; + setTimeout(() => { this.scheduleNavigation(rawUrlTree, source, {replaceUrl: true}); }, 0); })); } } @@ -505,12 +495,12 @@ export class Router { Promise { if (url instanceof UrlTree) { return this.scheduleNavigation( - this.urlHandlingStrategy.merge(url, this.rawUrlTree), true, extras); + this.urlHandlingStrategy.merge(url, this.rawUrlTree), 'imperative', extras); } const urlTree = this.urlSerializer.parse(url); return this.scheduleNavigation( - this.urlHandlingStrategy.merge(urlTree, this.rawUrlTree), true, extras); + this.urlHandlingStrategy.merge(urlTree, this.rawUrlTree), 'imperative', extras); } /** @@ -585,8 +575,26 @@ export class Router { .subscribe(() => {}); } - private scheduleNavigation(rawUrl: UrlTree, imperative: boolean, extras: NavigationExtras): + private scheduleNavigation(rawUrl: UrlTree, source: NavigationSource, extras: NavigationExtras): Promise { + const lastNavigation = this.navigations.value; + + // 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()) { + return null; // return value is not used + } + + // Because of a bug in IE and Edge, the location class fires two events (popstate and + // hashchange) + // every single time. The second one should be ignored. Otherwise, the URL will flicker. + if (lastNavigation && source == 'hashchange' && lastNavigation.source === 'popstate' && + lastNavigation.rawUrl.toString() === rawUrl.toString()) { + return null; // return value is not used + } + let resolve: any = null; let reject: any = null; @@ -596,7 +604,7 @@ export class Router { }); const id = ++this.navigationId; - this.navigations.next({id, imperative, rawUrl, extras, resolve, reject, promise}); + this.navigations.next({id, source, rawUrl, extras, resolve, reject, promise}); // Make sure that the error is propagated even though `processNavigations` catch // handler does not rethrow diff --git a/modules/@angular/router/test/integration.spec.ts b/modules/@angular/router/test/integration.spec.ts index 3b2f708fe6..ca29193f81 100644 --- a/modules/@angular/router/test/integration.spec.ts +++ b/modules/@angular/router/test/integration.spec.ts @@ -389,6 +389,43 @@ describe('Integration', () => { expect(cmp.recordedUrls()).toEqual(['one/two', 'three/four']); }))); + describe('should reset location if a navigation by location is successful', () => { + beforeEach(() => { + TestBed.configureTestingModule({ + providers: [{ + provide: 'in1Second', + useValue: (c: any, a: ActivatedRouteSnapshot, b: RouterStateSnapshot) => { + let res: any = null; + const p = new Promise(_ => res = _); + setTimeout(() => res(true), 1000); + return p; + } + }] + }); + }); + + it('work', fakeAsync(inject([Router, Location], (router: Router, location: Location) => { + const fixture = createRoot(router, RootCmp); + + router.resetConfig([{path: 'simple', component: SimpleCmp, canActivate: ['in1Second']}]); + + // Trigger two location changes to the same URL. + // Because of the guard the order will look as follows: + // - location change 'simple' + // - start processing the change, start a guard + // - 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'); + + tick(2000); + advance(fixture); + + expect(location.path()).toEqual('/simple'); + }))); + }); + it('should support secondary routes', fakeAsync(inject([Router], (router: Router) => { const fixture = createRoot(router, RootCmp); @@ -1416,7 +1453,7 @@ describe('Integration', () => { log.push('called'); return false; } - }, + } ] }); });