Skip to content

Commit 50d7916

Browse files
atscottdylhunn
atscott
authored and
dylhunn
committed
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
1 parent 65cc8be commit 50d7916

6 files changed

Lines changed: 57 additions & 17 deletions

File tree

‎goldens/public-api/router/index.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -733,6 +733,7 @@ export interface RouterConfigOptions {
733733
canceledNavigationResolution?: 'replace' | 'computed';
734734
onSameUrlNavigation?: OnSameUrlNavigation;
735735
paramsInheritanceStrategy?: 'emptyOnly' | 'always';
736+
resolveNavigationPromiseOnError?: boolean;
736737
urlUpdateStrategy?: 'deferred' | 'eager';
737738
}
738739

‎packages/router/src/navigation_transition.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -718,7 +718,20 @@ export class NavigationTransitions {
718718
try {
719719
overallTransitionState.resolve(router.errorHandler(e));
720720
} catch (ee) {
721-
overallTransitionState.reject(ee);
721+
// TODO(atscott): consider flipping the default behavior of
722+
// resolveNavigationPromiseOnError to be `resolve(false)` when
723+
// undefined. This is the most sane thing to do given that
724+
// applications very rarely handle the promise rejection and, as a
725+
// result, would get "unhandled promise rejection" console logs.
726+
// The vast majority of applications would not be affected by this
727+
// change so omitting a migration seems reasonable. Instead,
728+
// applications that rely on rejection can specifically opt-in to the
729+
// old behavior.
730+
if (this.options.resolveNavigationPromiseOnError) {
731+
overallTransitionState.resolve(false);
732+
} else {
733+
overallTransitionState.reject(ee);
734+
}
722735
}
723736
}
724737
return EMPTY;

‎packages/router/src/router.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -498,9 +498,9 @@ export class Router {
498498
* @param extras An options object that determines how the URL should be constructed or
499499
* interpreted.
500500
*
501-
* @returns A Promise that resolves to `true` when navigation succeeds, to `false` when navigation
502-
* fails,
503-
* or is rejected on error.
501+
* @returns A Promise that resolves to `true` when navigation succeeds, or `false` when navigation
502+
* fails. The Promise is rejected when an error occurs if `resolveNavigationPromiseOnError` is
503+
* not `true`.
504504
*
505505
* @usageNotes
506506
*

‎packages/router/src/router_config.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,10 @@ import {UrlSerializer, UrlTree} from './url_tree';
2121
*
2222
* @publicApi
2323
* @deprecated Subscribe to the `Router` events and watch for `NavigationError` instead.
24+
* If the ErrorHandler is used to prevent unhandled promise rejections when navigation
25+
* errors occur, use the `resolveNavigationPromiseOnError` option instead.
26+
*
27+
* @see RouterConfigOptions
2428
*/
2529
export type ErrorHandler = (error: any) => any;
2630

@@ -110,6 +114,15 @@ export interface RouterConfigOptions {
110114
* showing an error message with the URL that failed.
111115
*/
112116
urlUpdateStrategy?: 'deferred'|'eager';
117+
118+
/**
119+
* When `true`, the `Promise` will instead resolve with `false`, as it does with other failed
120+
* navigations (for example, when guards are rejected).
121+
122+
* Otherwise the `Promise` returned by the Router's navigation with be rejected
123+
* if an error occurs.
124+
*/
125+
resolveNavigationPromiseOnError?: boolean;
113126
}
114127

115128
/**
@@ -222,6 +235,10 @@ export interface ExtraOptions extends InMemoryScrollingOptions, RouterConfigOpti
222235
* If the handler throws an exception, the navigation Promise is rejected with the exception.
223236
*
224237
* @deprecated Subscribe to the `Router` events and watch for `NavigationError` instead.
238+
* If the ErrorHandler is used to prevent unhandled promise rejections when navigation
239+
* errors occur, use the `resolveNavigationPromiseOnError` option instead.
240+
*
241+
* @see RouterConfigOptions
225242
*/
226243
errorHandler?: (error: any) => any;
227244

‎packages/router/test/computed_state_restoration.spec.ts‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,11 @@ describe('`restoredState#ɵrouterPageId`', () => {
109109
canLoad: ['alwaysFalse']
110110
}
111111
],
112-
withRouterConfig({urlUpdateStrategy, canceledNavigationResolution: 'computed'})),
112+
withRouterConfig({
113+
urlUpdateStrategy,
114+
canceledNavigationResolution: 'computed',
115+
resolveNavigationPromiseOnError: true,
116+
})),
113117
]
114118
});
115119
const router = TestBed.inject(Router);
@@ -477,10 +481,8 @@ describe('`restoredState#ɵrouterPageId`', () => {
477481

478482
TestBed.inject(ThrowingCanActivateGuard).throw = true;
479483

480-
expect(() => {
481-
location.back();
482-
advance(fixture);
483-
}).toThrow();
484+
location.back();
485+
advance(fixture);
484486
expect(location.path()).toEqual('/second');
485487
expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2}));
486488

‎packages/router/testing/test/router_testing_harness.spec.ts‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ import {RouterTestingHarness} from '@angular/router/testing';
1414
import {of} from 'rxjs';
1515
import {delay} from 'rxjs/operators';
1616

17+
import {withRouterConfig} from '../../src/provide_router';
18+
1719
describe('navigateForTest', () => {
1820
it('gives null for the activatedComponent when no routes are configured', async () => {
1921
TestBed.configureTestingModule({providers: [provideRouter([])]});
@@ -52,15 +54,20 @@ describe('navigateForTest', () => {
5254

5355
it('throws error if routing throws', async () => {
5456
TestBed.configureTestingModule({
55-
providers: [provideRouter([{
56-
path: '',
57-
canActivate: [() => {
58-
throw new Error('oh no');
59-
}],
60-
children: []
61-
}])]
57+
providers: [provideRouter(
58+
[
59+
{
60+
path: 'e',
61+
canActivate: [() => {
62+
throw new Error('oh no');
63+
}],
64+
children: []
65+
},
66+
],
67+
withRouterConfig({resolveNavigationPromiseOnError: true}))]
6268
});
63-
await expectAsync(RouterTestingHarness.create('/')).toBeRejected();
69+
const harness = await RouterTestingHarness.create();
70+
await expectAsync(harness.navigateByUrl('e')).toBeResolvedTo(null);
6471
});
6572

6673
it('can observe param changes on routed component with second navigation', async () => {

0 commit comments

Comments
 (0)