From 8fd782b5a959620a37a4c4834d9b37a122d61f50 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 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 | 13 ++- packages/router/test/integration.spec.ts | 124 +++++++++++++++++++++-- 2 files changed, 119 insertions(+), 18 deletions(-) diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index e5c190ea112cf..d9478096fcb41 100644 --- a/packages/router/src/router.ts +++ b/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); diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index 19c82f0a86d5c..1e46bb41c5c43 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,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 {