diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index 18b92f9b1381e..f1706f528382e 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -664,14 +664,14 @@ export class Router { }; }), switchMap(t => { + const browserUrlTree = this.browserUrlTree.toString(); 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() !== browserUrlTree || + // 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. + browserUrlTree !== this.currentUrlTree.toString(); const processCurrentUrl = (this.onSameUrlNavigation === 'reload' ? true : urlTransition) && this.urlHandlingStrategy.shouldProcessUrl(t.rawUrl); diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index 3e9210b418af1..ccfa781fee009 100644 --- a/packages/router/test/integration.spec.ts +++ b/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,58 @@ 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); + + // Try to navigate to /simple but guard rejects + canActivate.canActivateResult = false; + router.navigateByUrl('/simple'); + advance(fixture); + expect(location.path()).toEqual('/'); + expect(fixture.nativeElement.innerHTML).not.toContain('simple'); + + // Renavigate to /simple without guard rejection, should succeed. + 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); + + // Start navigating to /simple, but do not flush the guard delay + router.navigateByUrl('/simple'); + tick(); + // eager update strategy so URL is already updated. + expect(location.path()).toEqual('/simple'); + expect(fixture.nativeElement.innerHTML).not.toContain('simple'); + + // Start an additional navigation to /simple and ensure at least one of those succeeds. + // It's not super important which one gets processed, but in the past, the router would + // cancel the in-flight one and not process the new one. + router.navigateByUrl('/simple'); + tick(1000); + expect(location.path()).toEqual('/simple'); + expect(fixture.nativeElement.innerHTML).toContain('simple'); + })); }); it('should navigate back and forward', @@ -1569,6 +1640,33 @@ 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); + + // Try navigating to a component which throws an error during activation. + ConditionalThrowingCmp.throwError = true; + expect(() => { + router.navigateByUrl('/throwing'); + advance(fixture); + }).toThrow(); + expect(location.path()).toEqual('/'); + expect(fixture.nativeElement.innerHTML).not.toContain('throwing'); + + // Ensure we can re-navigate to that same URL and succeed. + 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; @@ -6145,6 +6243,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'); + } + } +} @@ -6195,7 +6302,8 @@ class LazyComponent { RootCmpWithTwoOutlets, RootCmpWithNamedOutlet, EmptyQueryParamsCmp, - ThrowingCmp + ThrowingCmp, + ConditionalThrowingCmp, ], @@ -6227,7 +6335,8 @@ class LazyComponent { RootCmpWithTwoOutlets, RootCmpWithNamedOutlet, EmptyQueryParamsCmp, - ThrowingCmp + ThrowingCmp, + ConditionalThrowingCmp, ], @@ -6260,7 +6369,8 @@ class LazyComponent { RootCmpWithTwoOutlets, RootCmpWithNamedOutlet, EmptyQueryParamsCmp, - ThrowingCmp + ThrowingCmp, + ConditionalThrowingCmp, ] }) class TestModule {