feat(effects): resubscribe to effects on error

modules/effects/spec/effect_creator.spec.ts

@@ -58,7 +58,7 @@ describe('createEffect()', () => {
         expectSnippet(`
           const effect = createEffect(() => ({ foo: 'a' }), { dispatch: false });
         `).toFail(
-          /Type '{ foo: string; }' is not assignable to type 'Observable<Action> | ((...args: any[]) => Observable<Action>)'./
+          /Type '{ foo: string; }' is not assignable to type 'Observable<unknown> | ((...args: any[]) => Observable<unknown>)'./
         );
       });
     });
@@ -73,31 +73,39 @@ describe('createEffect()', () => {
   it('should dispatch by default', () => {
     const effect: any = createEffect(() => of({ type: 'a' }));
 
-    expect(effect['__@ngrx/effects_create__']).toEqual({ dispatch: true });
+    expect(effect['__@ngrx/effects_create__']).toEqual(
+      jasmine.objectContaining({ dispatch: true })
+    );
   });
 
   it('should be possible to explicitly create a dispatching effect', () => {
     const effect: any = createEffect(() => of({ type: 'a' }), {
       dispatch: true,
     });
 
-    expect(effect['__@ngrx/effects_create__']).toEqual({ dispatch: true });
+    expect(effect['__@ngrx/effects_create__']).toEqual(
+      jasmine.objectContaining({ dispatch: true })
+    );
   });
 
   it('should be possible to create a non-dispatching effect', () => {
     const effect: any = createEffect(() => of({ type: 'a' }), {
       dispatch: false,
     });
 
-    expect(effect['__@ngrx/effects_create__']).toEqual({ dispatch: false });
+    expect(effect['__@ngrx/effects_create__']).toEqual(
+      jasmine.objectContaining({ dispatch: false })
+    );
   });
 
   it('should be possible to create a non-dispatching effect returning a non-action', () => {
     const effect: any = createEffect(() => of('foo'), {
       dispatch: false,
     });
 
-    expect(effect['__@ngrx/effects_create__']).toEqual({ dispatch: false });
+    expect(effect['__@ngrx/effects_create__']).toEqual(
+      jasmine.objectContaining({ dispatch: false })
+    );
   });
 
   describe('getCreateEffectMetadata', () => {
@@ -106,14 +114,30 @@ 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 });
+        e = createEffect(() => of({ type: 'd' }), {
+          resubscribeOnError: false,
+        });
+        f = createEffect(() => of({ type: 'e' }), {
+          dispatch: false,
+          resubscribeOnError: false,
+        });
+        g = createEffect(() => of({ type: 'e' }), {
+          dispatch: true,
+          resubscribeOnError: false,
+        });
       }
 
       const mock = new Fixture();
 
       expect(getCreateEffectMetadata(mock)).toEqual([
-        { propertyName: 'a', dispatch: true },
-        { propertyName: 'b', dispatch: true },
-        { propertyName: 'c', dispatch: false },
+        { 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 },
       ]);
     });
 

modules/effects/spec/effect_decorator.spec.ts

