Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(router): handle constructor errors during initial navigation #49953

Closed
wants to merge 1 commit into from
Closed

fix(router): handle constructor errors during initial navigation #49953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion goldens/public-api/router/index.md
View file
Open in desktop
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
13 changes: 10 additions & 3 deletions packages/router/src/provide_router.ts
View file
Open in desktop
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error would already be surfaced through the router’s errorHandler as well as the events as a NavigationError, right? Do we really need it raised in a third place?

Copy link
Contributor Author

@alan-agius4 alan-agius4 Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No the error is not captured in any other place at least only the unhandled promise rejection is currently surfaced to the user (console) when there is an error in the component ctor.

At the moment it is surfaced as an unhandled promise rejections, this just captures it and pass it to the error handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I stand corrected, the error is being added as navigation error. So the problem here is that the error handler provided from the DI is not used.

What are your thoughts about using the error handler provided through the DI token (ERROR_HANDLER) instead of an option to the router? Something like;

  private coreErrorHandler = inject(ERROR_HANDLER);

  /**
   * A handler for navigation errors in this NgModule.
   *
   * @deprecated Subscribe to the `Router` events and watch for `NavigationError` instead.
   *   `provideRouter` has the `withNavigationErrorHandler` feature to make this easier.
   * @see `withNavigationErrorHandler`
   */
  errorHandler = this.options.errorHandler ?? function defaultErrorHandler(error: unknown): never {
     this.coreErrorHandler.handle(error);
     throw error;
  }

This is important so not to make platform-server dependent on the router and have to listen to navigation errors from the router directly, where when using SSG we do want navigation errors to be fatal and causes the process to exit with a non zero error code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I would actually be more in favor of the initialNavigation catching and swallowing the promise rejection since it’s surfaced elsewhere. That should resolve the issue as well, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with catching the error will be swallowed since errors captured as NavigationErrors by default are not surfaced to the users. This is problematic as “critical” errors such as during component construction will not be surfaced unless users subscribe to the router events.

I see a couple of possible solutions.

  1. Catch the rejection and log it.
this.scheduleNavigation(urlTree, source, restoredState, extras).catch(e => this.errorHandler.handleError(e));
  1. Catch the rejection and change the defaultErrorHandler and capture log it
private coreErrorHandler = inject(ERROR_HANDLER);

errorHandler = this.options.errorHandler ?? function defaultErrorHandler(error: unknown): never {
   this.coreErrorHandler.handle(error);
   throw error;
}
...
this.scheduleNavigation(urlTree, source, restoredState, extras).catch(() => {});

The above somewhat aligns with what you want to do in #48910 but it does not remove the throw yet.

  1. Catch the error and register a default NavigationErrorHandler when none is provided.
export function provideRouter(routes: Routes, ...features: RouterFeatures[]): EnvironmentProviders {
  const providers: Provider[] = [
    {provide: ROUTES, multi: true, useValue: routes},
    typeof ngDevMode === 'undefined' || ngDevMode
      ? {provide: ROUTER_IS_PROVIDED, useValue: true}
      : [],
    {provide: ActivatedRoute, useFactory: rootRoute, deps: [Router]},
    {provide: APP_BOOTSTRAP_LISTENER, multi: true, useFactory: getBootstrapListener},
    features.map((feature) => feature.ɵproviders),
  ];

  if (!features.some(f => f.ɵkind === RouterFeatureKind.NavigationErrorHandlerFeature)) {
    providers.push(
      withNavigationErrorHandler((e) => inject(ErrorHandler).handleError(e)).ɵproviders
    );
  }

  return makeEnvironmentProviders(providers);
}
....
this.scheduleNavigation(urlTree, source, restoredState, extras).catch(() => {});

// 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
8 changes: 4 additions & 4 deletions packages/router/src/router.ts
View file
Open in desktop
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 @@ -389,7 +389,7 @@ export class Router {
* the Router needs to respond to ensure its internal state matches.
*/
private navigateToSyncWithBrowser(
url: string, source: NavigationTrigger, state: RestoredState|undefined) {
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
View file
Open in desktop
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