feat(Store): add action creators #1570

tja4472 · 2019-02-20T10:38:01Z

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?

[ ] Bugfix
[x] 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?

#1480

What is the new behavior?

Adds action creators

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

coveralls · 2019-02-20T10:40:26Z

Coverage increased (+0.02%) to 89.529% when pulling a3c4ee4 on tja4472:issue-1480 into 5cb9a46 on ngrx:master.

ngrxbot · 2019-02-20T10:44:16Z

Preview docs changes for b8d8099 at https://previews.ngrx.io/pr1570-b8d8099/

ngrxbot · 2019-02-21T10:31:40Z

Preview docs changes for 38c78c7 at https://previews.ngrx.io/pr1570-38c78c7/

ngrxbot · 2019-02-21T10:59:31Z

Preview docs changes for 447480a at https://previews.ngrx.io/pr1570-447480a/

ngrxbot · 2019-02-21T11:22:02Z

Preview docs changes for 47a8041 at https://previews.ngrx.io/pr1570-47a8041/

ngrxbot · 2019-02-22T10:53:06Z

Preview docs changes for 7976c89 at https://previews.ngrx.io/pr1570-7976c89/

brandonroberts · 2019-02-22T13:17:12Z

Some initial feedback:

We're not re-introducing payload as an option. Using payload is a convention and not required. I'd start out with creating an action with a type, and support for action with props.
Consolidate creating an action and an action with props to one function.
For the example app, the imports should not be AuthApiActions.AuthApiActions as this is intended to make that syntax shorter with the type merging
Since the action creators are functions, we can pass the creator into the map function. For example map(AuthApiActions.loginSuccess) vs map(user => AuthApiActions.loginSuccess(user))

Thanks for working on this!

brandonroberts

Initial changes

ngrxbot · 2019-02-23T11:31:45Z

Preview docs changes for 10412bc at https://previews.ngrx.io/pr1570-10412bc/

modules/store/src/action_creator.ts

ngrxbot · 2019-02-24T18:18:13Z

Preview docs changes for ca6d9db at https://previews.ngrx.io/pr1570-ca6d9db/

ngrxbot · 2019-02-25T11:02:18Z

Preview docs changes for ee081de at https://previews.ngrx.io/pr1570-ee081de/

ngrxbot · 2019-02-26T10:47:18Z

Preview docs changes for 67a692f at https://previews.ngrx.io/pr1570-67a692f/

ngrxbot · 2019-02-28T11:49:06Z

Preview docs changes for 2991f70 at https://previews.ngrx.io/pr1570-2991f70/

timdeschryver

Looking good!
I left some small comments.

Could you squash the commits in to two parts?

Add action creator
Refactor the example app

modules/store/spec/action_creator.spec.ts

projects/example-app/src/app/auth/effects/auth.effects.ts

projects/example-app/src/app/auth/actions/auth-api.actions.ts

projects/example-app/src/app/auth/actions/login-page.actions.ts

modules/store/src/action_creator.ts

ngrxbot · 2019-03-02T09:53:30Z

Preview docs changes for 8430c71 at https://previews.ngrx.io/pr1570-8430c71/

ngrxbot · 2019-03-02T10:39:27Z

Preview docs changes for 0f322b3 at https://previews.ngrx.io/pr1570-0f322b3/

ngrxbot · 2019-03-02T10:59:41Z

Preview docs changes for ba8c655 at https://previews.ngrx.io/pr1570-ba8c655/

ngrxbot · 2019-03-02T14:52:35Z

Preview docs changes for c8fc6b9 at https://previews.ngrx.io/pr1570-c8fc6b9/

ngrxbot · 2019-03-02T14:56:35Z

Preview docs changes for c6c86c1 at https://previews.ngrx.io/pr1570-c6c86c1/

ngrxbot · 2019-03-02T15:19:25Z

Preview docs changes for 5685c8c at https://previews.ngrx.io/pr1570-5685c8c/

ngrxbot · 2019-03-02T15:29:56Z

Preview docs changes for 52999ad at https://previews.ngrx.io/pr1570-52999ad/

osnoser1 · 2019-03-03T01:29:19Z

Hi, one question. This feature may appear before version 8, since it does not produce a breaking change? Thank you very much in advance.

brandonroberts · 2019-03-04T03:19:47Z

@osnoser1 this will most likely land with V8 as we'll want to upgrade TypeScript to the latest version first, along with other changes to support this feature.

modules/store/src/action_creator.ts

ngrxbot · 2019-03-09T11:50:36Z

Preview docs changes for 7d0a5ac at https://previews.ngrx.io/pr1570-7d0a5ac/

ngrxbot · 2019-03-09T14:50:41Z

Preview docs changes for a3c4ee4 at https://previews.ngrx.io/pr1570-a3c4ee4/

Requested changes are made

ngrxbot · 2019-03-10T10:37:20Z

Preview docs changes for 52a8470 at https://previews.ngrx.io/pr1570-52a8470/

tja4472 · 2019-03-10T10:48:28Z

