From 9119e1f7bdfab3161accc80f303d00c4d04ac2c3 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 22 Jul 2021 12:01:38 -0700 Subject: [PATCH] refactor(router): Ensure computed state restoration works for thrown errors (#42933) When `canceledNavigationResolution='computed'`, the `Router` needs to handle the cases where errors are thrown. Previously, the logic was not updated and would simply do a `replaceState` rather than determining where in the history we should move to restore the page/url from before the failed navigation. PR Close #42933 --- packages/router/src/router.ts | 96 ++++++++++++--------- packages/router/test/integration.spec.ts | 101 ++++++++++++++++++++++- 2 files changed, 156 insertions(+), 41 deletions(-) diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index b62672a85f..af30d2e32b 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -771,7 +771,8 @@ export class Router { filter(t => { if (!t.guardsResult) { - this.cancelNavigationTransitionRestoreHistory(t, ''); + this.restoreHistory(t); + this.cancelNavigationTransition(t, ''); return false; } return true; @@ -796,7 +797,8 @@ export class Router { next: () => dataResolved = true, complete: () => { if (!dataResolved) { - this.cancelNavigationTransitionRestoreHistory( + this.restoreHistory(t); + this.cancelNavigationTransition( t, `At least one route resolver didn't emit any value.`); } @@ -889,7 +891,8 @@ export class Router { // AngularJS sync code which looks for a value here in order to determine // whether or not to handle a given popstate event or to leave it to the // Angular router. - this.cancelNavigationTransitionRestoreHistory(t, cancelationReason); + this.restoreHistory(t); + this.cancelNavigationTransition(t, cancelationReason); } else { // We cannot trigger a `location.historyGo` if the // cancellation was due to a new navigation before the previous could @@ -908,6 +911,10 @@ export class Router { this.currentNavigation = null; }), catchError((e) => { + // TODO(atscott): The NavigationTransition `t` used here does not accurately + // reflect the current state of the whole transition because some operations + // return a new object rather than modifying the one in the outermost + // `switchMap`. errored = true; /* This error type is issued during Redirect, and is handled as a * cancellation rather than an error. */ @@ -920,7 +927,7 @@ export class Router { // This is only applicable with initial navigation, so setting // `navigated` only when not redirecting resolves this scenario. this.navigated = true; - this.resetStateAndUrl(t.currentRouterState, t.currentUrlTree, t.rawUrl); + this.restoreHistory(t, true); } const navCancel = new NavigationCancel( t.id, this.serializeUrl(t.extractedUrl), e.message); @@ -952,7 +959,7 @@ export class Router { /* All other errors should reset to the router's internal URL reference to * the pre-error state. */ } else { - this.resetStateAndUrl(t.currentRouterState, t.currentUrlTree, t.rawUrl); + this.restoreHistory(t, true); const navError = new NavigationError(t.id, this.serializeUrl(t.extractedUrl), e); eventsSubject.next(navError); @@ -1458,11 +1465,53 @@ export class Router { } } - 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(); + /** + * Performs the necessary rollback action to restore the browser URL to the + * state before the transition. + */ + private restoreHistory(t: NavigationTransition, restoringFromCaughtError = false) { + if (this.canceledNavigationResolution === 'computed') { + const targetPagePosition = this.currentPageId - t.targetPageId; + // The navigator change the location before triggered the browser event, + // so we need to go back to the current url if the navigation is canceled. + // Also, when navigation gets cancelled while using url update strategy eager, then we need to + // go back. Because, when `urlUpdateSrategy` is `eager`; `setBrowserUrl` method is called + // before any verification. + const browserUrlUpdateOccurred = + (t.source === 'popstate' || this.urlUpdateStrategy === 'eager' || + this.currentUrlTree === this.currentNavigation?.finalUrl); + if (browserUrlUpdateOccurred && targetPagePosition !== 0) { + this.location.historyGo(targetPagePosition); + } else if ( + this.currentUrlTree === this.currentNavigation?.finalUrl && targetPagePosition === 0) { + // We got to the activation stage (where currentUrlTree is set to the navigation's + // finalUrl), but we weren't moving anywhere in history (skipLocationChange or replaceUrl). + // We still need to reset the router state back to what it was when the navigation started. + this.resetState(t); + // TODO(atscott): resetting the `browserUrlTree` should really be done in `resetState`. + // Investigate if this can be done by running TGP. + this.browserUrlTree = t.currentUrlTree; + this.resetUrlToCurrentUrlTree(); + } else { + // The browser URL and router state was not updated before the navigation cancelled so + // there's no restoration needed. + } + } else if (this.canceledNavigationResolution === 'replace') { + // TODO(atscott): It seems like we should _always_ reset the state here. It would be a no-op + // for `deferred` navigations that haven't change the internal state yet because guards + // reject. For 'eager' navigations, it seems like we also really should reset the state + // because the navigation was cancelled. Investigate if this can be done by running TGP. + if (restoringFromCaughtError) { + this.resetState(t); + } + this.resetUrlToCurrentUrlTree(); + } + } + + private resetState(t: NavigationTransition): void { + (this as {routerState: RouterState}).routerState = t.currentRouterState; + this.currentUrlTree = t.currentUrlTree; + this.rawUrlTree = this.urlHandlingStrategy.merge(this.currentUrlTree, t.rawUrl); } private resetUrlToCurrentUrlTree(): void { @@ -1471,33 +1520,6 @@ export class Router { this.generateNgRouterState(this.lastSuccessfulId, this.currentPageId)); } - /** - * Responsible for handling the cancellation of a navigation: - * - performs the necessary rollback action to restore the browser URL to the - * state before the transition - * - triggers the `NavigationCancel` event - * - resolves the transition promise with `false` - */ - private cancelNavigationTransitionRestoreHistory(t: NavigationTransition, reason: string) { - if (this.canceledNavigationResolution === 'computed') { - // The navigator change the location before triggered the browser event, - // so we need to go back to the current url if the navigation is canceled. - // Also, when navigation gets cancelled while using url update strategy eager, then we need to - // go back. Because, when `urlUpdateSrategy` is `eager`; `setBrowserUrl` method is called - // before any verification. - if (t.source === 'popstate' || this.urlUpdateStrategy === 'eager') { - const targetPagePosition = this.currentPageId - t.targetPageId; - this.location.historyGo(targetPagePosition); - } else { - // If update is not 'eager' and the transition navigation source isn't 'popstate', then the - // navigation was cancelled before any browser url change so nothing needs to be restored. - } - } else { - this.resetUrlToCurrentUrlTree(); - } - this.cancelNavigationTransition(t, reason); - } - private cancelNavigationTransition(t: NavigationTransition, reason: string) { const navCancel = new NavigationCancel(t.id, this.serializeUrl(t.extractedUrl), reason); this.triggerEvent(navCancel); diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index fb5211f2d5..3d95fa71a1 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -2822,6 +2822,20 @@ describe('Integration', () => { } } + @Injectable({providedIn: 'root'}) + class ThrowingCanActivateGuard implements CanActivate { + throw = false; + + constructor(private router: Router) {} + + canActivate(): boolean { + if (this.throw) { + throw new Error('error in guard'); + } + return true; + } + } + @Injectable({providedIn: 'root'}) class MyCanActivateGuard implements CanActivate { allow: boolean = true; @@ -2865,33 +2879,37 @@ describe('Integration', () => { const router = TestBed.inject(Router); (router as any).canceledNavigationResolution = 'computed'; const location = TestBed.inject(Location); - fixture = createRoot(router, SimpleCmp); + fixture = createRoot(router, RootCmp); router.resetConfig([ { path: 'first', component: SimpleCmp, canDeactivate: [MyCanDeactivateGuard], - canActivate: [MyCanActivateGuard], + canActivate: [MyCanActivateGuard, ThrowingCanActivateGuard], resolve: [MyResolve] }, { path: 'second', component: SimpleCmp, canDeactivate: [MyCanDeactivateGuard], - canActivate: [MyCanActivateGuard], + canActivate: [MyCanActivateGuard, ThrowingCanActivateGuard], resolve: [MyResolve] }, { path: 'third', component: SimpleCmp, canDeactivate: [MyCanDeactivateGuard], - canActivate: [MyCanActivateGuard], + canActivate: [MyCanActivateGuard, ThrowingCanActivateGuard], resolve: [MyResolve] }, { path: 'unguarded', component: SimpleCmp, }, + { + path: 'throwing', + component: ThrowingCmp, + }, {path: 'loaded', loadChildren: () => of(LoadedModule), canLoad: ['alwaysFalse']} ]); router.navigateByUrl('/first'); @@ -3156,6 +3174,81 @@ describe('Integration', () => { expect(location.path()).toEqual('/second'); expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); })); + + for (const urlUpdateSrategy of ['deferred', 'eager'] as const) { + it(`restores history correctly when an error is thrown in guard with urlUpdateStrategy ${ + urlUpdateSrategy}`, + fakeAsync(() => { + const location = TestBed.inject(Location); + const router = TestBed.inject(Router); + router.urlUpdateStrategy = urlUpdateSrategy; + + TestBed.inject(ThrowingCanActivateGuard).throw = true; + + expect(() => { + location.back(); + advance(fixture); + }).toThrow(); + expect(location.path()).toEqual('/second'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); + + TestBed.inject(ThrowingCanActivateGuard).throw = false; + location.back(); + advance(fixture); + expect(location.path()).toEqual('/first'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 1})); + })); + + it(`restores history correctly when component throws error in constructor with urlUpdateStrategy ${ + urlUpdateSrategy}`, + fakeAsync(() => { + const location = TestBed.inject(Location); + const router = TestBed.inject(Router); + router.urlUpdateStrategy = urlUpdateSrategy; + + router.navigateByUrl('/throwing').catch(() => null); + advance(fixture); + expect(location.path()).toEqual('/second'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); + + location.back(); + advance(fixture); + expect(location.path()).toEqual('/first'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 1})); + })); + } + + it('restores history correctly when component throws error in constructor and replaceUrl=true', + fakeAsync(() => { + const location = TestBed.inject(Location); + const router = TestBed.inject(Router); + + router.navigateByUrl('/throwing', {replaceUrl: true}).catch(() => null); + advance(fixture); + expect(location.path()).toEqual('/second'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); + + location.back(); + advance(fixture); + expect(location.path()).toEqual('/first'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 1})); + })); + + it('restores history correctly when component throws error in constructor and skipLocationChange=true', + fakeAsync(() => { + const location = TestBed.inject(Location); + const router = TestBed.inject(Router); + + router.navigateByUrl('/throwing', {skipLocationChange: true}).catch(() => null); + advance(fixture); + expect(location.path()).toEqual('/second'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); + + location.back(); + advance(fixture); + expect(location.path()).toEqual('/first'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 1})); + })); }); describe('guards', () => { describe('CanActivate', () => {