Skip to content

Commit

Permalink
fix(router): Default error handler no longer rejects the navigation p…
Browse files Browse the repository at this point in the history
…romise

The Router currently provides the ability to configure a custom
`errorHandler` for when unhandled exceptions are thrown during
navigation. This allows developers to handle navigation errors
separately from other errors that might be thrown and caught by the
global [`ErrorHandler`](https://angular.io/api/core/ErrorHandler). The
mechanism to do this would be to use a custom error handler that does
not rethrow the navigation error, thereby preventing it from ever
bubbling up to `ErrorHandler` at all.

Since we are deprecating the `errorHandler` configuration in the Router,
there needs to be a replacement for this use-case. That is, developers
need a way to handle navigation errors separately from other types of
errors.

This commit updates the default error handler in the Router to _not_
throw an error, meaning that the navigation promise will not be rejected.
Instead, the navigation promise is resolved with `false`, the same
behavior as when the navigation fails to complete for any other reason.

fixes #48902

BREAKING CHANGE: By default, the Router no longer rejects the navigation
promise when an unhandled Error is thrown during navigation.
Instead, the promise is resolved with `false`, matching the behavior of
any other navigations which do not complete successfully.
The default behavior previously rethrew the original error.
The original error is still available in the `NavigationError` event if
needed and can be handled by subscribing to the `Router.events` Observable.
  • Loading branch information
atscott committed Aug 30, 2023
1 parent eab9216 commit 382949d
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 48 deletions.
15 changes: 8 additions & 7 deletions packages/router/src/router.ts
Expand Up @@ -17,7 +17,7 @@ import {BeforeActivateRoutes, Event, IMPERATIVE_NAVIGATION, NavigationCancel, Na
import {NavigationBehaviorOptions, OnSameUrlNavigation, Routes} from './models';
import {isBrowserTriggeredNavigation, Navigation, NavigationExtras, NavigationTransition, NavigationTransitions, RestoredState, UrlCreationOptions} from './navigation_transition';
import {RouteReuseStrategy} from './route_reuse_strategy';
import {ROUTER_CONFIGURATION} from './router_config';
import {ErrorHandler, ROUTER_CONFIGURATION} from './router_config';
import {ROUTES} from './router_config_loader';
import {createEmptyState, RouterState} from './router_state';
import {Params} from './shared';
Expand All @@ -28,9 +28,11 @@ import {afterNextNavigation} from './utils/navigations';



function defaultErrorHandler(error: any): any {
throw error;
}
const defaultErrorHandler: ErrorHandler = error => {
// tslint:disable-next-line:no-console
ngDevMode && console.log(error);
return false;
};

function defaultMalformedUriErrorHandler(
error: URIError, urlSerializer: UrlSerializer, url: string): UrlTree {
Expand Down Expand Up @@ -681,9 +683,8 @@ 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.
*
* @usageNotes
*
Expand Down
6 changes: 2 additions & 4 deletions packages/router/test/computed_state_restoration.spec.ts
Expand Up @@ -477,10 +477,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}));

Expand Down
53 changes: 20 additions & 33 deletions packages/router/test/integration.spec.ts
Expand Up @@ -1645,10 +1645,8 @@ describe('Integration', () => {
const recordedEvents: Event[] = [];
router.events.forEach(e => recordedEvents.push(e));

let e: any;
router.navigateByUrl('/invalid').catch(_ => e = _);
router.navigateByUrl('/invalid');
advance(fixture);
expect(e.message).toContain('Cannot match any routes');

router.navigateByUrl('/user/fedor');
advance(fixture);
Expand Down Expand Up @@ -1685,7 +1683,7 @@ describe('Integration', () => {
]
});
const router = TestBed.inject(Router);
await expectAsync(router.navigateByUrl('/throw')).toBeRejected();
await router.navigateByUrl('/throw');
expect(TestBed.inject(Handler).handlerCalled).toBeTrue();
});

Expand Down Expand Up @@ -1732,10 +1730,8 @@ describe('Integration', () => {

// Try navigating to a component which throws an error during activation.
ConditionalThrowingCmp.throwError = true;
expect(() => {
router.navigateByUrl('/throwing');
advance(fixture);
}).toThrow();
router.navigateByUrl('/throwing');
advance(fixture);
expect(location.path()).toEqual('/');
expect(fixture.nativeElement.innerHTML).not.toContain('throwing');

Expand Down Expand Up @@ -1876,15 +1872,17 @@ describe('Integration', () => {
})));

it('should not swallow errors', fakeAsync(inject([Router], (router: Router) => {
const logSpy = spyOn(console, 'log');
const fixture = createRoot(router, RootCmp);

router.resetConfig([{path: 'simple', component: SimpleCmp}]);

router.navigateByUrl('/invalid');
expect(() => advance(fixture)).toThrow();
advance(fixture);

router.navigateByUrl('/invalid2');
expect(() => advance(fixture)).toThrow();
advance(fixture);
expect(logSpy).toHaveBeenCalledTimes(2);
})));

