Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(effects): make resubscription handler overridable #2295

Merged
merged 3 commits into from
Jan 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 13 additions & 11 deletions modules/effects/spec/effect_creator.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
]);
});

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

Expand Down
33 changes: 19 additions & 14 deletions modules/effects/spec/effect_sources.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,27 @@ import {
OnIdentifyEffects,
OnInitEffects,
createEffect,
EFFECTS_ERROR_HANDLER,
EffectsErrorHandler,
Actions,
} from '../';
import { defaultEffectsErrorHandler } from '../src/effects_error_handler';
import { EffectsRunner } from '../src/effects_runner';
import { Store } from '@ngrx/store';
import { ofType } from '../src';

describe('EffectSources', () => {
let mockErrorReporter: ErrorHandler;
let effectSources: EffectSources;
let effectsErrorHandler: EffectsErrorHandler;

beforeEach(() => {
TestBed.configureTestingModule({
providers: [
{
provide: EFFECTS_ERROR_HANDLER,
useValue: defaultEffectsErrorHandler,
},
EffectSources,
EffectsRunner,
{
Expand All @@ -47,6 +55,7 @@ describe('EffectSources', () => {

mockErrorReporter = TestBed.get(ErrorHandler);
effectSources = TestBed.get(EffectSources);
effectsErrorHandler = TestBed.get(EFFECTS_ERROR_HANDLER);

spyOn(mockErrorReporter, 'handleError');
});
Expand Down Expand Up @@ -144,6 +153,12 @@ describe('EffectSources', () => {
});

describe('toActions() Operator', () => {
function toActions(source: any): Observable<any> {
source['errorHandler'] = mockErrorReporter;
source['effectsErrorHandler'] = effectsErrorHandler;
return (effectSources as any)['toActions'].call(source);
}

describe('with @Effect()', () => {
const a = { type: 'From Source A' };
const b = { type: 'From Source B' };
Expand Down Expand Up @@ -346,9 +361,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');
Expand Down Expand Up @@ -387,11 +402,6 @@ describe('EffectSources', () => {

expect(output).toBeObservable(expected);
});

function toActions(source: any): Observable<any> {
source['errorHandler'] = mockErrorReporter;
return (effectSources as any)['toActions'].call(source);
}
});

describe('with createEffect()', () => {
Expand Down Expand Up @@ -635,7 +645,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(
Expand All @@ -646,7 +656,7 @@ describe('EffectSources', () => {
return v;
})
),
{ dispatch: false, resubscribeOnError: false }
{ dispatch: false, useEffectsErrorHandler: false }
);
}()
);
Expand Down Expand Up @@ -678,11 +688,6 @@ describe('EffectSources', () => {

expect(output).toBeObservable(expected);
});

function toActions(source: any): Observable<any> {
source['errorHandler'] = mockErrorReporter;
return (effectSources as any)['toActions'].call(source);
}
});
});

Expand Down
100 changes: 100 additions & 0 deletions modules/effects/spec/effects_error_handler.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
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('Effects Error Handler', () => {
let subscriptionCount: number;
let globalErrorHandler: jasmine.Spy;
let storeNext: jasmine.Spy;

function makeEffectTestBed(...providers: Provider[]) {
timdeschryver marked this conversation as resolved.
Show resolved Hide resolved
subscriptionCount = 0;

TestBed.configureTestingModule({
imports: [EffectsModule.forRoot([ErrorEffect])],
providers: [
{
provide: Store,
useValue: {
next: jasmine.createSpy('storeNext'),
dispatch: jasmine.createSpy('dispatch'),
},
},
alex-okrushko marked this conversation as resolved.
Show resolved Hide resolved
{
provide: ErrorHandler,
useValue: {
handleError: jasmine.createSpy('globalErrorHandler'),
},
},
...providers,
],
});

globalErrorHandler = TestBed.get(ErrorHandler).handleError;
const store = TestBed.get(Store);
storeNext = store.next;
}
alex-okrushko marked this conversation as resolved.
Show resolved Hide resolved

it('should retry and notify error handler when effect error handler is not provided', () => {
makeEffectTestBed();

// two subscriptions expected:
// 1. Initial subscription to the effect (this will error)
// 2. Resubscription to the effect after error (this will not error)
expect(subscriptionCount).toBe(2);
alex-okrushko marked this conversation as resolved.
Show resolved Hide resolved
expect(globalErrorHandler).toHaveBeenCalledWith(new Error('effectError'));
});

it('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' });
alex-okrushko marked this conversation as resolved.
Show resolved Hide resolved
});

class ErrorEffect {
effect$ = createEffect(errorFirstSubscriber, {
useEffectsErrorHandler: true,
});
}

/**
* This observable factory returns an observable that will never emit, but the first subscriber will get an immediate
* error. All subsequent subscribers will just get an observable that does not emit.
*/
function errorFirstSubscriber(): Observable<Action> {
return new Observable(observer => {
subscriptionCount++;

if (subscriptionCount === 1) {
alex-okrushko marked this conversation as resolved.
Show resolved Hide resolved
observer.error(new Error('effectError'));
}
});
}
});
28 changes: 14 additions & 14 deletions modules/effects/spec/effects_metadata.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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 },
});
});

Expand Down
2 changes: 1 addition & 1 deletion modules/effects/src/effect_creator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading