From d53b96f2a2c7843751abc57e681138ac24fa4e90 Mon Sep 17 00:00:00 2001 From: Chuck Jazdzewski Date: Mon, 9 Oct 2017 16:37:31 -0700 Subject: [PATCH] Revert "fix(router): do not call `location.go` when skipping a navigation (#19463)" This reverts commit 66515412308138ad083bdd91ab2e80cd2bdf7fac. --- packages/router/src/router.ts | 14 ++-- packages/router/test/integration.spec.ts | 96 +++++------------------- 2 files changed, 26 insertions(+), 84 deletions(-) diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index d1d944380e..920715c1c4 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -580,9 +580,10 @@ export class Router { } private runNavigate( - url: UrlTree, rawUrl: UrlTree, skipLocationChange: boolean, replaceUrl: boolean, id: number, - precreatedState: RouterStateSnapshot|null): Promise { + url: UrlTree, rawUrl: UrlTree, shouldPreventPushState: boolean, shouldReplaceUrl: 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), @@ -704,9 +705,9 @@ export class Router { (this as{routerState: RouterState}).routerState = state; - if (!skipLocationChange) { + if (!shouldPreventPushState) { const path = this.urlSerializer.serialize(this.rawUrlTree); - if (this.location.isCurrentPathEqualTo(path) || replaceUrl) { + if (this.location.isCurrentPathEqualTo(path) || shouldReplaceUrl) { this.location.replaceState(path); } else { this.location.go(path); @@ -754,13 +755,14 @@ export class Router { (this as{routerState: RouterState}).routerState = storedState; this.currentUrlTree = storedUrl; this.rawUrlTree = this.urlHandlingStrategy.merge(this.currentUrlTree, rawUrl); - this.resetUrlToCurrentUrlTree(); + this.location.replaceState(this.serializeUrl(this.rawUrlTree)); }); }); } private resetUrlToCurrentUrlTree(): void { - this.location.replaceState(this.urlSerializer.serialize(this.rawUrlTree)); + const path = this.urlSerializer.serialize(this.rawUrlTree); + this.location.replaceState(path); } } diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index 11933d6c5c..4682d8b93e 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -12,7 +12,6 @@ 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'; @@ -51,87 +50,28 @@ describe('Integration', () => { expect(fixture.nativeElement).toHaveText('route'); }))); - describe('navigation', function() { - it('should navigate to the current URL', fakeAsync(inject([Router], (router: Router) => { - router.resetConfig([ - {path: '', component: SimpleCmp}, - {path: 'simple', component: SimpleCmp}, - ]); + it('should navigate to the current URL', + fakeAsync(inject([Router, Location], (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'] - ]); - }))); - - 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'); - }))); - }); + expectEvents(events, [ + [NavigationStart, '/simple'], [NavigationEnd, '/simple'], [NavigationStart, '/simple'], + [NavigationEnd, '/simple'] + ]); + }))); describe('should execute navigations serially', () => { let log: any[] = [];