it('should not swallow errors from browser state update', async () => {
Expand Down Expand Up @@ -2144,19 +2142,12 @@ describe('Integration', () => {
[{path: 'simple', component: SimpleCmp, resolve: {error: 'resolveError'}}]);

const recordedEvents: any[] = [];
router.events.subscribe(e => e instanceof RouterEvent && recordedEvents.push(e));
router.events.subscribe(e => e instanceof NavigationError && recordedEvents.push(e));

let e: any = null;
router.navigateByUrl('/simple')!.catch(error => e = error);
router.navigateByUrl('/simple');
advance(fixture);

expectEvents(recordedEvents, [
[NavigationStart, '/simple'], [RoutesRecognized, '/simple'],
[GuardsCheckStart, '/simple'], [GuardsCheckEnd, '/simple'], [ResolveStart, '/simple'],
[NavigationError, '/simple']
]);

expect(e).toEqual('error');
expectEvents(recordedEvents, [[NavigationError, '/simple']]);
})));

it('should handle empty errors', fakeAsync(inject([Router], (router: Router) => {
Expand All @@ -2166,13 +2157,12 @@ describe('Integration', () => {
[{path: 'simple', component: SimpleCmp, resolve: {error: 'resolveNullError'}}]);

const recordedEvents: any[] = [];
router.events.subscribe(e => e instanceof RouterEvent && recordedEvents.push(e));
router.events.subscribe(e => e instanceof NavigationError && recordedEvents.push(e));

let e: any = 'some value';
router.navigateByUrl('/simple').catch(error => e = error);
router.navigateByUrl('/simple');
advance(fixture);

expect(e).toEqual(null);
expect(recordedEvents.length).toEqual(1);
})));

it('should not navigate when all resolvers return empty result',
Expand Down Expand Up @@ -2335,12 +2325,7 @@ describe('Integration', () => {
});
router.resetConfig(
[{path: 'throwing', resolve: {thrower: ThrowingResolver}, component: BlankCmp}]);
try {
await router.navigateByUrl('/throwing');
fail('navigation should throw');
} catch (e: unknown) {
expect((e as Error).message).toEqual(errorMessage);
}
await router.navigateByUrl('/throwing');

expect(caughtError).toBeDefined();
expect(caughtError?.target).toBeDefined();
Expand Down Expand Up @@ -5887,10 +5872,11 @@ describe('Integration', () => {

router.resetConfig([{path: 'lazy', loadChildren: () => LoadedModule}]);

let recordedError: any = null;
router.navigateByUrl('/lazy/loaded')!.catch(err => recordedError = err);
let recordedError: NavigationError|null = null;
router.events.subscribe(e => e instanceof NavigationError && (recordedError = e));
router.navigateByUrl('/lazy/loaded');
advance(fixture);
expect(recordedError.message).toContain(`NG04007`);
expect(recordedError!.error.message).toContain(`NG04007`);
})));

it('should combine routes from multiple modules into a single configuration',
Expand Down Expand Up @@ -6183,6 +6169,7 @@ describe('Integration', () => {
router.resetConfig([{path: 'lazy', loadChildren: () => LoadedModule}]);

let recordedError: any = null;
router.events.subscribe(e => e instanceof NavigationError && (recordedError = e.error));
router.navigateByUrl('/lazy/loaded').catch(err => recordedError = err);
advance(fixture);

Expand Down
20 changes: 16 additions & 4 deletions packages/router/test/standalone.spec.ts
Expand Up @@ -11,6 +11,8 @@ import {ComponentFixture, fakeAsync, TestBed, tick} from '@angular/core/testing'
import {By} from '@angular/platform-browser';
import {provideRoutes, Router, RouterModule, ROUTES} from '@angular/router';

import {NavigationError} from '../src/events';

@Component({template: '<div>simple standalone</div>', standalone: true})
export class SimpleStandaloneComponent {
}
Expand Down Expand Up @@ -50,6 +52,7 @@ describe('standalone in Router API', () => {

it('throws an error when loadChildren=>routes has a component that is not standalone',
fakeAsync(() => {
const logSpy = spyOn(console, 'log');
TestBed.configureTestingModule({
imports: [RouterModule.forRoot([
{
Expand All @@ -64,8 +67,9 @@ describe('standalone in Router API', () => {

const router = TestBed.inject(Router);
router.navigateByUrl('/lazy/notstandalone');
expect(() => advance(root))
.toThrowError(/.*lazy\/notstandalone.*component must be standalone/);
advance(root);
expect(logSpy.calls.argsFor(0)[0])
.toMatch(/.*lazy\/notstandalone.*component must be standalone/);
}));
});
describe('route providers', () => {
Expand Down Expand Up @@ -332,8 +336,12 @@ describe('standalone in Router API', () => {
});

const root = TestBed.createComponent(RootCmp);
let errorEvent: NavigationError|null = null;
TestBed.inject(Router).events.subscribe(
e => e instanceof NavigationError && (errorEvent = e));
TestBed.inject(Router).navigateByUrl('/home');
expect(() => advance(root)).toThrowError(/.*home.*component must be standalone/);
advance(root);
expect(errorEvent!.error.message).toMatch(/.*home.*component must be standalone/);
}));

it('throws error when loadComponent is used with a module', fakeAsync(() => {
Expand All @@ -349,8 +357,12 @@ describe('standalone in Router API', () => {
});

const root = TestBed.createComponent(RootCmp);
let errorEvent: NavigationError|null = null;
TestBed.inject(Router).events.subscribe(
e => e instanceof NavigationError && (errorEvent = e));
TestBed.inject(Router).navigateByUrl('/home');
expect(() => advance(root)).toThrowError(/.*home.*Use 'loadChildren' instead/);
advance(root);
expect(errorEvent!.error.message).toMatch(/.*home.*Use 'loadChildren' instead/);
}));
});
describe('default export unwrapping', () => {
Expand Down

0 comments on commit 382949d

Please sign in to comment.