From 85a3194a1766996bbc8ba4ffa726e6f970e59631 Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Thu, 20 Apr 2023 13:01:44 +0000 Subject: [PATCH] fix(router): handle constructor errors during initial navigation 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 https://github.com/angular/angular/issues/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: https://github.com/nodejs/node/issues/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 https://github.com/angular/angular/issues/33642 --- goldens/public-api/router/index.md | 2 +- .../router/bundle.golden_symbols.json | 3 ++ packages/router/src/provide_router.ts | 13 +++-- packages/router/src/router.ts | 10 ++-- packages/router/test/bootstrap.spec.ts | 48 ++++++++++++++++++- 5 files changed, 65 insertions(+), 11 deletions(-) diff --git a/goldens/public-api/router/index.md b/goldens/public-api/router/index.md index b716d1a2b50d6e..0c63b3a4cc13ab 100644 --- a/goldens/public-api/router/index.md +++ b/goldens/public-api/router/index.md @@ -698,7 +698,7 @@ export class Router { errorHandler: (error: any) => any; get events(): Observable; getCurrentNavigation(): Navigation | null; - initialNavigation(): void; + initialNavigation(): Promise; // @deprecated isActive(url: string | UrlTree, exact: boolean): boolean; isActive(url: string | UrlTree, matchOptions: IsActiveMatchOptions): boolean; diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index 74f1d96def2026..e9850bd8f22837 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -833,6 +833,9 @@ { "name": "_NullComponentFactoryResolver" }, + { + "name": "__async" + }, { "name": "__forward_ref__" }, diff --git a/packages/router/src/provide_router.ts b/packages/router/src/provide_router.ts index ed41b9fac0412e..9b8e3f6d5fc5df 100644 --- a/packages/router/src/provide_router.ts +++ b/packages/router/src/provide_router.ts @@ -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'; @@ -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(); @@ -335,7 +341,8 @@ export function withEnabledBlockingInitialNavigation(): EnabledBlockingInitialNa resolve(true); return bootstrapDone.closed ? of(void 0) : bootstrapDone; }; - router.initialNavigation(); + + return router.initialNavigation(); }); }); }; diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index 437725076d93d5..9416179be83f75 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -350,11 +350,11 @@ export class Router { /** * Sets up the location change listener and performs the initial navigation. */ - initialNavigation(): void { + async initialNavigation(): Promise { 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); } } @@ -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 { const extras: NavigationExtras = {replaceUrl: true}; // TODO: restoredState should always include the entire state, regardless @@ -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. */ diff --git a/packages/router/test/bootstrap.spec.ts b/packages/router/test/bootstrap.spec.ts index 164ec89f1d2761..10031e3a88b91f 100644 --- a/packages/router/test/bootstrap.spec.ts +++ b/packages/router/test/bootstrap.spec.ts @@ -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'; @@ -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: '', + 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(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({