From fdfa31798befb57d90770956cc95511d9b3b7bd1 Mon Sep 17 00:00:00 2001 From: vsavkin Date: Fri, 29 Sep 2017 11:03:16 -0400 Subject: [PATCH] fix(router): do not call `location.go` when skipping a navigation (#19463) Closes #18036 PR Close #19463 --- packages/router/src/router.ts | 14 ++-- packages/router/test/integration.spec.ts | 96 +++++++++++++++++++----- 2 files changed, 84 insertions(+), 26 deletions(-) diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index 920715c1c4..d1d944380e 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -580,10 +580,9 @@ export class Router { } private runNavigate( - url: UrlTree, rawUrl: UrlTree, shouldPreventPushState: boolean, shouldReplaceUrl: boolean, - id: number, precreatedState: RouterStateSnapshot|null): Promise { + url: UrlTree, rawUrl: UrlTree, skipLocationChange: boolean, replaceUrl: boolean, id: number, + precreatedState: RouterStateSnapshot|null): Promise { if (id !== this.navigationId) { - this.location.go(this.urlSerializer.serialize(this.currentUrlTree)); (this.events as Subject) .next(new NavigationCancel( id, this.serializeUrl(url), @@ -705,9 +704,9 @@ export class Router { (this as{routerState: RouterState}).routerState = state; - if (!shouldPreventPushState) { + if (!skipLocationChange) { const path = this.urlSerializer.serialize(this.rawUrlTree); - if (this.location.isCurrentPathEqualTo(path) || shouldReplaceUrl) { + if (this.location.isCurrentPathEqualTo(path) || replaceUrl) { this.location.replaceState(path); } else { this.location.go(path); @@ -755,14 +754,13 @@ export class Router { (this as{routerState: RouterState}).routerState = storedState; this.currentUrlTree = storedUrl; this.rawUrlTree = this.urlHandlingStrategy.merge(this.currentUrlTree, rawUrl); - this.location.replaceState(this.serializeUrl(this.rawUrlTree)); + this.resetUrlToCurrentUrlTree(); }); }); } private resetUrlToCurrentUrlTree(): void { - const path = this.urlSerializer.serialize(this.rawUrlTree); - this.location.replaceState(path); + this.location.replaceState(this.urlSerializer.serialize(this.rawUrlTree)); } } diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index 4682d8b93e..11933d6c5c 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -12,6 +12,7 @@ import {ComponentFixture, TestBed, fakeAsync, inject, tick} from '@angular/core/ import {By} from '@angular/platform-browser/src/dom/debug/by'; import {expect} from '@angular/platform-browser/testing/src/matchers'; import {ActivatedRoute, ActivatedRouteSnapshot, ActivationEnd, ActivationStart, CanActivate, CanDeactivate, ChildActivationEnd, ChildActivationStart, DetachedRouteHandle, Event, GuardsCheckEnd, GuardsCheckStart, NavigationCancel, NavigationEnd, NavigationError, NavigationStart, PRIMARY_OUTLET, ParamMap, Params, PreloadAllModules, PreloadingStrategy, Resolve, ResolveEnd, ResolveStart, RouteConfigLoadEnd, RouteConfigLoadStart, RouteReuseStrategy, Router, RouterEvent, RouterModule, RouterPreloader, RouterStateSnapshot, RoutesRecognized, RunGuardsAndResolvers, UrlHandlingStrategy, UrlSegmentGroup, UrlTree} from '@angular/router'; +import {SpyLocation} from 'common/testing'; import {Observable} from 'rxjs/Observable'; import {Observer} from 'rxjs/Observer'; import {of } from 'rxjs/observable/of'; @@ -50,28 +51,87 @@ describe('Integration', () => { expect(fixture.nativeElement).toHaveText('route'); }))); - it('should navigate to the current URL', - fakeAsync(inject([Router, Location], (router: Router) => { - router.resetConfig([ - {path: '', component: SimpleCmp}, - {path: 'simple', component: SimpleCmp}, - ]); + describe('navigation', function() { + it('should navigate to the current URL', fakeAsync(inject([Router], (router: Router) => { + router.resetConfig([ + {path: '', component: SimpleCmp}, + {path: 'simple', component: SimpleCmp}, + ]); - const fixture = createRoot(router, RootCmp); - const events: Event[] = []; - router.events.subscribe(e => onlyNavigationStartAndEnd(e) && events.push(e)); + const fixture = createRoot(router, RootCmp); + const events: Event[] = []; + router.events.subscribe(e => onlyNavigationStartAndEnd(e) && events.push(e)); - router.navigateByUrl('/simple'); - tick(); + router.navigateByUrl('/simple'); + tick(); - router.navigateByUrl('/simple'); - tick(); + router.navigateByUrl('/simple'); + tick(); - expectEvents(events, [ - [NavigationStart, '/simple'], [NavigationEnd, '/simple'], [NavigationStart, '/simple'], - [NavigationEnd, '/simple'] - ]); - }))); + expectEvents(events, [ + [NavigationStart, '/simple'], [NavigationEnd, '/simple'], [NavigationStart, '/simple'], + [NavigationEnd, '/simple'] + ]); + }))); + + it('should not pollute browser history when replaceUrl is set to true', + fakeAsync(inject([Router, Location], (router: Router, location: SpyLocation) => { + router.resetConfig([ + {path: '', component: SimpleCmp}, {path: 'a', component: SimpleCmp}, + {path: 'b', component: SimpleCmp} + ]); + + const fixture = createRoot(router, RootCmp); + + router.navigateByUrl('/a', {replaceUrl: true}); + router.navigateByUrl('/b', {replaceUrl: true}); + tick(); + + expect(location.urlChanges).toEqual(['replace: /', 'replace: /b']); + }))); + + it('should skip navigation if another navigation is already scheduled', + fakeAsync(inject([Router, Location], (router: Router, location: SpyLocation) => { + router.resetConfig([ + {path: '', component: SimpleCmp}, {path: 'a', component: SimpleCmp}, + {path: 'b', component: SimpleCmp} + ]); + + const fixture = createRoot(router, RootCmp); + + router.navigate( + ['/a'], {queryParams: {a: true}, queryParamsHandling: 'merge', replaceUrl: true}); + router.navigate( + ['/b'], {queryParams: {b: true}, queryParamsHandling: 'merge', replaceUrl: true}); + tick(); + + /** + * Why do we have '/b?b=true' and not '/b?a=true&b=true'? + * + * This is because the router has the right to stop a navigation mid-flight if another + * navigation has been already scheduled. This is why we can use a top-level guard + * to perform redirects. Calling `navigate` in such a guard will stop the navigation, and + * the components won't be instantiated. + * + * This is a fundamental property of the router: it only cares about its latest state. + * + * This means that components should only map params to something else, not reduce them. + * In other words, the following component is asking for trouble: + * + * ``` + * class MyComponent { + * constructor(a: ActivatedRoute) { + * a.params.scan(...) + * } + * } + * ``` + * + * This also means "queryParamsHandling: 'merge'" should only be used to merge with + * long-living query parameters (e.g., debug). + */ + expect(router.url).toEqual('/b?b=true'); + }))); + }); describe('should execute navigations serially', () => { let log: any[] = [];