fix(action): Stop passing a function to store.dipatch and stop effect… #1914

SerkanSipahi · 2019-06-04T20:22:34Z

… that is returns a function

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Closes #1906

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

… that is returns a function ngrx#1906

SerkanSipahi · 2019-06-04T20:26:51Z

modules/store/src/actions_subject.ts

@@ -13,12 +25,7 @@ export class ActionsSubject extends BehaviorSubject<Action>
  }

  next(action: Action): void {
-    if (typeof action === 'undefined') {


There were no tests for this block (for the errors in the expression sections). Should I write some for it?

ngrxbot · 2019-06-04T20:38:22Z

Preview docs changes for 216a4a5 at https://previews.ngrx.io/pr1914-216a4a5/

timdeschryver · 2019-06-04T21:00:38Z

modules/effects/src/effects_runner.ts

@@ -17,6 +19,7 @@ export class EffectsRunner implements OnDestroy {
    if (!this.effectsSubscription) {
      this.effectsSubscription = this.effectSources
        .toActions()
+        .pipe(tap(action => throwErrorOnInvalidAction(action)))


We don't need to to verify the action here. The action returned will be dispatched to the store, and will be verified there.

You are absolutely right. The circle closes at the place!
EffectsRunner.start() => Store.next() => ActionsSubject.next() :)

Its removed!

timdeschryver · 2019-06-04T21:01:06Z

modules/store/src/actions_subject.ts

+    throw new TypeError(`Actions must be objects`);
+  } else if (typeof action === 'function') {
+    throw new TypeError(`
+      Actions as function are not now allowed.


Suggested change

Actions as function are not now allowed.

Actions as function are not allowed.

timdeschryver · 2019-06-04T21:05:51Z

modules/store/src/actions_subject.ts

+  } else if (typeof action === 'function') {
+    throw new TypeError(`
+      Actions as function are not now allowed.
+      Make sure your action is called e.g someAction()`);


Should we make this more explicit?

Dispatch expected an object, instead it received xxx If you're using the createAction function, make sure to invoke the function before dispatching the action. E.g. someAction should be someAction().

What about this?

const typeOf = (value: any) => ({}).toString.call(value).slice(8, -1).toLowerCase(); ...code... ...code... ...code... next(action: Action): void { if(typeOf(action) !== 'object') { throw new TypeError(` Dispatch expected an object, instead it received ${action}. If you're using the createAction function, make sure to invoke the function before dispatching the action. E.g. someAction should be someAction().`); } else if (typeof action.type === 'undefined') { throw new TypeError(`Actions must have a type property`); } super.next(action); }

Should we test this cases?

Its more explicit now (commited)

ngrx#1914

ngrxbot · 2019-06-05T18:10:33Z

Preview docs changes for 55ed464 at https://previews.ngrx.io/pr1914-55ed464/

brandonroberts · 2019-06-05T18:21:47Z

There should be an added check here also https://github.com/ngrx/platform/blob/master/modules/effects/src/effect_notification.ts#L33

SerkanSipahi · 2019-06-05T18:32:20Z

Is there any place in ngrx where we can put helper methods? e.g. this:

const throwErrorOnInvalidAction = action => {
  if(typeOf(action) !== 'object') {
    throw new TypeError(`
      Dispatch expected an object, instead it received ${action}.
      If you're using the createAction function, make sure to invoke the function
      before dispatching the action. E.g. someAction should be someAction().`);    
  } else if (typeof action.type === 'undefined') {
    throw new TypeError(`Actions must have a type property`);
  }
}

This can be used can also in https://github.com/ngrx/platform/blob/master/modules/effects/src/effect_notification.ts#L33

brandonroberts · 2019-06-05T18:38:46Z

No. In this case they will just have to be separate. I don't think the function needs to exist in the public API for either library.

SerkanSipahi · 2019-06-05T18:58:32Z

@brandonroberts I hope I understood you correctly! Like this?

const typeOf = action =>  ({}).toString.call(action).slice(8, -1).toLowerCase();

function isAction(action: any): action is Action {
  return typeOf(action) === 'object' && action && action.type && typeof action.type === 'string';
}

ngrxbot · 2019-06-05T21:21:24Z

Preview docs changes for 2b25c95 at https://previews.ngrx.io/pr1914-2b25c95/

SerkanSipahi · 2019-06-05T21:38:27Z

@brandonroberts its separated now (commited)

alex-okrushko · 2019-06-06T03:53:04Z

modules/store/src/actions_subject.ts

@@ -13,12 +19,14 @@ export class ActionsSubject extends BehaviorSubject<Action>
  }

  next(action: Action): void {
-    if (typeof action === 'undefined') {
-      throw new TypeError(`Actions must be objects`);
+    if (typeOf(action) !== 'object') {


would typeof action !== 'object' not work?

There is a edge case when you pass accidently an array instead of object.

typeof [] // object

// this code snipped can recognize each kind of type

({}).toString.call([]).slice(8, -1).toLowerCase() // array

I just wanted a strict treatment regardless of whether we use typescript or not.

It's true that array is type of object as well, however:

we are trying to catch the case where function is passed instead of object, for example action creator is passed without calling it

even if array doesn't run into this check, it will be caught with the next if-statement typeof action.type === 'undefined'

Also, I find the code snippet ({}).toString.call([]).slice(8, -1).toLowerCase() to be somewhat "crafty" 😁and not very straightforward/readably. I'd rather avoid it.

I agree with @alex-okrushko.
To keep it readable I'm in favor of using typeof.

Plus we already have a compile error if we try to dispatch an array. The createAction is only the problem here.

I had the same thoughts (compile error) after reading what @alex-okrushko wrote. I was to strict! I have to find a balance between typescript and javascript :)

({}).toString.call([]).slice(8, -1).toLowerCase() its removed. There is now a simple the for function.

alex-okrushko · 2019-06-06T03:54:05Z

modules/effects/src/effect_notification.ts

@@ -30,8 +30,19 @@ export function reportInvalidActions(
  }
 }

+const typeOf = (value: any) =>


Generally these should be structured as function declarations, e.g.

function typeOf(value: any) {...}

The typeOf is removed. We use typeof xxx as you suggested.

…atch-action-and-effects-return-action

ngrxbot · 2019-06-07T05:50:50Z

Preview docs changes for c72dbde at https://previews.ngrx.io/pr1914-c72dbde/

SerkanSipahi · 2019-06-07T06:02:46Z

@timdeschryver @alex-okrushko all code parts were adjusted according your code review suggestions.

ngrxbot · 2019-06-07T06:03:49Z

Preview docs changes for ca62d3a at https://previews.ngrx.io/pr1914-ca62d3a/

ngrxbot · 2019-06-07T06:26:22Z

Preview docs changes for dd9131e at https://previews.ngrx.io/pr1914-dd9131e/

alex-okrushko · 2019-06-07T12:57:44Z

Looks good. Thanks Serkan!

brandonroberts · 2019-06-07T13:22:12Z

modules/store/src/actions_subject.ts

+      throw new TypeError(`
+        Dispatch expected an object, instead it received function.
+        If you're using the createAction function, make sure to invoke the function
+        before dispatching the action. E.g. someAction should be someAction().`);


Nit: Everyone doesn't know what e.g. means.

Suggested change

before dispatching the action. E.g. someAction should be someAction().`);

before dispatching the action. For example, someAction should be someAction().`);

timdeschryver · 2019-06-09T08:43:03Z

modules/store/src/actions_subject.ts

-    if (typeof action === 'undefined') {
+    if (typeof action === 'function') {
+      throw new TypeError(`
+        Dispatch expected an object, instead it received function.


Suggested change

Dispatch expected an object, instead it received function.

Dispatch expected an object, instead it received a function.

…atch-action-and-effects-return-action

ngrxbot · 2019-06-09T10:29:46Z

Preview docs changes for 11b3e8a at https://previews.ngrx.io/pr1914-11b3e8a/

ngrxbot · 2019-06-09T10:39:44Z

Preview docs changes for 9619b37 at https://previews.ngrx.io/pr1914-9619b37/

alex-okrushko · 2019-06-10T15:34:08Z

modules/effects/src/effect_notification.ts

+    action &&
+    action.type &&
+    typeof action.type === 'string'
+  );


I think you don't need to wrap the return in brackets. This could be reverted to what it was before.

Oh, sorry, I didn't realize that! I had already wondered why prettier always jumped on when I committed something on NgRx. That doesn't happen with my private repos. I thought it was something like a hooks coming from ngrx repo.

fix(action): Stop passing a function to store.dipatch and stop effect…

216a4a5

… that is returns a function ngrx#1906

SerkanSipahi commented Jun 4, 2019

View reviewed changes

timdeschryver requested changes Jun 4, 2019

View reviewed changes

timdeschryver added the 8.x label Jun 4, 2019

fix(action): revert using of throwErrorOnInvalidAction in EffectsRunner

55ed464

ngrx#1914

fix(action): code changes acording code review ngrx#1914

2b25c95

alex-okrushko requested changes Jun 6, 2019

View reviewed changes

Merge https://github.com/ngrx/platform into bug/strange-behavior-disp…

c72dbde

…atch-action-and-effects-return-action

fix(action): it should be checked if the action is not a function

ca62d3a

fix(action): add missing expression when action is undefined

dd9131e

alex-okrushko approved these changes Jun 7, 2019

View reviewed changes

brandonroberts requested changes Jun 7, 2019

View reviewed changes

timdeschryver reviewed Jun 9, 2019

View reviewed changes

SerkanSipahi added 2 commits June 9, 2019 12:21

Merge https://github.com/ngrx/platform into bug/strange-behavior-disp…

11b3e8a

…atch-action-and-effects-return-action

fix(action): adjust wording of action error

9619b37

brandonroberts requested a review from timdeschryver June 10, 2019 11:28

brandonroberts approved these changes Jun 10, 2019

View reviewed changes

timdeschryver approved these changes Jun 10, 2019

View reviewed changes

brandonroberts merged commit 78153cb into ngrx:master Jun 10, 2019

alex-okrushko reviewed Jun 10, 2019

View reviewed changes

	Actions as function are not now allowed.
	Actions as function are not allowed.

	before dispatching the action. E.g. someAction should be someAction().`);
	before dispatching the action. For example, someAction should be someAction().`);

	Dispatch expected an object, instead it received function.
	Dispatch expected an object, instead it received a function.

fix(action): Stop passing a function to store.dipatch and stop effect… #1914

fix(action): Stop passing a function to store.dipatch and stop effect… #1914

Conversation

SerkanSipahi commented Jun 4, 2019

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

SerkanSipahi Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

ngrxbot commented Jun 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SerkanSipahi Jun 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngrxbot commented Jun 5, 2019

brandonroberts commented Jun 5, 2019

SerkanSipahi commented Jun 5, 2019

brandonroberts commented Jun 5, 2019

SerkanSipahi commented Jun 5, 2019 • edited Loading

ngrxbot commented Jun 5, 2019

SerkanSipahi commented Jun 5, 2019

Choose a reason for hiding this comment

SerkanSipahi Jun 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SerkanSipahi Jun 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngrxbot commented Jun 7, 2019

SerkanSipahi commented Jun 7, 2019 • edited Loading

ngrxbot commented Jun 7, 2019

ngrxbot commented Jun 7, 2019

alex-okrushko commented Jun 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngrxbot commented Jun 9, 2019

ngrxbot commented Jun 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SerkanSipahi Jun 4, 2019 •

edited

Loading

SerkanSipahi Jun 5, 2019 •

edited

Loading

SerkanSipahi commented Jun 5, 2019 •

edited

Loading

SerkanSipahi Jun 6, 2019 •

edited

Loading

SerkanSipahi Jun 6, 2019 •

edited

Loading

SerkanSipahi commented Jun 7, 2019 •

edited

Loading