diff --git a/modules/effects/spec/effect_creator.spec.ts b/modules/effects/spec/effect_creator.spec.ts index 5febe71523..899a5538c3 100644 --- a/modules/effects/spec/effect_creator.spec.ts +++ b/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 | ((...args: any[]) => Observable)'./ + /Type '{ foo: string; }' is not assignable to type 'Observable | ((...args: any[]) => Observable)'./ ); }); }); @@ -73,7 +73,9 @@ 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', () => { @@ -81,7 +83,9 @@ describe('createEffect()', () => { 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', () => { @@ -89,7 +93,9 @@ describe('createEffect()', () => { 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', () => { @@ -97,7 +103,9 @@ describe('createEffect()', () => { 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 }, ]); }); diff --git a/modules/effects/spec/effect_decorator.spec.ts b/modules/effects/spec/effect_decorator.spec.ts index 5a8183edf2..7b91245a98 100644 --- a/modules/effects/spec/effect_decorator.spec.ts +++ b/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 }, ]); }); diff --git a/modules/effects/spec/effect_sources.spec.ts b/modules/effects/spec/effect_sources.spec.ts index 114ff55fb4..0ed8af2ac0 100644 --- a/modules/effects/spec/effect_sources.spec.ts +++ b/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()); diff --git a/modules/effects/spec/effects_metadata.spec.ts b/modules/effects/spec/effects_metadata.spec.ts index 6162770689..5cc2e141e0 100644 --- a/modules/effects/spec/effects_metadata.spec.ts +++ b/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[] = [ + { 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: 'g' }), { + 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 }, }); }); diff --git a/modules/effects/src/effect_creator.ts b/modules/effects/src/effect_creator.ts index 739b1b33a4..575ee0e068 100644 --- a/modules/effects/src/effect_creator.ts +++ b/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 extends { dispatch: infer U } ? U : unknown; export function createEffect< - R extends Observable | ((...args: any[]) => Observable) ->(source: () => R, options: { dispatch: false }): R; -export function createEffect< - T extends Action, - R extends Observable | ((...args: any[]) => Observable) ->(source: () => R, options?: { dispatch: true }): R; -export function createEffect< - T extends Action, - R extends Observable | ((...args: any[]) => Observable) ->(source: () => R, { dispatch = true } = {}): R { + C extends EffectConfig, + T extends DispatchType, + O extends T extends false ? Observable : Observable, + R extends O | ((...args: any[]) => O) +>(source: () => R, config?: Partial): 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; } diff --git a/modules/effects/src/effect_decorator.ts b/modules/effects/src/effect_decorator.ts index 82c87a8671..7b12251678 100644 --- a/modules/effects/src/effect_decorator.ts +++ b/modules/effects/src/effect_decorator.ts @@ -1,15 +1,25 @@ import { compose } from '@ngrx/store'; -import { EffectMetadata } from './models'; +import { EffectMetadata, EffectConfig } from './models'; import { getSourceForInstance } from './utils'; const METADATA_KEY = '__@ngrx/effects__'; -export function Effect({ dispatch = true } = {}): PropertyDecorator { +export function Effect({ + dispatch = true, + resubscribeOnError = true, +}: EffectConfig = {}): PropertyDecorator { return function>( target: T, propertyName: K ) { - const metadata: EffectMetadata = { 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 = { + propertyName, + dispatch, + resubscribeOnError, + }; setEffectMetadataEntries(target, [metadata]); } as (target: {}, propertyName: string | symbol) => void; } diff --git a/modules/effects/src/effect_notification.ts b/modules/effects/src/effect_notification.ts index 439166b9b6..e2945d8ce8 100644 --- a/modules/effects/src/effect_notification.ts +++ b/modules/effects/src/effect_notification.ts @@ -10,21 +10,7 @@ export interface EffectNotification { notification: Notification; } -export function verifyOutput( - output: EffectNotification, - reporter: ErrorHandler -) { - reportErrorThrown(output, reporter); - reportInvalidActions(output, reporter); -} - -function reportErrorThrown(output: EffectNotification, reporter: ErrorHandler) { - if (output.notification.kind === 'E') { - reporter.handleError(output.notification.error); - } -} - -function reportInvalidActions( +export function reportInvalidActions( output: EffectNotification, reporter: ErrorHandler ) { diff --git a/modules/effects/src/effect_sources.ts b/modules/effects/src/effect_sources.ts index 77a1b761c4..5d85587d18 100644 --- a/modules/effects/src/effect_sources.ts +++ b/modules/effects/src/effect_sources.ts @@ -10,7 +10,10 @@ import { mergeMap, } from 'rxjs/operators'; -import { verifyOutput } from './effect_notification'; +import { + reportInvalidActions, + EffectNotification, +} from './effect_notification'; import { mergeEffects } from './effects_resolver'; import { onIdentifyEffectsKey, @@ -47,10 +50,9 @@ export class EffectSources extends Subject { mergeMap(source$ => source$.pipe(groupBy(effectsInstance))), mergeMap(source$ => source$.pipe( - exhaustMap(resolveEffectSource), + exhaustMap(resolveEffectSource(this.errorHandler)), map(output => { - verifyOutput(output, this.errorHandler); - + reportInvalidActions(output, this.errorHandler); return output.notification; }), filter( @@ -75,14 +77,18 @@ function effectsInstance(sourceInstance: any) { return ''; } -function resolveEffectSource(sourceInstance: any) { - const mergedEffects$ = mergeEffects(sourceInstance); +function resolveEffectSource( + errorHandler: ErrorHandler +): (sourceInstance: any) => Observable { + return sourceInstance => { + const mergedEffects$ = mergeEffects(sourceInstance, errorHandler); - if (isOnRunEffects(sourceInstance)) { - return sourceInstance.ngrxOnRunEffects(mergedEffects$); - } + if (isOnRunEffects(sourceInstance)) { + return sourceInstance.ngrxOnRunEffects(mergedEffects$); + } - return mergedEffects$; + return mergedEffects$; + }; } function isOnRunEffects(sourceInstance: { diff --git a/modules/effects/src/effects_metadata.ts b/modules/effects/src/effects_metadata.ts index 6a9a8d99d9..18aaebd6f5 100644 --- a/modules/effects/src/effects_metadata.ts +++ b/modules/effects/src/effects_metadata.ts @@ -5,8 +5,12 @@ import { getEffectDecoratorMetadata } from './effect_decorator'; export function getEffectsMetadata(instance: T): EffectsMetadata { const metadata: EffectsMetadata = {}; - for (const { propertyName, dispatch } of getSourceMetadata(instance)) { - metadata[propertyName] = { dispatch }; + for (const { + propertyName, + dispatch, + resubscribeOnError, + } of getSourceMetadata(instance)) { + metadata[propertyName] = { dispatch, resubscribeOnError }; } return metadata; diff --git a/modules/effects/src/effects_resolver.ts b/modules/effects/src/effects_resolver.ts index 0cadfb8f2c..f677c0c713 100644 --- a/modules/effects/src/effects_resolver.ts +++ b/modules/effects/src/effects_resolver.ts @@ -1,28 +1,44 @@ import { Action } from '@ngrx/store'; import { merge, Notification, Observable } from 'rxjs'; -import { ignoreElements, map, materialize } from 'rxjs/operators'; +import { ignoreElements, map, materialize, catchError } from 'rxjs/operators'; import { EffectNotification } from './effect_notification'; import { getSourceMetadata } from './effects_metadata'; import { getSourceForInstance } from './utils'; +import { ErrorHandler } from '@angular/core'; export function mergeEffects( - sourceInstance: any + sourceInstance: any, + errorHandler?: ErrorHandler ): Observable { const sourceName = getSourceForInstance(sourceInstance).constructor.name; - const observables: Observable[] = getSourceMetadata(sourceInstance).map( - ({ propertyName, dispatch }): Observable => { - const observable: Observable = + const observables$: Observable[] = getSourceMetadata(sourceInstance).map( + ({ + propertyName, + dispatch, + resubscribeOnError, + }): Observable => { + const observable$: Observable = typeof sourceInstance[propertyName] === 'function' ? sourceInstance[propertyName]() : sourceInstance[propertyName]; + const resubscribable$ = resubscribeOnError + ? observable$.pipe( + catchError(error => { + if (errorHandler) errorHandler.handleError(error); + // Return observable that produces this particular effect + return observable$; + }) + ) + : observable$; + if (dispatch === false) { - return observable.pipe(ignoreElements()); + return resubscribable$.pipe(ignoreElements()); } - const materialized$ = observable.pipe(materialize()); + const materialized$ = resubscribable$.pipe(materialize()); return materialized$.pipe( map( @@ -38,5 +54,5 @@ export function mergeEffects( } ); - return merge(...observables); + return merge(...observables$); } diff --git a/modules/effects/src/models.ts b/modules/effects/src/models.ts index fdbca30e91..d05ead1e8b 100644 --- a/modules/effects/src/models.ts +++ b/modules/effects/src/models.ts @@ -1,8 +1,12 @@ -export interface EffectMetadata { +export interface EffectConfig { + dispatch?: boolean; + resubscribeOnError?: boolean; +} + +export interface EffectMetadata extends Required { propertyName: Extract; - dispatch: boolean; } export type EffectsMetadata = { - [key in Extract]?: { dispatch: boolean } + [key in Extract]?: EffectConfig }; diff --git a/projects/ngrx.io/content/guide/effects/lifecycle.md b/projects/ngrx.io/content/guide/effects/lifecycle.md index a54fab3183..00f79c6284 100644 --- a/projects/ngrx.io/content/guide/effects/lifecycle.md +++ b/projects/ngrx.io/content/guide/effects/lifecycle.md @@ -14,6 +14,8 @@ init$ = createEffect(() => ); +## Effect Metadata + ### Non-dispatching Effects Sometimes you don't want effects to dispatch an action, for example when you only want to log or navigate based on an incoming action. But when an effect does not dispatch another action, the browser will crash because the effect is both 'subscribing' to and 'dispatching' the exact same action, causing an infinite loop. To prevent this, add `{ dispatch: false }` to the `createEffect` function as the second argument. @@ -36,6 +38,58 @@ export class LogEffects { } +### Resubscribe on Error + +Starting with version 8, when an error happens in the effect's main stream it is +reported using Angular's `ErrorHandler`, and the source effect is +**automatically** resubscribed to (instead of completing), so it continues to +listen to all dispatched Actions. + +Generally, errors should be handled by users, and operators such as `mapToAction` +should make it easier to do. However, for the cases where errors were missed, +this new behavior adds an additional safety net. + +In some cases where particular RxJS operators are used, the new behavior might +produce unexpected results. For example, if the `startWith` operator is within the +effect's pipe then it will be triggered again. + +To disable resubscriptions add `{resubscribeOnError: false}` to the `createEffect` +metadata (second argument). + + +import { Injectable } from '@angular/core'; +import { Actions, ofType, createEffect, mapToAction } from '@ngrx/effects'; +import { of } from 'rxjs'; +import { catchError, exhaustMap, map } from 'rxjs/operators'; +import { + LoginPageActions, + AuthApiActions, +} from '../actions'; +import { Credentials } from '../models/user'; +import { AuthService } from '../services/auth.service'; + +@Injectable() +export class AuthEffects { + login$ = createEffect(() => + this.actions$.pipe( + ofType(LoginPageActions.login), + mapToAction( + // Happy path callback + action => this.authService.login(action.credentials).pipe( + map(user => AuthApiActions.loginSuccess({ user }))), + // error callback + error => AuthApiActions.loginFailure({ error }), + ) + // Errors are handled and it is safe to disable resubscription + ), {resubscribeOnError: false }); + + constructor( + private actions$: Actions, + private authService: AuthService + ) {} +} + + ## Controlling Effects ### OnInitEffects diff --git a/projects/ngrx.io/content/guide/effects/testing.md b/projects/ngrx.io/content/guide/effects/testing.md index aa96719e00..e0ec80487d 100644 --- a/projects/ngrx.io/content/guide/effects/testing.md +++ b/projects/ngrx.io/content/guide/effects/testing.md @@ -119,11 +119,15 @@ describe('My Effects', () => { }); it('should register someSource$ that dispatches an action', () => { - expect(metadata.someSource$).toEqual({ dispatch: true }); + expect(metadata.someSource$).toEqual( + jasmine.objectContaining({ dispatch: true }) + ); }); it('should register someOtherSource$ that does not dispatch an action', () => { - expect(metadata.someOtherSource$).toEqual({ dispatch: false }); + expect(metadata.someOtherSource$).toEqual( + jasmine.objectContaining({ dispatch: false }) + ); }); it('should not register undecoratedSource$', () => { diff --git a/projects/ngrx.io/content/guide/migration/v8.md b/projects/ngrx.io/content/guide/migration/v8.md index bde77e0e7f..75dc5e0e7c 100644 --- a/projects/ngrx.io/content/guide/migration/v8.md +++ b/projects/ngrx.io/content/guide/migration/v8.md @@ -146,6 +146,14 @@ export const getNews: MemoizedSelector = createSelector( ); ``` +### @ngrx/effects + +#### Resubscribe on Errors + +If an error occurs (or is flattened) in the main effect's pipe then it will be +reported and the effect is resubscribed automatically. In cases when this new behavior is +undesirable, it can be disabled using `{resubscribeOnError: false}` within the effect metadata + (for each effect individually). [Learn more](/guide/effects/lifecycle#resubscribe-on-error). ### @ngrx/router-store