From 4034f252c9707dabd01386843f7c278f3d2e4302 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 10 Sep 2021 13:25:14 -0700 Subject: [PATCH] fix(router): Allow renavigating to failed URLs (#43424) 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 PR Close #43424 --- packages/router/src/router.ts | 14 +-- packages/router/test/integration.spec.ts | 132 +++++++++++++++++++++-- 2 files changed, 128 insertions(+), 18 deletions(-) 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 {