From f747de3b4ad9ab3e81914a4cfb79c2872805fcc7 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 31 Aug 2023 11:39:39 -0700 Subject: [PATCH] feat(router): Add router configuration to resolve navigation promise on error (#48910) With the deprecation of the configurable errorHandler in the Router, there is a missing use-case to prevent the navigation promise from rejecting on an error. This rejection results in unhandled promise rejections. This commit allows developers to instruct the router to instead resolve the navigation promise with 'false', which matches the behavior of other failed navigations. Resolving the Promise would be the ideal default behavior. It is rare that any code handles the navigation Promise at all and even more rare that the Promise rejection is caught. Updating the default value for this option should be considered for an upcoming major version. fixes #48902 PR Close #48910 --- goldens/public-api/router/index.md | 1 + packages/router/src/navigation_transition.ts | 15 +++++++++++- packages/router/src/router.ts | 6 ++--- packages/router/src/router_config.ts | 17 ++++++++++++++ .../test/computed_state_restoration.spec.ts | 12 ++++++---- .../test/router_testing_harness.spec.ts | 23 ++++++++++++------- 6 files changed, 57 insertions(+), 17 deletions(-) diff --git a/goldens/public-api/router/index.md b/goldens/public-api/router/index.md index 70a817c58e0d5..c6d6291d5a00b 100644 --- a/goldens/public-api/router/index.md +++ b/goldens/public-api/router/index.md @@ -733,6 +733,7 @@ export interface RouterConfigOptions { canceledNavigationResolution?: 'replace' | 'computed'; onSameUrlNavigation?: OnSameUrlNavigation; paramsInheritanceStrategy?: 'emptyOnly' | 'always'; + resolveNavigationPromiseOnError?: boolean; urlUpdateStrategy?: 'deferred' | 'eager'; } diff --git a/packages/router/src/navigation_transition.ts b/packages/router/src/navigation_transition.ts index effe69c60c58a..1d05b557ed949 100644 --- a/packages/router/src/navigation_transition.ts +++ b/packages/router/src/navigation_transition.ts @@ -718,7 +718,20 @@ export class NavigationTransitions { try { overallTransitionState.resolve(router.errorHandler(e)); } catch (ee) { - overallTransitionState.reject(ee); + // TODO(atscott): consider flipping the default behavior of + // resolveNavigationPromiseOnError to be `resolve(false)` when + // undefined. This is the most sane thing to do given that + // applications very rarely handle the promise rejection and, as a + // result, would get "unhandled promise rejection" console logs. + // The vast majority of applications would not be affected by this + // change so omitting a migration seems reasonable. Instead, + // applications that rely on rejection can specifically opt-in to the + // old behavior. + if (this.options.resolveNavigationPromiseOnError) { + overallTransitionState.resolve(false); + } else { + overallTransitionState.reject(ee); + } } } return EMPTY; diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index 9dbac92f1021c..79d73a0423d3a 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -498,9 +498,9 @@ export class Router { * @param extras An options object that determines how the URL should be constructed or * interpreted. * - * @returns A Promise that resolves to `true` when navigation succeeds, to `false` when navigation - * fails, - * or is rejected on error. + * @returns A Promise that resolves to `true` when navigation succeeds, or `false` when navigation + * fails. The Promise is rejected when an error occurs if `resolveNavigationPromiseOnError` is + * not `true`. * * @usageNotes * diff --git a/packages/router/src/router_config.ts b/packages/router/src/router_config.ts index 09542d0d510bb..451d1506ab3ef 100644 --- a/packages/router/src/router_config.ts +++ b/packages/router/src/router_config.ts @@ -21,6 +21,10 @@ import {UrlSerializer, UrlTree} from './url_tree'; * * @publicApi * @deprecated Subscribe to the `Router` events and watch for `NavigationError` instead. + * If the ErrorHandler is used to prevent unhandled promise rejections when navigation + * errors occur, use the `resolveNavigationPromiseOnError` option instead. + * + * @see RouterConfigOptions */ export type ErrorHandler = (error: any) => any; @@ -110,6 +114,15 @@ export interface RouterConfigOptions { * showing an error message with the URL that failed. */ urlUpdateStrategy?: 'deferred'|'eager'; + + /** + * When `true`, the `Promise` will instead resolve with `false`, as it does with other failed + * navigations (for example, when guards are rejected). + + * Otherwise the `Promise` returned by the Router's navigation with be rejected + * if an error occurs. + */ + resolveNavigationPromiseOnError?: boolean; } /** @@ -222,6 +235,10 @@ export interface ExtraOptions extends InMemoryScrollingOptions, RouterConfigOpti * If the handler throws an exception, the navigation Promise is rejected with the exception. * * @deprecated Subscribe to the `Router` events and watch for `NavigationError` instead. + * If the ErrorHandler is used to prevent unhandled promise rejections when navigation + * errors occur, use the `resolveNavigationPromiseOnError` option instead. + * + * @see RouterConfigOptions */ errorHandler?: (error: any) => any; diff --git a/packages/router/test/computed_state_restoration.spec.ts b/packages/router/test/computed_state_restoration.spec.ts index fbc4abc93bd80..63979a472e8f3 100644 --- a/packages/router/test/computed_state_restoration.spec.ts +++ b/packages/router/test/computed_state_restoration.spec.ts @@ -109,7 +109,11 @@ describe('`restoredState#ɵrouterPageId`', () => { canLoad: ['alwaysFalse'] } ], - withRouterConfig({urlUpdateStrategy, canceledNavigationResolution: 'computed'})), + withRouterConfig({ + urlUpdateStrategy, + canceledNavigationResolution: 'computed', + resolveNavigationPromiseOnError: true, + })), ] }); const router = TestBed.inject(Router); @@ -477,10 +481,8 @@ describe('`restoredState#ɵrouterPageId`', () => { TestBed.inject(ThrowingCanActivateGuard).throw = true; - expect(() => { - location.back(); - advance(fixture); - }).toThrow(); + location.back(); + advance(fixture); expect(location.path()).toEqual('/second'); expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); diff --git a/packages/router/testing/test/router_testing_harness.spec.ts b/packages/router/testing/test/router_testing_harness.spec.ts index 48305cb6b75ae..8192255ef1da2 100644 --- a/packages/router/testing/test/router_testing_harness.spec.ts +++ b/packages/router/testing/test/router_testing_harness.spec.ts @@ -14,6 +14,8 @@ import {RouterTestingHarness} from '@angular/router/testing'; import {of} from 'rxjs'; import {delay} from 'rxjs/operators'; +import {withRouterConfig} from '../../src/provide_router'; + describe('navigateForTest', () => { it('gives null for the activatedComponent when no routes are configured', async () => { TestBed.configureTestingModule({providers: [provideRouter([])]}); @@ -52,15 +54,20 @@ describe('navigateForTest', () => { it('throws error if routing throws', async () => { TestBed.configureTestingModule({ - providers: [provideRouter([{ - path: '', - canActivate: [() => { - throw new Error('oh no'); - }], - children: [] - }])] + providers: [provideRouter( + [ + { + path: 'e', + canActivate: [() => { + throw new Error('oh no'); + }], + children: [] + }, + ], + withRouterConfig({resolveNavigationPromiseOnError: true}))] }); - await expectAsync(RouterTestingHarness.create('/')).toBeRejected(); + const harness = await RouterTestingHarness.create(); + await expectAsync(harness.navigateByUrl('e')).toBeResolvedTo(null); }); it('can observe param changes on routed component with second navigation', async () => {