RFC: Ability to customise resubscribeInCaseOfError functionality to allow for user interaction with errors

[zakhenry]

Currently the resubscribeInCaseOfError function is not configurable, and it simply calls errorHandler.handleError(error) and resubscribes. This behavior is not sufficient in the case that the application has a custom error handler that allows users to decide whether or not to retry the failure.

The cleanest solution would likely be to provide the resubscribeInCaseOfError function using an injected symbol, which would allow downstream consumers to do something along the lines of the following:

{
  provide: RESUBSCRIBE_ON_ERROR_HANDLER,
  deps: [MyCustomErrorHandler],
  useFactory: (errorHandler: ErrorHander) => {
    return (observable$) => observable$.pipe(
      retryWhen(errors =>
        errors.pipe(
          switchMap((e: IsRetryableError) => {

            if (e.isRetryable) {
              return errorHandler.handleRecoverableError(e)
            }

            errorHandler.handleError(e);
            return throwError(e);
          }),
        ),
      ),
    );
  },
},

Describe any alternatives/workarounds you're currently using

Using a custom rxjs operator for every effect

Other information:

If accepted, I would be willing to submit a PR for this feature

[x] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

[timdeschryver]

The resubscribeInCaseOfError was added as a safety for when an Effect stream would complete (and thus not receiving new actions) when an inner stream error occurred without catching the error with catchError.

I think a custom RxJS operator would be more suitable if you want this customized.

[zakhenry]

@timdeschryver yep I understand the usefulness of resubscribeInCaseOfError, but in my case I don't want to have to remember to explicitly add the custom rxjs operator to the ~150 existing and future effects - rather I'd like to attach the error handling to my global error handler that is able to selectively decide whether or not to retry a behavior (it opens an error dialog that the user can choose to retry or not)

[zakhenry]

I've created a draft PR that I've verified works with my use case, I'd like feedback on the general strategy before I add tests for this use case as there's a significant amount of tests missing in the area of modules/effects/src/effects_resolver.ts

platform/modules/effects/spec/effects_resolver.spec.ts

Line 1 in de703f0

describe('mergeEffects', () => {});

[alex-okrushko]

Would providing custom ErrorHandler be not enough?

[zakhenry]

@alex-okrushko that was my initial thought but no actually - the existing implementation only invokes the handleError method of the error handler, not allowing a custom error handler to control the subscription in order to retry (or not).

platform/modules/effects/src/effects_resolver.ts

Lines 54 to 65 in de703f0

	function resubscribeInCaseOfError<T extends Action>(
	observable$: Observable<T>,
	errorHandler?: ErrorHandler
	): Observable<T> {
	return observable$.pipe(
	catchError(error => {
	if (errorHandler) errorHandler.handleError(error);
	// Return observable that produces this particular effect
	return resubscribeInCaseOfError(observable$, errorHandler);
	})
	);
	}

[alex-okrushko]

Here are my thoughts on this:

• I would definitely prefer to avoid additional tokens, since that unnecessarily increases API surface, puts additional burden on maintainability and limits flexibility for future potential refactoring
• If one needs to conditionally prevent re-subscription there are at least two options:
  • Handle the error within the effect itself (use catchError yourself) - good for one-time use
  • Create your higher-order function that would wrap createEffect function and append the catchError with whatever custom logic is needed - good for reusability

[timdeschryver]

I'm going to close this issue because we share the same thoughts here @alex-okrushko .
We can re-evaluate later if this pops up again.

(or if @brandonroberts thinks otherwise)

[zakhenry]

@alex-okrushko I disagree with your second statement, which is pivotal to the point of this issue and associated PR.

I understand the desire to keep the API surface minimal, however we're in a position that the only options available to us are cumbersome and error prone, or wrapping the ngrx framework in something that is usable for our needs. To me this tells me that there is room for minor customisation of behavior to support custom error handling.

The latest update that introduces resubscription represents a breaking change, which can cause very unexpected failure conditions and infinite loops. What is worse is that there is no safe way to escape from this failure in a generic fashion, rather developers have to remember to explicitly handle this case every time this can happen, or as you say not use the ngrx api at all in userland code and wrap it to make it safer. Neither of these options are good, and I'm disappointed that a call to add robust error handling customisation, with an accompanying example PR is dismissed without further discussion.

[zakhenry]

Also regarding wrapping createEffect is undesirable in my specific situation as we use the @Effect decorator currently (173 usages) and I'm really not keen to have to constantly tell developers to not follow the ngrx documentation because it has unsafe error handling behavior

[tyroneneill]

It seems to be a trade-off between customisation and API surface area, as a framework user I would be far more frustrated not being able to change the behaviour rather than having to deal with the consequences of a large API.

This will be the second large project using ngrx I will have had to monkey-patch to get the desired behaviour.

[alex-okrushko]

@zakhenry @tyroneneill
Ok, let's reopen this one.
I'm think that EFFECTS_ERROR_HANDLER might be a better name for this token - it would be up to the user to determine if it should be resubscribed or not (and if so, then after how many attempts) or any other custom logic.

Now the resubscribeOnError Effect config won't sound good, if such token is provided... it probably should be named as useEffectsErrorHandler or something like that.

[timdeschryver]

Apologies, perhaps I was too eager to close this issue now that I've read the responses.
How would this overlap with #2303, do we need #2303 after this change?

My understanding is that #2303 adds the number of retries in the default behavior (in dev mode?) - if a error handler is provided the NgRx effect uses the custom handler (thus will not retry automatically). I think this is also what you meant, right @alex-okrushko ?

[zakhenry]

@timdeschryver Hmm I'm not sure that this fix would directly resolve that issue, however it would at least provide the mechanism for downstream projects to implement this behavior. I suppose a strategy might be to introduce the customisation capability, then later add a more developer friendly devmode-only implementation as the default behavior?

[zakhenry]

@alex-okrushko I agree with you on the naming not really making sense now, do you think the change should be done now (and make this a breaking change for 9.x)? Given 9.x seems to be just around the corner that might be practical

[alex-okrushko]

Breaking change should be fine. Surprisingly (or maybe not so much), I haven't found even a single case where resubscribeOnError was turned off within Google's codebase.

[zakhenry]

#2295 has now been updated to incorporate the breaking change and now includes tests.