@@ -5,17 +5,30 @@ describe('@Effect()', () => {
     it('should get the effects metadata for a class instance', () => {
       class Fixture {
         @Effect() a: any;
-        @Effect() b: any;
+        @Effect({ dispatch: true })
+        b: any;
         @Effect({ dispatch: false })
         c: any;
+        @Effect({ resubscribeOnError: true })
+        d: any;
+        @Effect({ resubscribeOnError: false })
+        e: any;
+        @Effect({ dispatch: false, resubscribeOnError: false })
+        f: any;
+        @Effect({ dispatch: true, resubscribeOnError: false })
+        g: any;
       }
 
       const mock = new Fixture();
 
       expect(getEffectDecoratorMetadata(mock)).toEqual([
-        { propertyName: 'a', dispatch: true },
-        { propertyName: 'b', dispatch: true },
-        { propertyName: 'c', dispatch: false },
+        { 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 },
       ]);
     });
 

modules/effects/spec/effect_sources.spec.ts

@@ -1,8 +1,8 @@
 import { ErrorHandler } from '@angular/core';
 import { TestBed } from '@angular/core/testing';
-import { cold, getTestScheduler } from 'jasmine-marbles';
+import { cold, hot, getTestScheduler } from 'jasmine-marbles';
 import { concat, NEVER, Observable, of, throwError, timer } from 'rxjs';
-import { map } from 'rxjs/operators';
+import { concatMap, map } from 'rxjs/operators';
 
 import {
   Effect,
@@ -235,6 +235,54 @@ describe('EffectSources', () => {
         );
       });
 
+      it('should report an error if an effect throws one', () => {
+        const sources$ = of(new SourceError());
+
+        toActions(sources$).subscribe();
+
+        expect(mockErrorReporter.handleError).toHaveBeenCalledWith(
+          new Error('An Error')
+        );
+      });
+
+      it('should resubscribe on error by default', () => {
+        class Eff {
+          @Effect()
+          b$ = hot('a--b--c--d').pipe(
+            map(v => {
+              if (v == 'b') throw new Error('An Error');
+              return v;
+            })
+          );
+        }
+
+        const sources$ = of(new Eff());
+
+        //                       👇 'b' is ignored.
+        const expected = cold('a-----c--d');
+
+        expect(toActions(sources$)).toBeObservable(expected);
+      });
+
+      it('should not resubscribe on error when resubscribeOnError is false', () => {
+        class Eff {
+          @Effect({ resubscribeOnError: false })
+          b$ = hot('a--b--c--d').pipe(
+            map(v => {
+              if (v == 'b') throw new Error('An Error');
+              return v;
+            })
+          );
+        }
+
+        const sources$ = of(new Eff());
+
+        //                       👇 completes.
+        const expected = cold('a--|');
+
+        expect(toActions(sources$)).toBeObservable(expected);
+      });
+
       it(`should not break when the action in the error message can't be stringified`, () => {
         const sources$ = of(new SourceG());
 
@@ -454,6 +502,77 @@ describe('EffectSources', () => {
         );
       });
 
+      it('should report an error if an effect throws one', () => {
+        const sources$ = of(new SourceError());
+
+        toActions(sources$).subscribe();
+
+        expect(mockErrorReporter.handleError).toHaveBeenCalledWith(
+          new Error('An Error')
+        );
+      });
+
+      it('should resubscribe on error by default', () => {
+        const sources$ = of(
+          new class {
+            b$ = createEffect(() =>
+              hot('a--b--c--d').pipe(
+                map(v => {
+                  if (v == 'b') throw new Error('An Error');
+                  return v;
+                })
+              )
+            );
+          }()
+        );
+        //                       👇 'b' is ignored.
+        const expected = cold('a-----c--d');
+
+        expect(toActions(sources$)).toBeObservable(expected);
+      });
+
+      it('should resubscribe on error by default when dispatch is false', () => {
+        const sources$ = of(
+          new class {
+            b$ = createEffect(
+              () =>
+                hot('a--b--c--d').pipe(
+                  map(v => {
+                    if (v == 'b') throw new Error('An Error');
+                    return v;
+                  })
+                ),
+              { dispatch: false }
+            );
+          }()
+        );
+        //                    👇 doesn't complete and doesn't dispatch
+        const expected = cold('----------');
+
+        expect(toActions(sources$)).toBeObservable(expected);
+      });
+
+      it('should not resubscribe on error when resubscribeOnError is false', () => {
+        const sources$ = of(
+          new class {
+            b$ = createEffect(
+              () =>
+                hot('a--b--c--d').pipe(
+                  map(v => {
+                    if (v == 'b') throw new Error('An Error');
+                    return v;
+                  })
+                ),
+              { dispatch: false, resubscribeOnError: false }
+            );
+          }()
+        );
+        //                       👇 errors with dispatch false
+        const expected = cold('---#', undefined, new Error('An Error'));
+
+        expect(toActions(sources$)).toBeObservable(expected);
+      });
+
       it(`should not break when the action in the error message can't be stringified`, () => {
         const sources$ = of(new SourceG());
 

modules/effects/spec/effects_metadata.spec.ts

@@ -1,6 +1,7 @@
 import { getEffectsMetadata, getSourceMetadata } from '../src/effects_metadata';
 import { of } from 'rxjs';
 import { Effect, createEffect } from '..';
+import { EffectMetadata } from '../src/models';
 
 describe('Effects metadata', () => {
   describe('getSourceMetadata', () => {
@@ -11,17 +12,24 @@ describe('Effects metadata', () => {
         @Effect({ dispatch: false })
         c: any;
         d = createEffect(() => of({ type: 'a' }), { dispatch: false });
+        @Effect({ dispatch: false, resubscribeOnError: 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 },
+      ];
 
-      expect(getSourceMetadata(mock)).toEqual([
-        { propertyName: 'a', dispatch: true },
-        { propertyName: 'c', dispatch: false },
-        { propertyName: 'b', dispatch: true },
-        { propertyName: 'd', dispatch: false },
-      ]);
+      expect(getSourceMetadata(mock)).toEqual(
+        jasmine.arrayContaining(expected)
+      );
+      expect(getSourceMetadata(mock).length).toEqual(expected.length);
     });
   });
 
@@ -36,17 +44,21 @@ describe('Effects metadata', () => {
         @Effect({ dispatch: false })
         e: any;
         f = createEffect(() => of({ type: 'f' }), { dispatch: false });
+        g = createEffect(() => of({ type: 'f' }), {
+          resubscribeOnError: false,
+        });
       }
 
       const mock = new Fixture();
 
       expect(getEffectsMetadata(mock)).toEqual({
-        a: { dispatch: true },
-        c: { dispatch: true },
-        e: { dispatch: false },
-        b: { dispatch: true },
-        d: { dispatch: true },
-        f: { dispatch: false },
+        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 },
       });
     });
 

modules/effects/src/effect_creator.ts

@@ -1,25 +1,27 @@
 import { Observable } from 'rxjs';
 import { Action } from '@ngrx/store';
-import { EffectMetadata } from './models';
+import { EffectMetadata, EffectConfig } from './models';
 
 const CREATE_EFFECT_METADATA_KEY = '__@ngrx/effects_create__';
 
+type DispatchType<T> = T extends { dispatch: infer U } ? U : unknown;
 export function createEffect<
-  R extends Observable<unknown> | ((...args: any[]) => Observable<unknown>)
->(source: () => R, options: { dispatch: false }): R;
-export function createEffect<
-  T extends Action,
-  R extends Observable<T> | ((...args: any[]) => Observable<T>)
->(source: () => R, options?: { dispatch: true }): R;
-export function createEffect<
-  T extends Action,
-  R extends Observable<T> | ((...args: any[]) => Observable<T>)
->(source: () => R, { dispatch = true } = {}): R {
+  C extends EffectConfig,
+  T extends DispatchType<C>,
+  O extends T extends false ? Observable<unknown> : Observable<Action>,
+  R extends O | ((...args: any[]) => O)
+>(source: () => R, config?: Partial<C>): R {
   const effect = source();
+  // Right now both createEffect and @Effect decorator set default values.
+  // Ideally that should only be done in one place that aggregates that info,
+  // for example in mergeEffects().
+  const value: EffectConfig = {
+    dispatch: true,
+    resubscribeOnError: true,
+    ...config, // Overrides any defaults if values are provided
+  };
   Object.defineProperty(effect, CREATE_EFFECT_METADATA_KEY, {
-    value: {
-      dispatch,
-    },
+    value,
   });
   return effect;
 }

modules/effects/src/effect_decorator.ts

@@ -4,12 +4,22 @@ import { getSourceForInstance } from './utils';
 
 const METADATA_KEY = '__@ngrx/effects__';
 
-export function Effect<T>({ dispatch = true } = {}): PropertyDecorator {
+export function Effect<T>({
+  dispatch = true,
+  resubscribeOnError = true,
+} = {}): PropertyDecorator {
   return function<K extends Extract<keyof T, string>>(
     target: T,
     propertyName: K
   ) {
-    const metadata: EffectMetadata<T> = { propertyName, dispatch };
+    // Right now both createEffect and @Effect decorator set default values.
+    // Ideally that should only be done in one place that aggregates that info,
+    // for example in mergeEffects().
+    const metadata: EffectMetadata<T> = {
+      propertyName,
+      dispatch,
+      resubscribeOnError,
+    };
     setEffectMetadataEntries<T>(target, [metadata]);
   } as (target: {}, propertyName: string | symbol) => void;
 }