From baeec4dbe2562ad0a9218408265ada6119e87d97 Mon Sep 17 00:00:00 2001 From: vsavkin Date: Tue, 5 Dec 2017 11:34:34 -0500 Subject: [PATCH] fix(router): NavigatonError and NavigationCancel should be emitted after resetting the URL (#20803) PR Close #20803 --- packages/router/src/router.ts | 16 ++-- packages/router/test/integration.spec.ts | 96 ++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 6 deletions(-) diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index 0b92fcc778..53aed24a0b 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -745,12 +745,14 @@ export class Router { }, (e: any) => { if (isNavigationCancelingError(e)) { - this.resetUrlToCurrentUrlTree(); this.navigated = true; + this.resetStateAndUrl(storedState, storedUrl, rawUrl); (this.events as Subject) .next(new NavigationCancel(id, this.serializeUrl(url), e.message)); + resolvePromise(false); } else { + this.resetStateAndUrl(storedState, storedUrl, rawUrl); (this.events as Subject) .next(new NavigationError(id, this.serializeUrl(url), e)); try { @@ -759,15 +761,17 @@ export class Router { rejectPromise(ee); } } - - (this as{routerState: RouterState}).routerState = storedState; - this.currentUrlTree = storedUrl; - this.rawUrlTree = this.urlHandlingStrategy.merge(this.currentUrlTree, rawUrl); - this.resetUrlToCurrentUrlTree(); }); }); } + private resetStateAndUrl(storedState: RouterState, storedUrl: UrlTree, rawUrl: UrlTree): void { + (this as{routerState: RouterState}).routerState = storedState; + this.currentUrlTree = storedUrl; + this.rawUrlTree = this.urlHandlingStrategy.merge(this.currentUrlTree, rawUrl); + this.resetUrlToCurrentUrlTree(); + } + private resetUrlToCurrentUrlTree(): void { this.location.replaceState(this.urlSerializer.serialize(this.rawUrlTree)); } diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index 5797f6dbee..1fdcd7f9eb 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -17,6 +17,7 @@ import {Observable} from 'rxjs/Observable'; import {Observer} from 'rxjs/Observer'; import {of } from 'rxjs/observable/of'; import {map} from 'rxjs/operator/map'; +import {log} from 'util'; import {forEach} from '../src/utils/collection'; import {RouterTestingModule, SpyNgModuleFactoryLoader} from '../testing'; @@ -840,6 +841,92 @@ describe('Integration', () => { ]); }))); + it('should dispatch NavigationError after the url has been reset back', fakeAsync(() => { + const router = TestBed.get(Router); + const location = TestBed.get(Location); + const fixture = createRoot(router, RootCmp); + + router.resetConfig( + [{path: 'simple', component: SimpleCmp}, {path: 'throwing', component: ThrowingCmp}]); + + router.navigateByUrl('/simple'); + advance(fixture); + + let routerUrlBeforeEmittingError; + let locationUrlBeforeEmittingError; + router.events.forEach((e: any) => { + if (e instanceof NavigationError) { + routerUrlBeforeEmittingError = router.url; + locationUrlBeforeEmittingError = location.path(); + } + }); + + router.navigateByUrl('/throwing').catch(() => null); + advance(fixture); + + expect(routerUrlBeforeEmittingError).toEqual('/simple'); + expect(locationUrlBeforeEmittingError).toEqual('/simple'); + })); + + it('should not trigger another navigation when resetting the url back due to a NavigationError', + fakeAsync(() => { + const router = TestBed.get(Router); + router.onSameUrlNavigation = 'reload'; + + const fixture = createRoot(router, RootCmp); + + router.resetConfig( + [{path: 'simple', component: SimpleCmp}, {path: 'throwing', component: ThrowingCmp}]); + + const events: any[] = []; + router.events.forEach((e: any) => { + if (e instanceof NavigationStart) { + events.push(e.url); + } + }); + + router.navigateByUrl('/simple'); + advance(fixture); + + router.navigateByUrl('/throwing').catch(() => null); + advance(fixture); + + // we do not trigger another navigation to /simple + expect(events).toEqual(['/simple', '/throwing']); + })); + + it('should dispatch NavigationCancel after the url has been reset back', fakeAsync(() => { + TestBed.configureTestingModule( + {providers: [{provide: 'returnsFalse', useValue: () => false}]}); + + const router = TestBed.get(Router); + const location = TestBed.get(Location); + + const fixture = createRoot(router, RootCmp); + + router.resetConfig([ + {path: 'simple', component: SimpleCmp}, + {path: 'throwing', loadChildren: 'doesnotmatter', canLoad: ['returnsFalse']} + ]); + + router.navigateByUrl('/simple'); + advance(fixture); + + let routerUrlBeforeEmittingError; + let locationUrlBeforeEmittingError; + router.events.forEach((e: any) => { + if (e instanceof NavigationCancel) { + routerUrlBeforeEmittingError = router.url; + locationUrlBeforeEmittingError = location.path(); + } + }); + + (location).simulateHashChange('/throwing'); + advance(fixture); + + expect(locationUrlBeforeEmittingError).toEqual('/simple'); + })); + it('should support custom error handlers', fakeAsync(inject([Router], (router: Router) => { router.errorHandler = (error) => 'resolvedValue'; const fixture = createRoot(router, RootCmp); @@ -3915,6 +4002,12 @@ class RootCmpWithOnInit { class RootCmpWithTwoOutlets { } +@Component({selector: 'throwing-cmp', template: ''}) +class ThrowingCmp { + constructor() { throw new Error('Throwing Cmp'); } +} + + function advance(fixture: ComponentFixture): void { tick(); @@ -3955,6 +4048,7 @@ function createRoot(router: Router, type: any): ComponentFixture { RelativeLinkInIfCmp, RootCmpWithTwoOutlets, EmptyQueryParamsCmp, + ThrowingCmp ], @@ -3982,6 +4076,7 @@ function createRoot(router: Router, type: any): ComponentFixture { RelativeLinkInIfCmp, RootCmpWithTwoOutlets, EmptyQueryParamsCmp, + ThrowingCmp ], @@ -4010,6 +4105,7 @@ function createRoot(router: Router, type: any): ComponentFixture { RelativeLinkInIfCmp, RootCmpWithTwoOutlets, EmptyQueryParamsCmp, + ThrowingCmp ] }) class TestModule {