🚀[FEATURE]: Make choose to forward or not the errors to the global handler

[XavierDupessey]

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Performance issue
[x] Feature request
[ ] Documentation issue or request
[ ] Support request => https://github.com/ngxs/store/blob/master/CONTRIBUTING.md
[ ] Other... Please describe:

Current behavior

If we don't want to catch the errors directly in the actions to handle them with ofActionErrored, they get actually handled twice: once by ofActionErrored and once by the global error handler.

Expected behavior

I think we should be able to decide if NGXS should forward the errors to the global error handler or not. Another possibility could be to wrap them in a custom error object (e.g. UnhandledNgxsActionError) to be able to filter them in the handler.

Minimal reproduction of the problem with instructions

• Go to https://stackblitz.com/edit/ngxs-error-handling-c3brj2?file=app%2Fapp.component.ts
• Click on "Load data": 2 alerts are shown: one from ofActionErrored in app.component.ts and another one from handleError in global-error-handler.ts

Environment

Libs:
- @angular/core version: 9.1.12
- @ngxs/store version: 3.7.0

[markwhitfeld]

@XavierDupessey Thank you for submitting this issue.
Apart from the proposal of having the UnhandledNgxsActionError error type to assist with filtering, do you have any thoughts of how you would like to specify that a particular action should not be handled by the global error handler?

[XavierDupessey]

For a particular action, could it be part of the ActionOptions object?

@Action(Submit, { throwErrors: true })
submit(...)
{
  // Won't be forwarded to GlobalErrorHandler because this action was expected to throw errors
  return throwError(new Error('API is down'));
}

But I would prefer an option to set it globally (with NgxsConfig maybe?). I think it would be annoying to put { throwErrors: true } everywhere if we are used to handle errors with ofActionErrored, what do you think?

[markwhitfeld]

One option we could have a look at is to detect when an action is being monitored by an ofActionErrored pipe and then deem that action as handled and not forward to the global error handler in that case.
But this may be too much magic, and not make for an obvious usage pattern.

[kctang]

Detecting if an action is being monitored by ofActionErrored to decide if it should be forwarded to global error handler does feel too magical.

I prefer a more declarative approach, with ability to override like:

• a default global behavior (probably send to both to maintain backward compatibility),
• config to override global default (via NgxsModule.forRoot(...)?)
• option to override per action via @Action({ ...}) (~ as suggested by @XavierDupessey ).

[joaqcid]

another option could be to add a param to ofActionErrored to stop bubbling up the error

[DanBoSlice]

It seems like there is currently no way to return expected errors from an action without them being caught by the global error handler.

If, for example, I have two components that dispatch the same action but handle potential errors differently, I currently have no way of doing this, without the error also being handled by the global error handler. (Think adding a todo from a dialog component and from a normal page component. The error should be handling differently, e.g. closing the dialog vs navigating to a different page.)

This feature would solve the issue.

[fikkatra]

I'm in need of this feature, too. My use case is that some errors get handled in a custom way, inside the component. However, any errors that are not custom handled, should bubble up and are handled in a more general way by a custom ErrorHandler, typically by redirecting to an error page. Because the @action calls the ErrorHandler in case of error, the custom error handling code of the component is called too late: the ErrorHandler already redirected the user to a general error page and no custom error can be shown.

Rather than configuring the @action handler whether to call ErrorHandler or not, it makes much more sense to me to pass this configuration when dispatching the action, typically in the component. It is the component that 'knows' whether it's going to handle the error itself, or it would rather rely on the general ErrorHandler to do so.

It's been quite a while, any updates on this issue?

[fikkatra]

I'm curious to find out why the decision was made to explicitly call the Angular ErrorHandler from action handlers. Am I wrong to assume that any errors that are unhandled, eventually bubble up and are handled by the Angular ErrorHandler anyway?

This related issue seems to support my assumption: #803
Any errors occurring in action are passed to ErrorHandler, then bubble up when not handled, and are therefore passed to ErrorHandler AGAIN.

[fikkatra]

Awaiting this feature request, here's a way to bypass the undesired behaviour of errors being processed by the ErrorHandler even when they are caught, or errors that are processed twice by the ErrorHandler.

To summarize:

Desired behaviour:

• errors that aren't caught within the action handler, but are caught later (e.g. by the dispatcher), should not be handled by ErrorHandler
• errors that aren't caught anywhere and bubble up, should be handled by ErrorHandler, but only once

Observed behaviour:

