fix(router): do not call location.go when skipping a navigation (angular#19463)

Closes angular#18036

PR Close angular#19463

Author: vsavkin

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<boolean> {
+      url: UrlTree, rawUrl: UrlTree, skipLocationChange: boolean, replaceUrl: boolean, id: number,
+      precreatedState: RouterStateSnapshot|null): Promise<boolean> {
     if (id !== this.navigationId) {
-      this.location.go(this.urlSerializer.serialize(this.currentUrlTree));
       (this.events as Subject<Event>)
           .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));
   }
 }
 

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[] = [];