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(effect): catch action creators being returned in effect without () #2536

Merged
merged 3 commits into from
May 27, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion modules/effects/spec/effect_sources.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ describe('EffectSources', () => {
}

class SourceError {
e$ = createEffect(() => throwError(error));
e$ = createEffect(() => throwError(error) as any);
}

class SourceH {
Expand Down
2 changes: 1 addition & 1 deletion modules/effects/spec/effects_error_handler.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ describe('Effects Error Handler', () => {
}

class AlwaysErrorEffect {
effect$ = createEffect(() => throwError('always an error'));
effect$ = createEffect(() => throwError('always an error') as any);
}

/**
Expand Down
30 changes: 27 additions & 3 deletions modules/effects/spec/types/effect_creator.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ describe('createEffect()', () => {
code => `
import { Action } from '@ngrx/store';
import { createEffect } from '@ngrx/effects';
import { createAction } from '@ngrx/store';
import { of } from 'rxjs';

${code}`,
Expand All @@ -21,7 +22,22 @@ describe('createEffect()', () => {
expectSnippet(`
const effect = createEffect(() => of({ foo: 'a' }));
`).toFail(
/Type 'Observable<{ foo: string; }>' is not assignable to type 'Observable<Action> | ((...args: any[]) => Observable<Action>)'./
/Type 'Observable<{ foo: string; }>' is not assignable to type 'EffectResult<Action>'./
);
});

it('should help with action creator that is not called', () => {
// action creator is called with parentheses
expectSnippet(`
const action = createAction('action without props');
const effect = createEffect(() => of(action()));
`).toSucceed();

expectSnippet(`
const action = createAction('action without props');
const effect = createEffect(() => of(action));
`).toFail(
/ActionCreator cannot be dispatched. Did you forget to call action creator function/
alex-okrushko marked this conversation as resolved.
Show resolved Hide resolved
);
});

Expand All @@ -33,7 +49,7 @@ describe('createEffect()', () => {
expectSnippet(`
const effect = createEffect(() => of({ foo: 'a' }), { dispatch: true });
`).toFail(
/Type 'Observable<{ foo: string; }>' is not assignable to type 'Observable<Action> | ((...args: any[]) => Observable<Action>)'./
/Type 'Observable<{ foo: string; }>' is not assignable to type 'EffectResult<Action>'./
);
});
});
Expand All @@ -47,8 +63,16 @@ describe('createEffect()', () => {
expectSnippet(`
const effect = createEffect(() => ({ foo: 'a' }), { dispatch: false });
`).toFail(
/Type '{ foo: string; }' is not assignable to type 'Observable<unknown> | ((...args: any[]) => Observable<unknown>)'./
/Type '{ foo: string; }' is not assignable to type 'EffectResult<unknown>'./
);
});

it('should allow action creator even if it is not called', () => {
// action creator is called with parentheses
expectSnippet(`
const action = createAction('action without props');
const effect = createEffect(() => of(action), { dispatch: false });
`).toSucceed();
});
});
});
18 changes: 15 additions & 3 deletions modules/effects/src/effect_creator.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Observable } from 'rxjs';
import { Action } from '@ngrx/store';
import { Action, ActionCreator } from '@ngrx/store';
import {
EffectMetadata,
EffectConfig,
Expand All @@ -10,6 +10,15 @@ import {

type DispatchType<T> = T extends { dispatch: infer U } ? U : true;
type ObservableType<T, OriginalType> = T extends false ? OriginalType : Action;
type EffectResult<OT> = Observable<OT> | ((...args: any[]) => Observable<OT>);
type ConditionallyDisallowActionCreator<DT, Result> = DT extends false
? unknown // If DT (DispatchType is false, then we don't enforce any return types)
: Result extends EffectResult<infer OT>
? OT extends ActionCreator
? 'ActionCreator cannot be dispatched. Did you forget to call action creator function?'
alex-okrushko marked this conversation as resolved.
Show resolved Hide resolved
: unknown
: unknown;

/**
* @description
* Creates an effect from an `Observable` and an `EffectConfig`.
Expand Down Expand Up @@ -46,8 +55,11 @@ export function createEffect<
C extends EffectConfig,
DT extends DispatchType<C>,
OT extends ObservableType<DT, OT>,
R extends Observable<OT> | ((...args: any[]) => Observable<OT>)
>(source: () => R, config?: Partial<C>): R & CreateEffectMetadata {
R extends EffectResult<OT>
>(
source: () => R & ConditionallyDisallowActionCreator<DT, R>,
config?: Partial<C>
): R & CreateEffectMetadata {
const effect = source();
const value: EffectConfig = {
...DEFAULT_EFFECT_CONFIG,
Expand Down