Done! 😄

alex-okrushko · 2019-03-11T15:11:21Z

projects/example-app/src/app/auth/actions/auth-api.actions.ts

@@ -7,20 +7,12 @@ export enum AuthApiActionTypes {
  LoginRedirect = '[Auth/API] Login Redirect',
 }

-export class LoginSuccess implements Action {
-  readonly type = AuthApiActionTypes.LoginSuccess;
+export const AuthApiActions = {


I think having functions directly in the file would be better. You can then use named module import as a grouping mechanism.

We have to group them this way to take advantage of the ActionsUnion?

So a few issues I have with this:

both function and classes Action creator are allowed and welcomed. This changes to function-specific.

according to Google's TS style guide:

When providing a structural-based implementation, explicitly include the type at the declaration of the symbol (this allows more precise type checking and error reporting).

// use const foo: Foo = { a: 123, b: 'abc', } // instead of const foo = { a: 123, b: 'abc', }

On the other hand, I don't think anyone at Google is using function-based action creator and this proposal could be fine.

I'm planning to propose something like this for the class-based action creators: https://www.typescriptlang.org/play/index.html#src=%2F%2F%20action_helper.ts%0D%0Ainterface%20Type%3CC%2C%20T%20extends%20string%3E%20%7B%0D%0A%20%20%20new%20(...args%3A%20any%5B%5D)%3A%20C%3B%0D%0A%20%20%20readonly%20type%3A%20T%3B%0D%0A%7D%0D%0Aabstract%20class%20StaticAction%3CT%20extends%20string%3E%20%7B%0D%0A%20%20static%20readonly%20type%3A%20string%3B%0D%0A%20%20readonly%20type!%3A%20T%3B%0D%0A%7D%0D%0A%0D%0Afunction%20Action%3CT%20extends%20string%3E(type%3A%20T)%3A%20Type%3CStaticAction%3CT%3E%2C%20T%3E%20%7B%0D%0A%20%20return%20class%20extends%20StaticAction%3CT%3E%20%7B%0D%0A%20%20%20%20readonly%20type%20%3D%20type%3B%0D%0A%20%20%20%20static%20type%20%3D%20type%3B%0D%0A%20%20%7D%0D%0A%7D%0D%0A%0D%0Aclass%20MyAction%20extends%20Action('foo')%20%7B%7D%0D%0A%0D%0Aconsole.log(new%20MyAction().type)%3B%20%2F%2F%20logs%3A%20'foo'%0D%0Aconsole.log(MyAction.type)%3B%20%2F%2F%20logs%3A%20'foo

I should add that this was @kolodny idea, that I iterated on.

Will you open a new issue with this proposal? I'd lean towards creating a new API such as withAction('type') for this as opposed to changing Action.

alex-okrushko · 2019-03-19T10:20:48Z

@brandonroberts may I ask you to take a second look at ts-actions?
I was going through it lately, and the API there and the static type property are really appealing to me (instead of string/enum juggling).
It looks like this PR is trying to be that (but not being quite there)

Blog about ts-actions: https://blog.angularindepth.com/how-to-reduce-action-boilerplate-90dc3d389e2b

Lib itself: https://github.com/cartant/ts-action

I think it changes my opinion about Class-based actions. If anything, I'll probably switch to that style.

brandonroberts · 2019-03-19T13:32:12Z

@alex-okrushko Sure. Maybe we can land this one and follow-up with a PR that add something similar to what ts-action provides.

brandonroberts · 2019-03-19T14:46:18Z

@tja4472 will you rebase on master to fix the merge conflicts?

alex-okrushko · 2019-03-19T14:48:29Z

@brandonroberts @tja4472 @timdeschryver
I think they will be conflicting with each other.

I'd really like to remove the Enum/String type usage. e.g. AuthActionTypes.Logout vs AuthActions.logout- this is really one of the main pain points, which makes it hard to track the actions throughout the codebase.

Compare that to the ts-actions:

const foo = action("FOO", payload<{ foo: number }>());
foo({foo: 5}); // create action
foo.type // statically get the type to be used in reducer or ofType() in Effect.

What if we clone (with some adjustments that make sense for NgRx) the ts-actions lib into say @ngrx/actions package and make it the only and preferred choice? Or even make it as part of core @ngrx/store?

I'll be more than willing to drop my proposal from #1634 in favor of that.

timdeschryver · 2019-03-19T20:00:42Z

I'm in favor of having one way in comparison to having a function approach (this PR) as well as having a class approach (proposal in #1634).
The only drawback I see of using ts-action would be what it looks different from what we are used to. But on the other hand, is it a valid argument...?

brandonroberts · 2019-03-19T20:35:10Z

@alex-okrushko I'm open to including some form of the action creator to @ngrx/store. The current class based approach should still be a viable option though, with the ofType operator able to support both if possible.

ngrxbot · 2019-03-20T11:30:26Z

Preview docs changes for 6bba23d at https://previews.ngrx.io/pr1570-6bba23d/

ngrxbot · 2019-03-20T18:20:24Z

Preview docs changes for bb2c7b6 at https://previews.ngrx.io/pr1570-bb2c7b6/

ngrxbot · 2019-03-20T19:03:29Z

Preview docs changes for ee0d102 at https://previews.ngrx.io/pr1570-ee0d102/

tja4472 · 2019-03-20T20:36:20Z

@brandonroberts Rebased 😄

brandonroberts · 2019-03-25T18:57:29Z

@tja4472 thanks for your time and effort on this PR, but I believe the implementation proposed by @alex-okrushko and @kolodny on the idea/library by @cartant in #1634 is the one we'll go with.

tja4472 · 2019-03-25T20:36:24Z

@brandonroberts I quite understand. 👍

Thank you for your time.

Looking forward to the completed #1634

brandonroberts requested changes Feb 22, 2019

View reviewed changes

timdeschryver reviewed Feb 23, 2019

View reviewed changes

modules/store/src/action_creator.ts Outdated Show resolved Hide resolved

modules/store/src/action_creator.ts Outdated Show resolved Hide resolved

tja4472 marked this pull request as ready for review March 1, 2019 10:59

timdeschryver previously requested changes Mar 1, 2019

View reviewed changes

tja4472 force-pushed the issue-1480 branch 3 times, most recently from c8fc6b9 to c6c86c1 Compare March 2, 2019 14:50

tja4472 force-pushed the issue-1480 branch 2 times, most recently from 5685c8c to ec39242 Compare March 2, 2019 15:17

tja4472 force-pushed the issue-1480 branch 2 times, most recently from 14a803f to 52999ad Compare March 2, 2019 15:23

brandonroberts requested changes Mar 8, 2019

View reviewed changes

modules/store/src/action_creator.ts Outdated Show resolved Hide resolved

modules/store/src/action_creator.ts Outdated Show resolved Hide resolved

tja4472 force-pushed the issue-1480 branch from a3c4ee4 to 52a8470 Compare March 10, 2019 10:31

alex-okrushko reviewed Mar 11, 2019

View reviewed changes

tja4472 force-pushed the issue-1480 branch from 6bba23d to bb2c7b6 Compare March 20, 2019 18:13

tja4472 force-pushed the issue-1480 branch from bb2c7b6 to 52a8470 Compare March 20, 2019 18:36

tja4472 added 2 commits March 20, 2019 18:53

feat(Store): add action creator

c007649

refactor(Example): use action creators

ee0d102

tja4472 force-pushed the issue-1480 branch from 52a8470 to ee0d102 Compare March 20, 2019 18:55

tja4472 closed this Mar 25, 2019

feat(Store): add action creators #1570

feat(Store): add action creators #1570

Conversation

tja4472 commented Feb 20, 2019

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

coveralls commented Feb 20, 2019 • edited Loading

ngrxbot commented Feb 20, 2019

ngrxbot commented Feb 21, 2019

ngrxbot commented Feb 21, 2019

ngrxbot commented Feb 21, 2019

ngrxbot commented Feb 22, 2019

brandonroberts commented Feb 22, 2019

brandonroberts left a comment

Choose a reason for hiding this comment

ngrxbot commented Feb 23, 2019

ngrxbot commented Feb 24, 2019

ngrxbot commented Feb 25, 2019

ngrxbot commented Feb 26, 2019

ngrxbot commented Feb 28, 2019

timdeschryver left a comment

Choose a reason for hiding this comment

ngrxbot commented Mar 2, 2019

ngrxbot commented Mar 2, 2019

ngrxbot commented Mar 2, 2019

ngrxbot commented Mar 2, 2019

ngrxbot commented Mar 2, 2019

ngrxbot commented Mar 2, 2019

ngrxbot commented Mar 2, 2019

osnoser1 commented Mar 3, 2019

brandonroberts commented Mar 4, 2019

ngrxbot commented Mar 9, 2019

ngrxbot commented Mar 9, 2019

ngrxbot commented Mar 10, 2019

tja4472 commented Mar 10, 2019

alex-okrushko Mar 11, 2019

Choose a reason for hiding this comment

brandonroberts Mar 12, 2019

Choose a reason for hiding this comment

alex-okrushko Mar 12, 2019

Choose a reason for hiding this comment

alex-okrushko Mar 12, 2019

Choose a reason for hiding this comment

brandonroberts Mar 12, 2019

Choose a reason for hiding this comment

alex-okrushko commented Mar 19, 2019

brandonroberts commented Mar 19, 2019

brandonroberts commented Mar 19, 2019

alex-okrushko commented Mar 19, 2019

timdeschryver commented Mar 19, 2019

brandonroberts commented Mar 19, 2019

ngrxbot commented Mar 20, 2019

ngrxbot commented Mar 20, 2019

ngrxbot commented Mar 20, 2019

tja4472 commented Mar 20, 2019

brandonroberts commented Mar 25, 2019

tja4472 commented Mar 25, 2019

coveralls commented Feb 20, 2019 •

edited

Loading