feat(effects): catch action creators being returned in effect without being called (#2536)

Author: alex-okrushko

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 {

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);
   }
 
   /**

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,23 @@ 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();
+
+      // 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 the action creator function/
       );
     });
 
@@ -33,7 +50,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>'./
       );
     });
   });
@@ -47,8 +64,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 not called (no parentheses), but we have no-dispatch.
+      expectSnippet(`
+      const action = createAction('action without props');
+      const effect = createEffect(() => of(action), { dispatch: false });
+      `).toSucceed();
+    });
   });
 });

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> = 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 the 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<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,

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/
     );
   });
 });

modules/store/src/store.ts

@@ -98,7 +98,7 @@ export class Store<T = object> extends Observable<T>
     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);