Skip to content

Commit

Permalink
fix(router): handle constructor errors during initial navigation
Browse files Browse the repository at this point in the history
Currently, when there is an error inside a lazy loaded component constructor an unhandled promise rejection is thrown which causes the error handler not to be invoked on the server due to an issue with Zone.js see #49930.

While this is a workaround for that in general we should not cause unhandled promise rejections and always await and catch promises especially going for going zoneless, this is because Node.js does not wait for pending unhandled promises and these will cause the process to exit. See: nodejs/node#22088

Eventually, the framework will need to be updated to better handle  promises in certain listeners for zoneless and avoid unhandled promises

Example on potential problematic code.
```ts
{
    provide: APP_BOOTSTRAP_LISTENER,
    useFactory: () => {
      Promise.resolve().then(() => {})
    },
}
```

This change is also a step closer to address #33642
  • Loading branch information
alan-agius4 committed Apr 20, 2023
1 parent c029c67 commit 85a3194
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 11 deletions.
2 changes: 1 addition & 1 deletion goldens/public-api/router/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,7 @@ export class Router {
errorHandler: (error: any) => any;
get events(): Observable<Event_2>;
getCurrentNavigation(): Navigation | null;
initialNavigation(): void;
initialNavigation(): Promise<void>;
// @deprecated
isActive(url: string | UrlTree, exact: boolean): boolean;
isActive(url: string | UrlTree, matchOptions: IsActiveMatchOptions): boolean;
Expand Down
3 changes: 3 additions & 0 deletions packages/core/test/bundling/router/bundle.golden_symbols.json
Original file line number Diff line number Diff line change
Expand Up @@ -833,6 +833,9 @@
{
"name": "_NullComponentFactoryResolver"
},
{
"name": "__async"
},
{
"name": "__forward_ref__"
},
Expand Down
13 changes: 10 additions & 3 deletions packages/router/src/provide_router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import {HashLocationStrategy, LOCATION_INITIALIZED, LocationStrategy, ViewportScroller} from '@angular/common';
import {APP_BOOTSTRAP_LISTENER, APP_INITIALIZER, ApplicationRef, Component, ComponentRef, ENVIRONMENT_INITIALIZER, EnvironmentInjector, EnvironmentProviders, inject, InjectFlags, InjectionToken, Injector, makeEnvironmentProviders, NgZone, Provider, Type} from '@angular/core';
import {APP_BOOTSTRAP_LISTENER, APP_INITIALIZER, ApplicationRef, Component, ComponentRef, ENVIRONMENT_INITIALIZER, EnvironmentInjector, EnvironmentProviders, ErrorHandler, inject, InjectFlags, InjectionToken, Injector, makeEnvironmentProviders, NgZone, Provider, Type} from '@angular/core';
import {of, Subject} from 'rxjs';

import {INPUT_BINDER, RoutedComponentInputBinder} from './directives/router_outlet';
Expand Down Expand Up @@ -203,7 +203,13 @@ export function getBootstrapListener() {
const bootstrapDone = injector.get(BOOTSTRAP_DONE);

if (injector.get(INITIAL_NAVIGATION) === InitialNavigation.EnabledNonBlocking) {
router.initialNavigation();
const errorHandler = injector.get(ErrorHandler, null);
const initialNavigation = router.initialNavigation();
if (errorHandler) {
// The below is to avoid unhandled promise rejections
// when there is a error in the component contructor.
initialNavigation.catch(e => errorHandler.handleError(e));
}
}

injector.get(ROUTER_PRELOADER, null, InjectFlags.Optional)?.setUpPreloading();
Expand Down Expand Up @@ -335,7 +341,8 @@ export function withEnabledBlockingInitialNavigation(): EnabledBlockingInitialNa
resolve(true);
return bootstrapDone.closed ? of(void 0) : bootstrapDone;
};
router.initialNavigation();

return router.initialNavigation();
});
});
};
Expand Down
10 changes: 5 additions & 5 deletions packages/router/src/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -350,11 +350,11 @@ export class Router {
/**
* Sets up the location change listener and performs the initial navigation.
*/
initialNavigation(): void {
async initialNavigation(): Promise<void> {
this.setUpLocationChangeListener();
if (!this.navigationTransitions.hasRequestedNavigation) {
const state = this.location.getState() as RestoredState;
this.navigateToSyncWithBrowser(this.location.path(true), IMPERATIVE_NAVIGATION, state);
await this.navigateToSyncWithBrowser(this.location.path(true), IMPERATIVE_NAVIGATION, state);
}
}

Expand Down Expand Up @@ -388,8 +388,8 @@ export class Router {
* two scenarios represent times when the browser URL/state has been updated and
* the Router needs to respond to ensure its internal state matches.
*/
private navigateToSyncWithBrowser(
url: string, source: NavigationTrigger, state: RestoredState|undefined) {
private async navigateToSyncWithBrowser(
url: string, source: NavigationTrigger, state: RestoredState|undefined): Promise<boolean> {
const extras: NavigationExtras = {replaceUrl: true};

// TODO: restoredState should always include the entire state, regardless
Expand All @@ -414,7 +414,7 @@ export class Router {
}

const urlTree = this.parseUrl(url);
this.scheduleNavigation(urlTree, source, restoredState, extras);
return this.scheduleNavigation(urlTree, source, restoredState, extras);
}

/** The current URL. */
Expand Down
48 changes: 46 additions & 2 deletions packages/router/test/bootstrap.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ import {DOCUMENT, PlatformLocation, ɵgetDOM as getDOM} from '@angular/common';
import {BrowserPlatformLocation} from '@angular/common/src/location/platform_location';
import {NullViewportScroller, ViewportScroller} from '@angular/common/src/viewport_scroller';
import {MockPlatformLocation} from '@angular/common/testing';
import {ApplicationRef, Component, CUSTOM_ELEMENTS_SCHEMA, destroyPlatform, ENVIRONMENT_INITIALIZER, inject, Injectable, NgModule} from '@angular/core';
import {ApplicationRef, Component, CUSTOM_ELEMENTS_SCHEMA, destroyPlatform, ENVIRONMENT_INITIALIZER, ErrorHandler, inject, Injectable, NgModule} from '@angular/core';
import {TestBed} from '@angular/core/testing';
import {BrowserModule} from '@angular/platform-browser';
import {bootstrapApplication, BrowserModule} from '@angular/platform-browser';
import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
import {NavigationEnd, provideRouter, Router, RouterModule, RouterOutlet, withEnabledBlockingInitialNavigation} from '@angular/router';

Expand Down Expand Up @@ -396,6 +396,50 @@ describe('bootstrap', () => {
});
});

it('should handle errors during initial navigation to a lazy component', async () => {
@Component({selector: 'lazy-comp-with-error', template: '', standalone: true})
class LazyCmpWithError {
constructor() {
throw new Error('Error from LazyCmpWithError ctor.');
}
}

@Component({
selector: 'test-app',
template: '<router-outlet></router-outlet>',
standalone: true,
imports: [RouterOutlet]
})
class RootComp {
}

const errorLogs: string[] = [];
await bootstrapApplication(RootComp, {
providers: [
...testProviders,
{
provide: ErrorHandler,
useClass: class {
handleError(error: Error) {
errorLogs.push(error.message);
}
},
},
provideRouter([{path: '**', loadComponent: () => LazyCmpWithError}]),
]
});

// Flush all microtasks by creating a macrotask.
// This is needed because the the framework promises are not always awaited.
// Such as promises inside APP_BOOTSTRAP_LISTENER, while on the browser this is not a
// big of deal on node this can result in some issues.
// - Node.js does not wait for promises which are not awaited.
// (https://github.com/nodejs/node/issues/22088)
// - We need to delay the check for logs to be done after the next macro eventloop cycle.
await new Promise<void>(resolve => setTimeout(() => resolve(), 0));
expect(errorLogs).toEqual(['Error from LazyCmpWithError ctor.']);
});

it('should reinit router navigation listeners if a previously bootstrapped root component is destroyed',
async () => {
@NgModule({
Expand Down

0 comments on commit 85a3194

Please sign in to comment.