fix(router): adjust setting navigationTransition when a new navigation cancels an existing one (#29636)

jasonaden · jasonaden · commit e884c0cd7b5c · 2019-04-01T12:11:57.000-07:00
Prior to this change, if a navigation was ongoing and a new one came in, the router could get into a state where `router.currentNavigation` was `null` even though a navigation was executing. This change moves where we set the `currentNavigation` value so it's inside a `switchMap`. This solves the problem because the `finally` on the `switchMap` had been setting `currentNavigation` to `null` but the new `currentNavigation` value would have already been set. Essentially this was a timing problem and is resolved with this change. Fixes #29389 #29590 PR Close #29636
diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts
@@ -452,25 +452,24 @@ export class Router {
               ...t, extractedUrl: this.urlHandlingStrategy.extract(t.rawUrl)
             } as NavigationTransition)),
 
-        // Store the Navigation object
-        tap(t => {
-          this.currentNavigation = {
-            id: t.id,
-            initialUrl: t.currentRawUrl,
-            extractedUrl: t.extractedUrl,
-            trigger: t.source,
-            extras: t.extras,
-            previousNavigation: this.lastSuccessfulNavigation ?
-                {...this.lastSuccessfulNavigation, previousNavigation: null} :
-                null
-          };
-        }),
-
         // Using switchMap so we cancel executing navigations when a new one comes in
         switchMap(t => {
           let completed = false;
           let errored = false;
           return of (t).pipe(
+              // Store the Navigation object
+              tap(t => {
+                this.currentNavigation = {
+                  id: t.id,
+                  initialUrl: t.currentRawUrl,
+                  extractedUrl: t.extractedUrl,
+                  trigger: t.source,
+                  extras: t.extras,
+                  previousNavigation: this.lastSuccessfulNavigation ?
+                      {...this.lastSuccessfulNavigation, previousNavigation: null} :
+                      null
+                };
+              }),
               switchMap(t => {
                 const urlTransition =
                     !this.navigated || t.extractedUrl.toString() !== this.browserUrlTree.toString();
diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts
@@ -1071,6 +1071,27 @@ describe('Integration', () => {
        ]);
      })));
 
+  it('should properly set currentNavigation when cancelling in-flight navigations',
+     fakeAsync(inject([Router], (router: Router) => {
+       const fixture = createRoot(router, RootCmp);
+
+       router.resetConfig([{path: 'user/:name', component: UserCmp}]);
+
+       router.navigateByUrl('/user/init');
+       advance(fixture);
+
+       router.navigateByUrl('/user/victor');
+       expect((router as any).currentNavigation).not.toBe(null);
+       router.navigateByUrl('/user/fedor');
+       // Due to https://github.com/angular/angular/issues/29389, this would be `false`
+       // when running a second navigation.
+       expect((router as any).currentNavigation).not.toBe(null);
+       advance(fixture);
+
+       expect((router as any).currentNavigation).toBe(null);
+       expect(fixture.nativeElement).toHaveText('user fedor');
+     })));
+
   it('should handle failed navigations gracefully', fakeAsync(inject([Router], (router: Router) => {
        const fixture = createRoot(router, RootCmp);
 
