fix(router): make setUpLocationChangeListener idempotent

angular · Dec 5, 2016 · 25e5b2f · 25e5b2f
1 parent 307c469
commit 25e5b2f
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 17 deletions.
diff --git a/modules/@angular/router/src/router.ts b/modules/@angular/router/src/router.ts
@@ -382,23 +382,25 @@ export class Router {
   setUpLocationChangeListener(): void {
     // Zone.current.wrap is needed because of the issue with RxJS scheduler,
     // which does not work properly with zone.js in IE and Safari
-    this.locationSubscription = <any>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;
-      }
+    if (!this.locationSubscription) {
+      this.locationSubscription = <any>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);
-    }));
+        setTimeout(() => {
+          this.scheduleNavigation(
+              rawUrlTree, false, {skipLocationChange: change['pop'], replaceUrl: true});
+        }, 0);
+      }));
+    }
   }
 
   /**
@@ -443,7 +445,12 @@ export class Router {
   /**
    * Disposes of the router.
    */
-  dispose(): void { this.locationSubscription.unsubscribe(); }
+  dispose(): void {
+    if (this.locationSubscription) {
+      this.locationSubscription.unsubscribe();
+      this.locationSubscription = null;
+    }
+  }
 
   /**
    * Applies an array of commands to the current url tree and creates a new url tree.

diff --git a/modules/@angular/router/test/router.spec.ts b/modules/@angular/router/test/router.spec.ts
@@ -6,6 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {Location} from '@angular/common';
 import {TestBed} from '@angular/core/testing';
 
 import {ResolveData} from '../src/config';
@@ -32,6 +33,28 @@ describe('Router', () => {
     });
   });
 
+  describe('setUpLocationChangeListener', () => {
+    beforeEach(() => { TestBed.configureTestingModule({imports: [RouterTestingModule]}); });
+
+    it('should be indempotent', () => {
+      const r: Router = TestBed.get(Router);
+      const location: Location = TestBed.get(Location);
+
+      r.setUpLocationChangeListener();
+      const a = (<any>r).locationSubscription;
+      r.setUpLocationChangeListener();
+      const b = (<any>r).locationSubscription;
+
+      expect(a).toBe(b);
+
+      r.dispose();
+      r.setUpLocationChangeListener();
+      const c = (<any>r).locationSubscription;
+
+      expect(c).not.toBe(b);
+    });
+  });
+
   describe('PreActivation', () => {
     const serializer = new DefaultUrlSerializer();
     const inj = {get: (token: any) => () => `${token}_value`};