From 28ff098af058c5257402f6aa671c905c6cd47ea2 Mon Sep 17 00:00:00 2001 From: aokrushko Date: Thu, 21 May 2020 23:25:49 -0400 Subject: [PATCH 1/3] feat(effect): catch action creators being returned in effect without () --- modules/effects/spec/effect_sources.spec.ts | 2 +- .../spec/effects_error_handler.spec.ts | 2 +- .../effects/spec/types/effect_creator.spec.ts | 30 +++++++++++++++++-- modules/effects/src/effect_creator.ts | 18 +++++++++-- 4 files changed, 44 insertions(+), 8 deletions(-) diff --git a/modules/effects/spec/effect_sources.spec.ts b/modules/effects/spec/effect_sources.spec.ts index c107524ccd..4a6f5d9a13 100644 --- a/modules/effects/spec/effect_sources.spec.ts +++ b/modules/effects/spec/effect_sources.spec.ts @@ -375,7 +375,7 @@ describe('EffectSources', () => { } class SourceError { - e$ = createEffect(() => throwError(error)); + e$ = createEffect(() => throwError(error) as any); } class SourceH { diff --git a/modules/effects/spec/effects_error_handler.spec.ts b/modules/effects/spec/effects_error_handler.spec.ts index 0a26d5e8fe..d724833c3a 100644 --- a/modules/effects/spec/effects_error_handler.spec.ts +++ b/modules/effects/spec/effects_error_handler.spec.ts @@ -93,7 +93,7 @@ describe('Effects Error Handler', () => { } class AlwaysErrorEffect { - effect$ = createEffect(() => throwError('always an error')); + effect$ = createEffect(() => throwError('always an error') as any); } /** diff --git a/modules/effects/spec/types/effect_creator.spec.ts b/modules/effects/spec/types/effect_creator.spec.ts index 6388c23219..f5e3e38d6b 100644 --- a/modules/effects/spec/types/effect_creator.spec.ts +++ b/modules/effects/spec/types/effect_creator.spec.ts @@ -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}`, @@ -21,7 +22,22 @@ describe('createEffect()', () => { expectSnippet(` const effect = createEffect(() => of({ foo: 'a' })); `).toFail( - /Type 'Observable<{ foo: string; }>' is not assignable to type 'Observable | ((...args: any[]) => Observable)'./ + /Type 'Observable<{ foo: string; }>' is not assignable to type 'EffectResult'./ + ); + }); + + 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/ ); }); @@ -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 | ((...args: any[]) => Observable)'./ + /Type 'Observable<{ foo: string; }>' is not assignable to type 'EffectResult'./ ); }); }); @@ -47,8 +63,16 @@ 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 'EffectResult'./ ); }); + + 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(); + }); }); }); diff --git a/modules/effects/src/effect_creator.ts b/modules/effects/src/effect_creator.ts index 7d972d41cb..eb2aa4b16e 100644 --- a/modules/effects/src/effect_creator.ts +++ b/modules/effects/src/effect_creator.ts @@ -1,5 +1,5 @@ import { Observable } from 'rxjs'; -import { Action } from '@ngrx/store'; +import { Action, ActionCreator } from '@ngrx/store'; import { EffectMetadata, EffectConfig, @@ -10,6 +10,15 @@ import { type DispatchType = T extends { dispatch: infer U } ? U : true; type ObservableType = T extends false ? OriginalType : Action; +type EffectResult = Observable | ((...args: any[]) => Observable); +type ConditionallyDisallowActionCreator = DT extends false + ? unknown // If DT (DispatchType is false, then we don't enforce any return types) + : Result extends EffectResult + ? OT extends ActionCreator + ? 'ActionCreator cannot be dispatched. Did you forget to call action creator function?' + : unknown + : unknown; + /** * @description * Creates an effect from an `Observable` and an `EffectConfig`. @@ -46,8 +55,11 @@ export function createEffect< C extends EffectConfig, DT extends DispatchType, OT extends ObservableType, - R extends Observable | ((...args: any[]) => Observable) ->(source: () => R, config?: Partial): R & CreateEffectMetadata { + R extends EffectResult +>( + source: () => R & ConditionallyDisallowActionCreator, + config?: Partial +): R & CreateEffectMetadata { const effect = source(); const value: EffectConfig = { ...DEFAULT_EFFECT_CONFIG, From 014bff901538b1d67e8a27837a5d9cfc43fc1544 Mon Sep 17 00:00:00 2001 From: aokrushko Date: Fri, 22 May 2020 11:47:12 -0400 Subject: [PATCH 2/3] Add "the" to the error message --- modules/effects/spec/types/effect_creator.spec.ts | 7 ++++--- modules/effects/src/effect_creator.ts | 2 +- modules/store/src/store.ts | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/modules/effects/spec/types/effect_creator.spec.ts b/modules/effects/spec/types/effect_creator.spec.ts index f5e3e38d6b..7dc9a44090 100644 --- a/modules/effects/spec/types/effect_creator.spec.ts +++ b/modules/effects/spec/types/effect_creator.spec.ts @@ -27,17 +27,18 @@ describe('createEffect()', () => { }); it('should help with action creator that is not called', () => { - // action creator is called with parentheses + // Action creator is called with parentheses. expectSnippet(` const action = createAction('action without props'); const effect = createEffect(() => of(action())); `).toSucceed(); + // Action creator is not called (no parentheses). 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/ + /ActionCreator cannot be dispatched. Did you forget to call the action creator function/ ); }); @@ -68,7 +69,7 @@ describe('createEffect()', () => { }); it('should allow action creator even if it is not called', () => { - // action creator is called with parentheses + // Action creator is not called (no parentheses), but we have no-dispatch. expectSnippet(` const action = createAction('action without props'); const effect = createEffect(() => of(action), { dispatch: false }); diff --git a/modules/effects/src/effect_creator.ts b/modules/effects/src/effect_creator.ts index eb2aa4b16e..3f36fe0a77 100644 --- a/modules/effects/src/effect_creator.ts +++ b/modules/effects/src/effect_creator.ts @@ -15,7 +15,7 @@ type ConditionallyDisallowActionCreator = DT extends false ? unknown // If DT (DispatchType is false, then we don't enforce any return types) : Result extends EffectResult ? OT extends ActionCreator - ? 'ActionCreator cannot be dispatched. Did you forget to call action creator function?' + ? 'ActionCreator cannot be dispatched. Did you forget to call the action creator function?' : unknown : unknown; diff --git a/modules/store/src/store.ts b/modules/store/src/store.ts index 6677d84d0d..b9cc566d1e 100644 --- a/modules/store/src/store.ts +++ b/modules/store/src/store.ts @@ -98,7 +98,7 @@ export class Store extends Observable action: V & FunctionIsNotAllowed< V, - 'Functions are not allowed to be dispatched. Did you forget to call action creator function?' + 'Functions are not allowed to be dispatched. Did you forget to call the action creator function?' > ) { this.actionsObserver.next(action); From 89b509f3237f48fd7774368a5ec7c520f6d0056b Mon Sep 17 00:00:00 2001 From: aokrushko Date: Fri, 22 May 2020 11:50:13 -0400 Subject: [PATCH 3/3] Adjust store spec as well --- modules/store/spec/types/store.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/store/spec/types/store.spec.ts b/modules/store/spec/types/store.spec.ts index e8b0f6b2b0..37e73ae958 100644 --- a/modules/store/spec/types/store.spec.ts +++ b/modules/store/spec/types/store.spec.ts @@ -16,7 +16,7 @@ describe('Store', () => { it('should not allow passing action creator function without calling it', () => { expectSnippet(`store.dispatch(fooAction);`).toFail( - /is not assignable to type '"Functions are not allowed to be dispatched. Did you forget to call action creator function/ + /is not assignable to type '"Functions are not allowed to be dispatched. Did you forget to call the action creator function/ ); }); });