• errors that aren't caught within the action handler, but are caught later (e.g. by the dispatcher), ARE handled by ErrorHandler
• errors that aren't caught anywhere and bubble up, are handled by ErrorHandler, but they are handled TWICE (once because NGXS calls ErrorHandler, once because Angular calls ErrorHandler)

Solution:
Unfortunately, until this feature request is properly resolved, NGXS ALWAYS calls ErrorHandler when an error leaves the action handler, there's currently no way to prevent this (see this comment). The only thing you can do, is customize the logic in ErrorHandler so it can detect whether it is called by ngxs (rather than by Angular for uncaught errors), and have a duplicate detection system to ignore errors.

To do this, write an NgxsPlugin that hooks into the ngxs pipe:

import { Injectable } from '@angular/core';
import { catchError } from 'rxjs/operators';
import { NgxsPlugin } from '@ngxs/store';
import { ErrorConstants } from './models/error-constants';

@Injectable()
export class NgxsErrorPlugin implements NgxsPlugin {
  public handle(state: any, action: any, next: any): void {
    return next(state, action).pipe(
      catchError((error) => {
        error['skipErrorFlag'] = true;
        throw error;
      }),
    );
  }
}

Write a custom ErrorHandler:

@Injectable({
  providedIn: 'root',
})
export class CustomErrorHandler implements ErrorHandler {
  constructor(private customLogger: Logger) {}

  public handleError(error: any): void {
    if (error['skipErrorFlag'] && !error['errorSkippedFlag']) {
      error['errorSkippedFlag'] = true;
      // ignore error
      return;
    }

    this.customLogger.error(error);
    this.handleUncaughtError(error);
  }

  private handleUncaughtError(err: any): void {
    // implement logic for uncaught errors
  }
}

Provide both the plugin and the custom ErrorHandler:

import { ErrorHandler, NgModule } from '@angular/core';
import { NGXS_PLUGINS } from '@ngxs/store';
import { CustomErrorHandler } from './services/custom-error-handler';
import { NgxsErrorPlugin } from './ngxs-errors-plugin';

@NgModule({
  // imports, exports, declarations...
  providers: [
    { provide: ErrorHandler, useClass: CustomErrorHandler },
    {
      provide: NGXS_PLUGINS,
      useClass: NgxsErrorPlugin,
      multi: true,
    },
  ],
})
export class AppModule {}

It's hacky, but at least it enables you to let the dispatcher (rather than the action handler) decide whether it wants to catch/handle the error or not, and have a mechanism to handle errors that aren't caught anywhere.

[TheCelavi]

I am against calling this a "feature". This is a clearly a bug.

In pure theory, you can divide exception in 2 general groups, expected from which you can recover, or the ones (expected or unexpected) from which you can not recover.

We are using Angular's global error handler as a way to catch exceptions from which we can not recover.

This is perfectly normal developer strategy, especially for us which are coming from languages such as Java, C#, PHP, etc... using frameworks like Symfony, per example. Global error handlers are used as final guard/logger for all non-handled exception. We use it to log error, notify admin to escalate issue and to show end user some kind of apologetic message for the issue.

Therefore if code below:

try {
   await store.dispatch(new MyAction()).toPromise();
} catch (e) {
   // this is expected error, lets handle it
}

produces an error in global handler - we automatically assume that there is a bug in NGXS. I was shocked after hours of debugging that this was per design. For me - this was a "piece of magic" that ignores my "catch" of expected error and forwarding it to undesired piece of application.

If designed idea was to have a "sink" where all errors from actions are pushed, Angular's global error handler is the last place to do so.

Thanks @fikkatra for idea how to deal with this issue.

---

Addendum - I have stumbled upon the following suggestion "catch error in action handler and set state store to some kind of error state, put some error flag to true". This is terrible idea - I have invoked an action to transition state from state A to state B. Transition could not occur because of temporarily problem, but I can recover (invoke action again per example, login form is clear example). Having "error" flag in store just makes things complicated and simple, trivial code:

try {
await store.dispatch(new Login(user, pass)).toPromise();
} catch (e) {
toast.show('Invalid user/pass');
}

becomes unnecessarily complicated because now I have to deal with "error" state as well.

[arturovt]

Yep, I agree this was a bug, and not even a feature. Because the error is handled twice if you handle it explicitly through subscribe({ error: ... }).

I released a bug fix and it's available under 3.7.5-dev.master-b4f4da3 version. This enables the user to handle errors w/o calling to ErrorHandler.

[arturovt]

This has been fixed as part of the v3.7.6 release.
Please test this out and let us know if this is not resolved.