feat(effects): make resubscription handler customizable

BREAKING CHANGE: `resubscribeOnError` renamed to `useEffectsErrorHandler` in `createEffect` metadata Closes ngrx#2294
zakhenry · Jan 13, 2020 · d3ce151 · d3ce151
1 parent f17589f
commit d3ce151
Show file tree

Hide file tree

Showing 17 changed files with 302 additions and 70 deletions.
diff --git a/docs/effects/api.md b/docs/effects/api.md
@@ -204,6 +204,62 @@ export class UserEffects implements OnRunEffects {
 }
 ```
 
+### EffectsErrorHandler
+
+By default, if an effect has `{useEffectsErrorHandler: true}` (the effect metadata default), when the effect encounters an error it
+is automatically resubscribed to, and the Angular `ErrorHandler.handleError` method is called with the error, and the
+effect observable resubscribed to.
+
+If you want to customize this behavior, for example if you have a [custom error handler](https://angular.io/api/core/ErrorHandler) that needs specific input, or you
+only want to resubscribe on certain errors etc, you may provide a custom handler using the `EFFECTS_ERROR_HANDLER`
+injection token.
+
+Usage:
+
+```ts
+import { EffectsModule, EFFECTS_ERROR_HANDLER } from '@ngrx/effects';
+import { MovieEffects } from './effects/movie.effects';
+import { CustomErrorHandler, isRetryable } from '../custom-error-handler';
+import { Action } from '@ngrx/store';
+import { Observable, throwError } from 'rxjs';
+import { retryWhen, mergeMap } from 'rxjs/operators';
+
+export function effectResubscriptionHandler<T extends Action>(
+  observable$: Observable<T>,
+  errorHandler?: CustomErrorHandler
+): Observable<T> {
+  return observable$.pipe(
+    retryWhen(errors =>
+      errors.pipe(
+        mergeMap(e => {
+          if (isRetryable(e)) {
+            return errorHandler.handleRetryableError(e);
+          }
+
+          errorHandler.handleError(e);
+          return throwError(e);
+        })
+      )
+    )
+  );
+}
+
+@NgModule({
+  imports: [EffectsModule.forRoot([MovieEffects])],
+  providers: [
+    {
+      provide: EFFECTS_ERROR_HANDLER,
+      useValue: effectResubscriptionHandler,
+    },
+    {
+      provide: ErrorHandle,
+      useClass: CustomErrorHandler,
+    },
+  ],
+})
+export class AppModule {}
+```
+
 ## Utilities
 
 ### mergeEffects

diff --git a/modules/effects/spec/effect_creator.spec.ts b/modules/effects/spec/effect_creator.spec.ts
@@ -52,30 +52,32 @@ describe('createEffect()', () => {
         a = createEffect(() => of({ type: 'a' }));
         b = createEffect(() => of({ type: 'b' }), { dispatch: true });
         c = createEffect(() => of({ type: 'c' }), { dispatch: false });
-        d = createEffect(() => of({ type: 'd' }), { resubscribeOnError: true });
+        d = createEffect(() => of({ type: 'd' }), {
+          useEffectsErrorHandler: true,
+        });
         e = createEffect(() => of({ type: 'd' }), {
-          resubscribeOnError: false,
+          useEffectsErrorHandler: false,
         });
         f = createEffect(() => of({ type: 'e' }), {
           dispatch: false,
-          resubscribeOnError: false,
+          useEffectsErrorHandler: false,
         });
         g = createEffect(() => of({ type: 'e' }), {
           dispatch: true,
-          resubscribeOnError: false,
+          useEffectsErrorHandler: false,
         });
       }
 
       const mock = new Fixture();
 
       expect(getCreateEffectMetadata(mock)).toEqual([
-        { propertyName: 'a', dispatch: true, resubscribeOnError: true },
-        { propertyName: 'b', dispatch: true, resubscribeOnError: true },
-        { propertyName: 'c', dispatch: false, resubscribeOnError: true },
-        { propertyName: 'd', dispatch: true, resubscribeOnError: true },
-        { propertyName: 'e', dispatch: true, resubscribeOnError: false },
-        { propertyName: 'f', dispatch: false, resubscribeOnError: false },
-        { propertyName: 'g', dispatch: true, resubscribeOnError: false },
+        { propertyName: 'a', dispatch: true, useEffectsErrorHandler: true },
+        { propertyName: 'b', dispatch: true, useEffectsErrorHandler: true },
+        { propertyName: 'c', dispatch: false, useEffectsErrorHandler: true },
+        { propertyName: 'd', dispatch: true, useEffectsErrorHandler: true },
+        { propertyName: 'e', dispatch: true, useEffectsErrorHandler: false },
+        { propertyName: 'f', dispatch: false, useEffectsErrorHandler: false },
+        { propertyName: 'g', dispatch: true, useEffectsErrorHandler: false },
       ]);
     });
 

diff --git a/modules/effects/spec/effect_decorator.spec.ts b/modules/effects/spec/effect_decorator.spec.ts
@@ -9,26 +9,26 @@ describe('@Effect()', () => {
         b: any;
         @Effect({ dispatch: false })
         c: any;
-        @Effect({ resubscribeOnError: true })
+        @Effect({ useEffectsErrorHandler: true })
         d: any;
-        @Effect({ resubscribeOnError: false })
+        @Effect({ useEffectsErrorHandler: false })
         e: any;
-        @Effect({ dispatch: false, resubscribeOnError: false })
+        @Effect({ dispatch: false, useEffectsErrorHandler: false })
         f: any;
-        @Effect({ dispatch: true, resubscribeOnError: false })
+        @Effect({ dispatch: true, useEffectsErrorHandler: false })
         g: any;
       }
 
       const mock = new Fixture();
 
       expect(getEffectDecoratorMetadata(mock)).toEqual([
-        { propertyName: 'a', dispatch: true, resubscribeOnError: true },
-        { propertyName: 'b', dispatch: true, resubscribeOnError: true },
-        { propertyName: 'c', dispatch: false, resubscribeOnError: true },
-        { propertyName: 'd', dispatch: true, resubscribeOnError: true },
-        { propertyName: 'e', dispatch: true, resubscribeOnError: false },
-        { propertyName: 'f', dispatch: false, resubscribeOnError: false },
-        { propertyName: 'g', dispatch: true, resubscribeOnError: false },
+        { propertyName: 'a', dispatch: true, useEffectsErrorHandler: true },
+        { propertyName: 'b', dispatch: true, useEffectsErrorHandler: true },
+        { propertyName: 'c', dispatch: false, useEffectsErrorHandler: true },
+        { propertyName: 'd', dispatch: true, useEffectsErrorHandler: true },
+        { propertyName: 'e', dispatch: true, useEffectsErrorHandler: false },
+        { propertyName: 'f', dispatch: false, useEffectsErrorHandler: false },
+        { propertyName: 'g', dispatch: true, useEffectsErrorHandler: false },
       ]);
     });
 

diff --git a/modules/effects/spec/effect_sources.spec.ts b/modules/effects/spec/effect_sources.spec.ts
@@ -263,9 +263,9 @@ describe('EffectSources', () => {
         expect(toActions(sources$)).toBeObservable(expected);
       });
 
-      it('should not resubscribe on error when resubscribeOnError is false', () => {
+      it('should not resubscribe on error when useEffectsErrorHandler is false', () => {
         class Eff {
-          @Effect({ resubscribeOnError: false })
+          @Effect({ useEffectsErrorHandler: false })
           b$ = hot('a--b--c--d').pipe(
             map(v => {
               if (v == 'b') throw new Error('An Error');
@@ -552,7 +552,7 @@ describe('EffectSources', () => {
         expect(toActions(sources$)).toBeObservable(expected);
       });
 
-      it('should not resubscribe on error when resubscribeOnError is false', () => {
+      it('should not resubscribe on error when useEffectsErrorHandler is false', () => {
         const sources$ = of(
           new class {
             b$ = createEffect(
@@ -563,7 +563,7 @@ describe('EffectSources', () => {
                     return v;
                   })
                 ),
-              { dispatch: false, resubscribeOnError: false }
+              { dispatch: false, useEffectsErrorHandler: false }
             );
           }()
         );

diff --git a/modules/effects/spec/effects_error_handler.spec.ts b/modules/effects/spec/effects_error_handler.spec.ts
@@ -0,0 +1,92 @@
+import { ErrorHandler, Provider } from '@angular/core';
+import { TestBed } from '@angular/core/testing';
+import { Action, Store } from '@ngrx/store';
+import { Observable, of } from 'rxjs';
+import { catchError } from 'rxjs/operators';
+import { createEffect, EFFECTS_ERROR_HANDLER, EffectsModule } from '..';
+
+describe('NgRx Effects Error Handler spec', () => {
+  let subscriptionCount: number;
+  let globalErrorHandler: jasmine.Spy;
+  let storeNext: jasmine.Spy;
+
+  function makeEffectTestBed(...providers: Provider[]) {
+    subscriptionCount = 0;
+
+    TestBed.configureTestingModule({
+      imports: [EffectsModule.forRoot([ErrorEffect])],
+      providers: [
+        {
+          provide: Store,
+          useValue: {
+            next: jasmine.createSpy('storeNext'),
+          },
+        },
+        {
+          provide: ErrorHandler,
+          useValue: {
+            handleError: jasmine.createSpy('globalErrorHandler'),
+          },
+        },
+        ...providers,
+      ],
+    });
+
+    globalErrorHandler = TestBed.get(ErrorHandler).handleError;
+    const store = TestBed.get(Store);
+    storeNext = store.next;
+  }
+
+  it('should retry and notify error handler when effect error handler is not provided', () => {
+    makeEffectTestBed();
+
+    expect(subscriptionCount).toBe(2);
+    expect(globalErrorHandler).toHaveBeenCalledWith(new Error('effectError'));
+  });
+
+  fit('should use custom error behavior when EFFECTS_ERROR_HANDLER is provided', () => {
+    const effectsErrorHandlerSpy = jasmine
+      .createSpy()
+      .and.callFake((effect$: Observable<any>, errorHandler: ErrorHandler) => {
+        return effect$.pipe(
+          catchError(err => {
+            errorHandler.handleError(
+              new Error('inside custom handler: ' + err.message)
+            );
+            return of({ type: 'custom action' });
+          })
+        );
+      });
+
+    makeEffectTestBed({
+      provide: EFFECTS_ERROR_HANDLER,
+      useValue: effectsErrorHandlerSpy,
+    });
+
+    expect(effectsErrorHandlerSpy).toHaveBeenCalledWith(
+      jasmine.any(Observable),
+      TestBed.get(ErrorHandler)
+    );
+    expect(globalErrorHandler).toHaveBeenCalledWith(
+      new Error('inside custom handler: effectError')
+    );
+    expect(subscriptionCount).toBe(1);
+    expect(storeNext).toHaveBeenCalledWith({ type: 'custom action' });
+  });
+
+  class ErrorEffect {
+    effect$ = createEffect(errorFirstSubscriber, {
+      useEffectsErrorHandler: true,
+    });
+  }
+
+  function errorFirstSubscriber(): Observable<Action> {
+    return new Observable(observer => {
+      subscriptionCount++;
+
+      if (subscriptionCount === 1) {
+        observer.error(new Error('effectError'));
+      }
+    });
+  }
+});
diff --git a/modules/effects/spec/effects_metadata.spec.ts b/modules/effects/spec/effects_metadata.spec.ts
@@ -12,18 +12,18 @@ describe('Effects metadata', () => {
         @Effect({ dispatch: false })
         c: any;
         d = createEffect(() => of({ type: 'a' }), { dispatch: false });
-        @Effect({ dispatch: false, resubscribeOnError: false })
+        @Effect({ dispatch: false, useEffectsErrorHandler: false })
         e: any;
         z: any;
       }
 
       const mock = new Fixture();
       const expected: EffectMetadata<Fixture>[] = [
-        { propertyName: 'a', dispatch: true, resubscribeOnError: true },
-        { propertyName: 'c', dispatch: false, resubscribeOnError: true },
-        { propertyName: 'b', dispatch: true, resubscribeOnError: true },
-        { propertyName: 'd', dispatch: false, resubscribeOnError: true },
-        { propertyName: 'e', dispatch: false, resubscribeOnError: false },
+        { propertyName: 'a', dispatch: true, useEffectsErrorHandler: true },
+        { propertyName: 'c', dispatch: false, useEffectsErrorHandler: true },
+        { propertyName: 'b', dispatch: true, useEffectsErrorHandler: true },
+        { propertyName: 'd', dispatch: false, useEffectsErrorHandler: true },
+        { propertyName: 'e', dispatch: false, useEffectsErrorHandler: false },
       ];
 
       expect(getSourceMetadata(mock)).toEqual(
@@ -45,20 +45,20 @@ describe('Effects metadata', () => {
         e: any;
         f = createEffect(() => of({ type: 'f' }), { dispatch: false });
         g = createEffect(() => of({ type: 'g' }), {
-          resubscribeOnError: false,
+          useEffectsErrorHandler: false,
         });
       }
 
       const mock = new Fixture();
 
       expect(getEffectsMetadata(mock)).toEqual({
-        a: { dispatch: true, resubscribeOnError: true },
-        c: { dispatch: true, resubscribeOnError: true },
-        e: { dispatch: false, resubscribeOnError: true },
-        b: { dispatch: true, resubscribeOnError: true },
-        d: { dispatch: true, resubscribeOnError: true },
-        f: { dispatch: false, resubscribeOnError: true },
-        g: { dispatch: true, resubscribeOnError: false },
+        a: { dispatch: true, useEffectsErrorHandler: true },
+        c: { dispatch: true, useEffectsErrorHandler: true },
+        e: { dispatch: false, useEffectsErrorHandler: true },
+        b: { dispatch: true, useEffectsErrorHandler: true },
+        d: { dispatch: true, useEffectsErrorHandler: true },
+        f: { dispatch: false, useEffectsErrorHandler: true },
+        g: { dispatch: true, useEffectsErrorHandler: false },
       });
     });
 

diff --git a/modules/effects/src/effect_creator.ts b/modules/effects/src/effect_creator.ts
@@ -15,7 +15,7 @@ type ObservableType<T, OriginalType> = T extends false ? OriginalType : Action;
  * Creates an effect from an `Observable` and an `EffectConfig`.
  *
  * @param source A function which returns an `Observable`.
- * @param config A `Partial<EffectConfig>` to configure the effect.  By default, `dispatch` is true and `resubscribeOnError` is true.
+ * @param config A `Partial<EffectConfig>` to configure the effect.  By default, `dispatch` is true and `useEffectsErrorHandler` is true.
  * @returns If `EffectConfig`#`dispatch` is true, returns `Observable<Action>`.  Else, returns `Observable<unknown>`.
  *
  * @usageNotes

diff --git a/modules/effects/src/effect_sources.ts b/modules/effects/src/effect_sources.ts
@@ -1,4 +1,4 @@
-import { ErrorHandler, Injectable } from '@angular/core';
+import { ErrorHandler, Inject, Injectable, Optional } from '@angular/core';
 import { Action, Store } from '@ngrx/store';
 import { Notification, Observable, Subject } from 'rxjs';
 import {
@@ -14,18 +14,25 @@ import {
   reportInvalidActions,
   EffectNotification,
 } from './effect_notification';
-import { mergeEffects } from './effects_resolver';
+import { mergeEffects, EffectsErrorHandler } from './effects_resolver';
 import {
   onIdentifyEffectsKey,
   onRunEffectsKey,
   OnRunEffects,
   onInitEffects,
 } from './lifecycle_hooks';
+import { EFFECTS_ERROR_HANDLER } from './tokens';
 import { getSourceForInstance } from './utils';
 
 @Injectable()
 export class EffectSources extends Subject<any> {
-  constructor(private errorHandler: ErrorHandler, private store: Store<any>) {
+  constructor(
+    private errorHandler: ErrorHandler,
+    private store: Store<any>,
+    @Optional()
+    @Inject(EFFECTS_ERROR_HANDLER)
+    private effectsErrorHandler: EffectsErrorHandler | null
+  ) {
     super();
   }
 
@@ -49,7 +56,12 @@ export class EffectSources extends Subject<any> {
       mergeMap(source$ => source$.pipe(groupBy(effectsInstance))),
       mergeMap(source$ =>
         source$.pipe(
-          exhaustMap(resolveEffectSource(this.errorHandler)),
+          exhaustMap(
+            resolveEffectSource(
+              this.errorHandler,
+              this.effectsErrorHandler || undefined
+            )
+          ),
           map(output => {
             reportInvalidActions(output, this.errorHandler);
             return output.notification;
@@ -77,10 +89,15 @@ function effectsInstance(sourceInstance: any) {
 }
 
 function resolveEffectSource(
-  errorHandler: ErrorHandler
+  errorHandler: ErrorHandler,
+  effectsErrorHandler?: EffectsErrorHandler
 ): (sourceInstance: any) => Observable<EffectNotification> {
   return sourceInstance => {
-    const mergedEffects$ = mergeEffects(sourceInstance, errorHandler);
+    const mergedEffects$ = mergeEffects(
+      sourceInstance,
+      errorHandler,
+      effectsErrorHandler
+    );
 
     if (isOnRunEffects(sourceInstance)) {
       return sourceInstance.ngrxOnRunEffects(mergedEffects$);