This is my thinking on this issue.

Definition

export type AddTodo = { 
   type: 'ADD_TODO', 
   payload: {description: string, checked: boolean} 
};
export function addTodo(payload: AddTodo['payload']): AddTodo {
  return { type: 'ADD_TODO', payload };
}

export class AddTodo implements Action {
  readonly type = ADD_TODO;
  constructor(public payload: {description: string, checked: boolean}) {}
}

When it comes to defining actions, the first one is a bit more verbose, but, at the same time, it's a bit more flexible.

Usage

store.dispatch(addTodo({description: 'blah', checked: false}));

store.dispatch(new AddTodo({description: 'blah', checked: false}));

The usage is about the same. Both provide the same type-safety and autocompletion.

Why I'm not a big fan of using classes

I like my actions being just data. Using classes break that.

For instance, they are no longer serializable.

const clonedAction = JSON.parse(JSON.stringify(action))

Whereas, action instanceof AddTodo returns true, clonedAction instaceof AddTodo returns false. This also means that if we define a getter or a method, it won't work after the deserialization.

That's why I tend to favour action constructors instead of classes.

What do you think?

@vsavkin Bro, that's an interesting use case for function type actions, I've never thought about serializing them, in which kind of situation had you use this technique?

@vsavkin I get your point but then I guess using this approach it is not possible to get action type as string which is required for data persistence helpers that NX toolkit provides. Correct me if I am wrong.

@vsavkin those clones might be an issue although for people with Java background classes make more sense. Also I'm not sure if instanceof works even after code mangling anyways.

@ogix

export class AddTodo implements Action {
  readonly type = ADD_TODO;
  constructor(public payload: {description: string, checked: boolean}) {}
}
....
store.dispatch(new AddTodo({description: 'blah', checked: false}));
....
@Effect()
  public addTodo$: Observable<Action> = this.actions$
    .ofType(ADD_TODO) // event type to listen
    .switchMap( ( action: AddTodo) => {
       // action.type == ADD_TODO
    }

@MattiJarvinen-BA
I mean using the first approach with type and function that @vsavkin suggests. I know how to do it when you have exported actions as strings - I am the author of this PR :)

@ogix
Could you expand on how DataPersistence will be affected? I think it should work exactly the same way with both the approaches.

@MattiJarvinen-BA

those clones might be an issue although for people with Java background classes make more sense. Also I'm not sure if instanceof works even after code mangling anyways.

Minification doesn't break instanceof.

Just to be clear, I don't think there are a lot of use cases requiring serializing and deserializing actions. It's possible to come up with some (e.g., sending actions via a websocket from the backend or some dev tools stuff), but they aren't very common.

What I think is important here is the following:

• NgRx is a messaging system.
• Actions are messages.
• Messages should be pure data, they should not contain any behavior. Reducers and effects contain behavior.

That's why I'm not a big fan of using classes. They are designed to combine data and behavior. Even though it is possible not to do it, lots of folks will. This is especially true about the folks who have Java and .Net background.

The only reason to use classes is to support type completion on the creation side, right? you could always do the following:

s.dispatch<TodoActions>({
  type: 'ADD_TODO',
  payload: ...
})

you will get type safety and the overhead of creating an action is minimal:

export type AddTodo = { type: 'ADD_TODO', payload: {description: string, checked: boolean} };
export type TodoActions = AddTodo|...

@vsavkin It is completely possible and reasonable to do typed actions like you suggest, actually that was the way it was done before classes were introduced into actions.

The benefit of actions are both the types and convenience, when you compare the following code you can notice the class way is easier to understand (at least for me):

s.dispatch({
  type: 'ADD_TODO',
  payload: { ... },
})
// ----------------------------------------
s.dispatch(new todoActions.addTodo({ ... }))

Listing the benefits of class based actions are:

• Typed actions
• Easier refactor support in case you need to change the name of any action, or it's payload name (in my case we don't use payload but the actual item, if it's a todo, our payload would be named todo).
• Not typing string literals all over the place (refactor again), imagine you need to change the type string, you would need to go to each file and change each string action.

Right now i don't remember more benefits, and the overhead is not that big as to justify writing code harder to maintain, so i favor class based actions.

@luchillo17

I see your point. Let me chat a bit more with the rest of the NgRx core team. I don't remember why we went with classes over action constructors.

Hi Is there an update on this PR? are we good with using Classes to proceed with?

@vsavkin - I personally like Action classes

The original redux throws an error if you use classes instead of plain objects. I think Victors arguments are solid why POJOs are preferable. Classes makes writing actions a little bit shorter, that's true, but with action creator functions you will get the same user experience: s.dispatch(addTodo('Use Plain Objects'))
With typescript 2.8 and the new ReturnType it will even get more easy to get type safe actions.
Look at this article: https://medium.com/@martin_hotell/improved-redux-type-safety-with-typescript-2-8-2c11a8062575
I'm using this already with the createAction helper and the UnionType-Helper. It's so easy and fast to write actions now!

@BeaveArony The action creators are making the same thing as a class, you hardcode the type in the action creator, so in theory class actions are action creators in the constructor, just think in the class as a fancier action creator.

That aside, i think it's nice being consistent, but by using the same way as redux we're just dumping away all the nice things TypeScript give us, i think consistency is not enough reason to halt improvements (or progress).

So i think at this point we have discussed a good amount on the subject, pros & cons stated, at this point we're informed enough to take a decision, since we seems to be split in opinion we have 2 options:

• Poll to decide by quantity of people (Democracy?).
• Implement both behind a flag & decide which will be the default (How would you call this one?).

@vsavkin I'll let the details of that to you.

Closing since #359 uses @ngrx/schematics and ngrx-generated action classes.

For those that doesn't know about @ngrx/schematics, they have some nice defaults i didn't thought about, like the ActionTypes inside an enum for better organization, and they use class actions by default, so seems that this discussion comes to an end with the use of their schematics.

This should be fixed with SHA 4edc8e6.

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.