Store: improve action creators

[alex-okrushko]

Problem

1. Actions are consumed by STRING or ENUM types, however created by class/function which makes them hard to track through IDEs or other search tools. At some point our team resorted to creating the visualizer to try to tackle that problem. "Good action hygiene" helps, but only in devtools.

// component.ts
this.store.dispatch(new MyAction('blah')); // uses Class symbol

// effect.ts
ofType(MY_ACTION_TYPE) // uses just string or enum
map(() => new MyOtherAction()) // uses Class symbol

// reducer.ts
case MY_ACTION_TYPE: // uses string or enum

There has been a number of takes on this already (#584, current function-base action creator #1480).

2. Class-based creators is the most used way to create Actions within Google's codebase (to the point where I couldn't find even a single place where function-based are used) and having const string MY_ACTION_TYPE (instead of type enum) to accompany the Class itself is also something that is frequently done.
   However, recently new linter rule was added that requires any exported symbols to have the comment. This ruled turned any Action into the following:

/** comment is required for exported const */
export const MY_ACTION_TYPE = 'type1';
/** comment is required as well, which would duplicate the type comment, 
  * which is really annoying */
export class MyAction implements Action {
  readonly type = MY_ACTION_TYPE;
  
  constructor(readonly x: number) {}  
}

Describe any alternatives/workarounds you're currently using

None, however there were proposals to duplicate readonly type with static type on the same class.

Proposal

To address both of the issues I suggest to create function returning Class that would "enhance" the action creator.

/** comment is required for exported classes */
export class MyAction extends typedAction('type1') { // <--- do this instead of readonly type
  constructor(readonly x: number) {
    super(); // this is the only annoying part. Needed when Action constructor has parameters.
  }
}

typedAction here would create a class and add both readonly type and static type to the class.
Suggestions about the naming of that function are very welcomed. I heard withAction, however not sure if that provides enough context about what it does.

New usage would be

// component.ts
this.store.dispatch(new MyAction('blah')); // uses Class symbol

// effect.ts
ofType(MyAction.type) // uses Class symbol with static type prop
map(() => new MyOtherAction()) // uses Class symbol

// reducer.ts
case MyAction.type: // uses Class symbol with static type prop

Implementation

Implementation requires 3 parts:

• the interface that describes the Class has the static type
• abstract class that requires both static and readonly type
• the function that ties all of it together

// standard NgRx built-in interface
interface Action {
  type: string;
}

// Make all of the following as part of NgRx:

export interface TypedAction<T extends string, A extends TypedStaticAction<T>> {
  new(...args: any[]): A;
  readonly type: T;
}

export abstract class TypedStaticAction<T extends string = string> implements
    Action {
  static type: string;
  // ideally this should be abstract, but return type is this anonymous class as well,
  // which forces Actions to re-implement them, so adding asserting operator. 
  readonly type!: T;
}

export function typedAction<T extends string>(type: T):
    TypedAction<T, TypedStaticAction<T>> {
  return class extends TypedStaticAction<T> {
    readonly type = type;
    static type = type;
  }
}

PLAYGROUND LINK

Limitation

Ideally I'd want readonly type to be abstract, and TS is supporting it

export abstract class TypedStaticAction<T extends string = string> implements
    Action {
  static type: string;
  abstract readonly type: T; // <---- notice the abstract identifier
}

However TS does not seem to like when I use this anonymous Class as a return type of and requires that Actions themselves reimplement it.

Non-abstract class 'A1' does not implement inherited abstract member 'type' from class 'TypedStaticAction<"type1">'

PLAYGROUND LINK

Alternative implementation

Alternative would be to help TS with that limitation and extract readonly type into another interface:

export interface TypedAction<T extends string, A extends TypedStaticAction<T>> {
  new(...args: any[]): A;
  readonly type: T;
}

export interface TypedStaticAction<T extends string> extends StaticAction, Action {
  readonly type: T;
}

abstract class StaticAction {
  static type: string;
}

export function typedAction<T extends string>(type: T):
    TypedAction<T, TypedStaticAction<T>> {
    return class extends StaticAction implements Action {
        readonly type = type;
        static type = type;
    };
}

PLAYGROUND LINK

In this cases static class doesn't have to be exported, so there would still be three exported symbols.

If accepted, I would be willing to submit a PR for this feature

[x] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

[alex-okrushko]

Further discussion lead to the advantage of the single createAction function that resembles createSelector.

The action creator could be further simplified to

export const myAction = createAction('type1', props<{x: number}>());

and the usage would be

// component.ts
this.store.dispatch(myAction({x: 5})); // uses function symbol

// effect.ts
ofType(myAction.type) // uses the same symbol with static type prop
map(() => myOtherAction()) // same symbol, without `new`

// reducer.ts
case myAction.type: // uses function symbol with static type prop

The implementation would be heavily inspired ({read: copied with minor adjustments}) from the ts-actions library by @cartant

[timdeschryver]

advantage of the single createAction function that resembles createSelector.

👍👍

I would also like the following to be possible - https://github.com/cartant/ts-action#creator
Because this gives the opportunity to set some default values, or to do something for example a calculation to set a property's value.

const foo = action("FOO", (name: string) => ({ name }));
const fooAction = foo("alice");
console.log(fooAction); // { type: "FOO", name: "alice" }

I think it would also be possible to allow an ofType like this, right?

ofType(myAction)

[alex-okrushko]

I would also like the following to be possible - https://github.com/cartant/ts-action#creator

Yes, I plan to leave the options of creator function, props() and payload().
empty() and fsa() aren't needed.

I think it would also be possible to allow an ofType like this, right? ofType(myAction)

ts-actions has isType(myAction) and guard(myAction).
I think we can adjust the ofType to handle these actions created with createAction, so that .type is no longer necessary - that would be similar to what guard is doing now.

[tja4472]

Have ended up with an example app which shows ngrx using classes, action creators & ts-action.
https://github.com/tja4472/ngrx-ts-action

[alex-okrushko]

We also discussed to drop payload() function.

Why? Payload was serving as a container object to be passed constructor, so that property names had to be explicitly specified, e.g.

// Unclear which props these arguments would be assigned to.
new MyAction('blah', 'blah blah', foo, bar);

// much better and explicit
new MyAction({
  prop1: 'blah',
  prop2: 'blah blah',
  prop3: foo,
  bar,
});

However, accessing properties through payload is a bit cumbersome. action.payload.prop1.

props<T>() function solves both of these problems.

Note, that if one still wishes to continue using payload that could be done via
props<{payload: {....}}>().

I'll send a PR for this proposal soon.

[itayod]

What about adding factory for async flow that will create success, falied (and pending) actions?

I think it would reduce the boilerplate of create async flow, and will help to create standard conventions for async actions.

Suggestions of how the api will be:

 const loadFeature =
  createAsyncAction<REQ, // the request payload type  
                      RES, // the response success payload type 
                      ERR // the response failure payload type  
                     >('[Feature] Load Feature');

console.log(loadFeature);
// {load: Action<REQ>, success: Action<RES>, fail: Action<ERR>, pending: Action}

Hope you like the idea, will be happy to help with PR. 😃

[alex-okrushko]

@itayod that would violate Good Action Hygiene™️ - actions types should include the source in their type.

[itayod]

@alex-okrushko so just to make things clear, what you mean is that it will violate the Good Action Hygiene™️ because the action types for success and failure are created by the function?

If so, I got your point, It could also effect the reducer typing because the actions will not be included in the FeatureActionTypes enum.

However, I think the value here in terms of naming standards and boilerplate reduce are two strong advantages here.

[timdeschryver]

Closed via #1654