fix(router): Allow renavigating to failed URLs

There are situations where the Router does not currently clean up failed navigations
correctly. While this is problematic on its own, we can mitigate some of
the damage by processing any URL when we get a navigation request when
the internal router state is out of sync.

Each of the added tests would fail without this change.

fixes #34795

packages/router/src/router.ts

@@ -651,13 +651,12 @@ export class Router {
                      }),
                      switchMap(t => {
                        const urlTransition = !this.navigated ||
-                           t.extractedUrl.toString() !== this.browserUrlTree.toString();
-                       /* || this.browserUrlTree.toString() !== this.currentUrlTree.toString() */
-                       // TODO(atscott): Run TGP to see if the above change can be made. There are
-                       // situations where a navigation is canceled _after_ browserUrlTree is
-                       // updated. For example, urlUpdateStrategy === 'eager': if a new
-                       // navigation happens (i.e. in a guard), this would cause the router to
-                       // be in an invalid state of tracking.
+                           t.extractedUrl.toString() !== this.browserUrlTree.toString() ||
+                           // Navigations which succeed or ones which fail and are cleaned up
+                           // correctly should result in `browserUrlTree` and `currentUrlTree`
+                           // matching. If this is not the case, assume something went wrong and try
+                           // processing the URL again.
+                           this.browserUrlTree.toString() !== this.currentUrlTree.toString();
                        const processCurrentUrl =
                            (this.onSameUrlNavigation === 'reload' ? true : urlTransition) &&
                            this.urlHandlingStrategy.shouldProcessUrl(t.rawUrl);

packages/router/test/integration.spec.ts

@@ -871,17 +871,36 @@ describe('Integration', () => {
      })));
 
   describe('"eager" urlUpdateStrategy', () => {
+    @Injectable()
+    class AuthGuard implements CanActivate {
+      canActivateResult = true;
+
+      canActivate() {
+        return this.canActivateResult;
+      }
+    }
+    @Injectable()
+    class DelayedGuard implements CanActivate {
+      canActivate() {
+        return of('').pipe(delay(1000), mapTo(true));
+      }
+    }
+
     beforeEach(() => {
       const serializer = new DefaultUrlSerializer();
       TestBed.configureTestingModule({
-        providers: [{
-          provide: 'authGuardFail',
-          useValue: (a: any, b: any) => {
-            return new Promise(res => {
-              setTimeout(() => res(serializer.parse('/login')), 1);
-            });
-          }
-        }]
+        providers: [
+          {
+            provide: 'authGuardFail',
+            useValue: (a: any, b: any) => {
+              return new Promise(res => {
+                setTimeout(() => res(serializer.parse('/login')), 1);
+              });
+            }
+          },
+          AuthGuard,
+          DelayedGuard,
+        ]
       });
     });
 
@@ -1028,6 +1047,52 @@ describe('Integration', () => {
          expect(navigation.extras.state).toBeDefined();
          expect(navigation.extras.state).toEqual({foo: 'bar'});
        })));
+
+    it('can renavigate to rejected URL', fakeAsync(() => {
+         const router = TestBed.inject(Router);
+         const canActivate = TestBed.inject(AuthGuard);
+         const location = TestBed.inject(Location) as SpyLocation
+         router.urlUpdateStrategy = 'eager';
+         router.resetConfig([
+           {path: '', component: BlankCmp},
+           {path: 'simple', component: SimpleCmp, canActivate: [AuthGuard]},
+         ]);
+         const fixture = createRoot(router, RootCmp);
+
+         canActivate.canActivateResult = false;
+         router.navigateByUrl('/simple');
+         advance(fixture);
+         expect(location.path()).toEqual('/');
+         expect(fixture.nativeElement.innerHTML).not.toContain('simple');
+
+         canActivate.canActivateResult = true;
+         router.navigateByUrl('/simple');
+         advance(fixture);
+         expect(location.path()).toEqual('/simple');
+         expect(fixture.nativeElement.innerHTML).toContain('simple');
+       }));
+
+    it('can renavigate to same URL during in-flight navigation', fakeAsync(() => {
+         const router = TestBed.inject(Router);
+         const location = TestBed.inject(Location) as SpyLocation
+         router.urlUpdateStrategy = 'eager';
+         router.resetConfig([
+           {path: '', component: BlankCmp},
+           {path: 'simple', component: SimpleCmp, canActivate: [DelayedGuard]},
+         ]);
+         const fixture = createRoot(router, RootCmp);
+
+         router.navigateByUrl('/simple');
+         tick();
+         // eager update strategy so URL is already updated.
+         expect(location.path()).toEqual('/simple');
+         expect(fixture.nativeElement.innerHTML).not.toContain('simple');
+
+         router.navigateByUrl('/simple');
+         tick(1000);
+         expect(location.path()).toEqual('/simple');
+         expect(fixture.nativeElement.innerHTML).toContain('simple');
+       }));
   });
 
   it('should navigate back and forward',
@@ -1568,6 +1633,31 @@ describe('Integration', () => {
          expect(locationUrlBeforeEmittingError).toEqual('/simple');
        }));
 
+    it('can renavigate to throwing component', fakeAsync(() => {
+         const router = TestBed.inject(Router);
+         const location = TestBed.inject(Location) as SpyLocation
+         router.urlUpdateStrategy = 'eager';
+         router.resetConfig([
+           {path: '', component: BlankCmp},
+           {path: 'throwing', component: ConditionalThrowingCmp},
+         ]);
+         const fixture = createRoot(router, RootCmp);
+
+         ConditionalThrowingCmp.throwError = true;
+         expect(() => {
+           router.navigateByUrl('/throwing');
+           advance(fixture);
+         }).toThrow();
+         expect(location.path()).toEqual('/');
+         expect(fixture.nativeElement.innerHTML).not.toContain('throwing');
+
+         ConditionalThrowingCmp.throwError = false;
+         router.navigateByUrl('/throwing');
+         advance(fixture);
+         expect(location.path()).toEqual('/throwing');
+         expect(fixture.nativeElement.innerHTML).toContain('throwing');
+       }));
+
     it('should reset the url with the right state when navigation errors', fakeAsync(() => {
          const router: Router = TestBed.inject(Router);
          const location = TestBed.inject(Location) as SpyLocation;
@@ -6198,6 +6288,15 @@ class ThrowingCmp {
     throw new Error('Throwing Cmp');
   }
 }
+@Component({selector: 'conditional-throwing-cmp', template: 'conditional throwing'})
+class ConditionalThrowingCmp {
+  static throwError = true;
+  constructor() {
+    if (ConditionalThrowingCmp.throwError) {
+      throw new Error('Throwing Cmp');
+    }
+  }
+}
 
 
 
@@ -6248,7 +6347,8 @@ class LazyComponent {
     RootCmpWithTwoOutlets,
     RootCmpWithNamedOutlet,
     EmptyQueryParamsCmp,
-    ThrowingCmp
+    ThrowingCmp,
+    ConditionalThrowingCmp,
   ],
 
 
@@ -6280,7 +6380,8 @@ class LazyComponent {
     RootCmpWithTwoOutlets,
     RootCmpWithNamedOutlet,
     EmptyQueryParamsCmp,
-    ThrowingCmp
+    ThrowingCmp,
+    ConditionalThrowingCmp,
   ],
 
 
@@ -6313,7 +6414,8 @@ class LazyComponent {
     RootCmpWithTwoOutlets,
     RootCmpWithNamedOutlet,
     EmptyQueryParamsCmp,
-    ThrowingCmp
+    ThrowingCmp,
+    ConditionalThrowingCmp,
   ]
 })
 class TestModule {