Skip to content

Commit

Permalink
feat(effects): resubscribe to effects on error (#1881)
Browse files Browse the repository at this point in the history 
BREAKING CHANGE:

Prior to introduction of automatic resubscriptions on errors, all effects had effectively {resubscribeOnError: false} behavior. For the rare cases when this is still wanted please add {resubscribeOnError: false} to the effect metadata.

BEFORE:

```ts
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 }),
      ) 
    ));
```

AFTER:

```ts
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 });
```
  • Loading branch information
@alex-okrushko @brandonroberts
alex-okrushko authored and brandonroberts committed May 23, 2019
1 parent 478b225 commit 71137e5
Show file tree
Hide file tree
Showing 14 changed files with 345 additions and 83 deletions.
40 changes: 32 additions & 8 deletions modules/effects/spec/effect_creator.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>)'./
);
});
});
Expand All @@ -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', () => {
Expand All @@ -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 },
]);
});

Expand Down
21 changes: 17 additions & 4 deletions modules/effects/spec/effect_decorator.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
]);
});

Expand Down
123 changes: 121 additions & 2 deletions modules/effects/spec/effect_sources.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -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());

Expand Down
36 changes: 24 additions & 12 deletions modules/effects/spec/effects_metadata.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -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', () => {
Expand All @@ -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);
});
});

Expand All @@ -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 },
});
});

Expand Down
30 changes: 16 additions & 14 deletions modules/effects/src/effect_creator.ts
View file
Original file line number Diff line number Diff line change
@@ -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;
}
Expand Down
Loading

0 comments on commit 71137e5

Please sign in to comment.