fix(router): handle constructor errors during initial navigation

[alan-agius4]

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.

{
    provide: APP_BOOTSTRAP_LISTENER,
    useFactory: () => {
      Promise.resolve().then(() => {})
    },
}

This change is also a step closer to address #33642

[alan-agius4]

caretaker: G3 failures are unrelated unrelated.

[atscott]

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?

[alan-agius4]

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.

[alan-agius4]

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.

[atscott]

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?

[alan-agius4]

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));

2. 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.

3. 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(() => {});

[atscott]

to be honest, I’m not a big fan of the Promise rejection at all right now. The rejection is pretty much never handled, except in tests. When’s the last time you’ve seen a try…catch or promise.catch/finally with a router navigation?

Since I’d like to change this behavior, I’m worried about surfacing the Promise in yet another place where it needs to be handled

#48910

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}