New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interface Action should be generic. #128

Closed
ovangle opened this Issue Jul 20, 2017 · 20 comments

Comments

Projects
None yet
9 participants
@ovangle

ovangle commented Jul 20, 2017

This issue is a resubmission (and improvement) of the proposal https://github.com/ngrx/store/issues/297: Action to be made generic.


New Action<T> Interfaces

ActionType represents the interface that is statically implemented by an action.

It guarantees that Action must be constructable using a single parameter, and be declared as part of an NgModule.

interface ActionType<T> {
  /**
   * The `NgModule` that this action is declared as part of.
   */
  ngModule: Type<T>;
  /**
   * The action is carrying information of type `payload`
   */
  new (payload: T): Action<T>;
}

 /**
  * Represents an instruction that the reducer will apply to the module/global state.
  */
 interface Action<T> {
    readonly payload: T;
 }

The following declarations would all be a conforming Actions:

class VoidAction implements Action<null> {
   static ngModule: AppModule;
   constructor(public readonly payload: null) {}
}

class IncrementAction implements Action<number> {
   static ngModule: MathModule;
   constructor(public readonly payload: number) {}
}

class ComplexAction implements Action<ComplexInterface> {
    constructor (public readonly payload: ComplexInterface) {}

Some utility functions

It needs to be possible to obtain a value which corresponds to the old action.type.

/** 
 * This function can be used wherever there was previously a call to `action.type`, 
 * which is expected to return a string.
 */
export function actionTypeToString(action: Action<T>): string {
    let type = actionType(action);
    function stringify(type: Type<T>) { return type.constructor.name; }
    return `${stringify(actionType.module)} (${stringify(type)})`;
}


export function actionType(action: Action<T>): ActionType<T> {
    return Object.getPrototypeOf(action).constructor;
}

See this attached gist for an example of using the improved interface for Action, and how the example (despite being a stupid example) feels more "angular"y, since it uses the static type of an object as a token...


Simple inline actions

By making the interface for Action rely upon the runtime type of the action for use in reducers, it removes the possibility for calls like the following:

store.dispatch({type: 'some-quick-and-dirty-string', payload: Math.PI});

However, we can workaround this by enforcing a simple rule throughout the code:

```
action has a property 'type' and the value is a string? Use it. Else, try use the runtime type.
```

Thus, for example, the function actionTypeToString would actually be

 export function actionTypeToString(action: Action<T>): string {
    if (typeof action.type === "string") {
        return action.type;
    } else {
       ... (see above)
    }
}

Void actions

The syntax for declaring a conforming Action<Void> (no payload) is a little clunky, and could be improved by changing the ActionType and Action interfaces to make the payload parameter optional. There are a number of common use cases (eg. a delete resource action) which would make this improvement justified.

@jinder

This comment has been minimized.

Show comment
Hide comment
@jinder

jinder Jul 20, 2017

Contributor

@ovangle why must there be a single payload property?

Contributor

jinder commented Jul 20, 2017

@ovangle why must there be a single payload property?

@ovangle

This comment has been minimized.

Show comment
Hide comment
@ovangle

ovangle Jul 20, 2017

Because my intuitive idea of Action is "Procedure (typeString) with arguments (payload) that represents an update of the global/module state". Now I may be completely off the mark, but that's the way I've always seen it be used in practice.

Having a single "payload" property on every Action allows you to codify (and thus typecheck) the notion that the payload represents the "arguments". If you change the ActionType interface so that there is no single payload property

interface ActionType<T> {
   new (...args: any[]): Action<T>;
}

Then you lose any connection between T and Action<T>, and the T parameter actually just becomes a useless appendage on the interface.

Basically, there always needs to be a typed property T on an object of a type with generic T, that's the way generics work. You can call it payload or you can call it anything else (but I think using "payload" is apt and reflects the current usage).

One improvement that could be made (to improve the declaration of void actions) is:

export interface ActionType<T> { 
    ...
    new (payload?: T) {}
}

export interface Action<T> {
    readonly payload?: T;
}

It depends how common you think void typed actions are.

ovangle commented Jul 20, 2017

Because my intuitive idea of Action is "Procedure (typeString) with arguments (payload) that represents an update of the global/module state". Now I may be completely off the mark, but that's the way I've always seen it be used in practice.

Having a single "payload" property on every Action allows you to codify (and thus typecheck) the notion that the payload represents the "arguments". If you change the ActionType interface so that there is no single payload property

interface ActionType<T> {
   new (...args: any[]): Action<T>;
}

Then you lose any connection between T and Action<T>, and the T parameter actually just becomes a useless appendage on the interface.

Basically, there always needs to be a typed property T on an object of a type with generic T, that's the way generics work. You can call it payload or you can call it anything else (but I think using "payload" is apt and reflects the current usage).

One improvement that could be made (to improve the declaration of void actions) is:

export interface ActionType<T> { 
    ...
    new (payload?: T) {}
}

export interface Action<T> {
    readonly payload?: T;
}

It depends how common you think void typed actions are.

@RicardoVaranda

This comment has been minimized.

Show comment
Hide comment
@RicardoVaranda

RicardoVaranda Jul 20, 2017

After upgrading to ngrx 4 my application is spamming me with
Property 'payload' does not exist on type 'Action'.

Pointed at https://github.com/ngrx/platform/blob/master/modules/store/src/models.ts#L1

Did you guys have this issue also, not really sure how to fix this.

RicardoVaranda commented Jul 20, 2017

After upgrading to ngrx 4 my application is spamming me with
Property 'payload' does not exist on type 'Action'.

Pointed at https://github.com/ngrx/platform/blob/master/modules/store/src/models.ts#L1

Did you guys have this issue also, not really sure how to fix this.

@ovangle

This comment has been minimized.

Show comment
Hide comment
@ovangle

ovangle Jul 20, 2017

Yep, that is correct -- there is no payload parameter on the Action interface (see here) and I'm arguing that it should be added.

So the fix (at the moment) is to coerce the type of all your calls to payload:

(action as any).payload

or to add interface HasPayload {payload: any} and then:

 (<HasPayload>action).payload

ovangle commented Jul 20, 2017

Yep, that is correct -- there is no payload parameter on the Action interface (see here) and I'm arguing that it should be added.

So the fix (at the moment) is to coerce the type of all your calls to payload:

(action as any).payload

or to add interface HasPayload {payload: any} and then:

 (<HasPayload>action).payload
@chrissena

This comment has been minimized.

Show comment
Hide comment
@chrissena

chrissena Jul 20, 2017

@ovangle @RicardoVaranda
Am I missing something?

In ngrx4 you can already just create an interface (e.g. MyActionInterface) that implements the provided Action interface with any extra properties you like called whatever you like, payload etc..(as long as it has the compulsory type property).

Your actions can all then just implement this interface. When creating effects the Actions stream is already generic (with default type parameter of Action). You can use..

constructor(private actions$: Actions<MyActionInterface>){}

and

@Effect()
   public sideEffect$: Observable<MyActionInterface> = this.actions$
        .ofType('ACTION_I_NEED')
        .map(a:ActionINeedType => ....

as usual and not have to worry about coercion etc. Just use MyActionInterface everywhere you would have used the Action interface (e.g. in reducer parameter definitions).

I'm using this now and it works a treat.

chrissena commented Jul 20, 2017

@ovangle @RicardoVaranda
Am I missing something?

In ngrx4 you can already just create an interface (e.g. MyActionInterface) that implements the provided Action interface with any extra properties you like called whatever you like, payload etc..(as long as it has the compulsory type property).

Your actions can all then just implement this interface. When creating effects the Actions stream is already generic (with default type parameter of Action). You can use..

constructor(private actions$: Actions<MyActionInterface>){}

and

@Effect()
   public sideEffect$: Observable<MyActionInterface> = this.actions$
        .ofType('ACTION_I_NEED')
        .map(a:ActionINeedType => ....

as usual and not have to worry about coercion etc. Just use MyActionInterface everywhere you would have used the Action interface (e.g. in reducer parameter definitions).

I'm using this now and it works a treat.

@MikeRyanDev

This comment has been minimized.

Show comment
Hide comment
@MikeRyanDev

MikeRyanDev Jul 20, 2017

Member

The only part of the action interface that you must implement is the type. When we had payload on the interface marked as any this introduced type safety issues, especially when interacting with the Actions stream in @ngrx/effects. We won't be making any changes to this interface at this time.

If you want a parameterized version you can create your own:

export interface ActionWithPayload<T> extends Action {
  payload: T;
}

And if you need an unsafe version to help with transition you can create that, too:

export interface UnsafeAction implements Action {
  payload?: any;
}
Member

MikeRyanDev commented Jul 20, 2017

The only part of the action interface that you must implement is the type. When we had payload on the interface marked as any this introduced type safety issues, especially when interacting with the Actions stream in @ngrx/effects. We won't be making any changes to this interface at this time.

If you want a parameterized version you can create your own:

export interface ActionWithPayload<T> extends Action {
  payload: T;
}

And if you need an unsafe version to help with transition you can create that, too:

export interface UnsafeAction implements Action {
  payload?: any;
}
@jinder

This comment has been minimized.

Show comment
Hide comment
@jinder

jinder Jul 20, 2017

Contributor

@ovangle I think your proposal makes the assumption that there should be a payload property on the action object. My actions do not have a single payload property, and I'm not sure ngrx should enforce it.

Contributor

jinder commented Jul 20, 2017

@ovangle I think your proposal makes the assumption that there should be a payload property on the action object. My actions do not have a single payload property, and I'm not sure ngrx should enforce it.

@ovangle

This comment has been minimized.

Show comment
Hide comment
@ovangle

ovangle Jul 21, 2017

@MikeRyanDev @jinder

That's disappointing but understandable. I know the redux community had a similar discussion about standardising the shape of Action and I know the conclusion that they came to, but I think in ngrx there is a far stronger case to be made for imposing additional structure (because typescript). I know that additionally, since you've just issued a v4 which completely removes payload, then that significantly reduces the chance that you'll bring an improved version of it back. It's unfortunate that my comment on the old issue was ignored, but it was, so...

I wish I could motivate you both to try out the changes I proposed locally and then come to a decision about whether the change is worth it, but I can't.

That said, the generic Actions does remedy a lot of the problems which motivated the proposal in the first place, so...

In the meantime, here's a gist which implements a modularised AppState that I wrote last night, something that might have been possible if this proposal was given the consideration it deserved.

ovangle commented Jul 21, 2017

@MikeRyanDev @jinder

That's disappointing but understandable. I know the redux community had a similar discussion about standardising the shape of Action and I know the conclusion that they came to, but I think in ngrx there is a far stronger case to be made for imposing additional structure (because typescript). I know that additionally, since you've just issued a v4 which completely removes payload, then that significantly reduces the chance that you'll bring an improved version of it back. It's unfortunate that my comment on the old issue was ignored, but it was, so...

I wish I could motivate you both to try out the changes I proposed locally and then come to a decision about whether the change is worth it, but I can't.

That said, the generic Actions does remedy a lot of the problems which motivated the proposal in the first place, so...

In the meantime, here's a gist which implements a modularised AppState that I wrote last night, something that might have been possible if this proposal was given the consideration it deserved.

@ovangle

This comment has been minimized.

Show comment
Hide comment
@ovangle

ovangle Jul 22, 2017

Some notes on my mental model of a library like ngrx/redux.

A library like ngrx is a von neumann machine emulator. At it's most basic, it consists of:

  • The application state, or memory of the machine.
  • The actions, or instruction set of the machine
  • The reducer, or CPU of the machine.

Each operation of the reducer on the memory applies a patch to the application state, contained in it's payload. But instead of an instruction set of basic math, memory access, jumps etc., the instructions of our machine can apply patches of structured data, into memory which is also structured data.

In order to clearly demonstrate the idea of ngrx as a von neumann machine emulator, I've implemented a turing machine:

class Tape {
   // The TERMINATE condition for our machine is moving the head to an invalid position
   static isTerminated(tape) { return tape.head < 0; }
   static isContinue(tape) { return !Tape.isTerminated(tape); }

   // Read the symbol at HEAD
   static read(tape) { return tape.contents[tape.head]; }
   // Get the head position
   static position(tape) { return tape.head; }

   readonly head: number;
   readonly contents: Array<number>;
   constructor(head, contents) {}
}

class Reset implements VoidAction {}
class Write implements Action<{value?: number}> {
  constructor(public payload: {value?: number}) {}
}
class Move implements Action<{offset: number}> {
  constructor(public payload: {offset: number}) {}
}

function turingReducer(tape: Tape, action: Reset | Move | Write) {
   switch (actionType(action)) {
       case Reset:
           return new Tape(0, []);
       case Write:
           contents = tape.contents.splice();
           contents[tape.head] = action.payload.value;
           return new Tape(tape.head, contents);
      case Move:
           return new Tape(tape.head + action.payload.offset, contents);
      default:
           return tape;
   }
}

// An example of using our machine to compute the fibonacci numbers
function fibonacci(store) {
    // Some boilerplate to step the machine through it's computation
    return doWhileAsync(
       () => store.select(Tape.isContinue).first().toPromise(),

       // The actual transition function.
       async function () {
          let position = await store.select(Tape.position).first().toPromise();
          if (position === 0) {
             // Write 0,1 at the head of the tape.
             store.dispatch(new Reset());
             store.dispatch(new Write({value: 0});
             store.dispatch(new Move({offset: 1});
             store.dispatch(new Write({value: 1});
             store.dispatch(new Move({offset: 1});
          } else {
             // Write the next fibonacci number at the head tape
             store.dispatch(new Move({offset: -2});
             let fib_a = await store.select(Tape.read).first().toPromise();
             store.dispatch(new Move({offset: 1});
             let fib_b = await store.select(Tape.read).first().toPromise();
             store.dispatch(new Move({offset: 1});
             store.dispatch(new Write({value: fib_a + fib_b});

             // And move to the position of the next write
             store.dispatch(new Move({offset: 1});
         }
    });
}

Notice how similar the reduction to a turing machine is to the reduction in the standard proof for turing completeness of a von neumann machine?

Why the hell am I telling you this?!?

You wrote the library, I expect you have a clearer idea of what your library is than I do. But sometimes it's important to be explicit about these things, to make sure we're all on the same page. That way if I have some grave misunderstanding about what the library is, you can correct me up front and we can both move on.

Von neumann instructions

All instructions share a similar scheme:

load                  $1 0x0234123
----                   --------------
Instruction        payload

The CPU reads a stream of instructions and each instruction patches the memory state. It's just that in our machine, our instructions can any semantically meaningful verbs, and our payloads can be arbitrary structured javascript objects.

So, what does this look like in the type system?

Well, using the current Action interface, it looks like

interface Action {
   type: string;                 // instruction
   [key: string]: any;        // patch to apply to the global state.
                                        // This is hidden in the current interface, because it is 
                                        // (rightly) assumed that all actions are javascript objects
}

Why this is fine in redux

Redux is javascript. All that a dynamic language cares about is that the data is structured, it doesn't care if it's possible to meaningfully reflect on the structure. So for javascript, Object.keys(action) is a perfectly reasonable way of reflecting on the payload.

Why this is a problem in ngrx

Ngrx is typescript. Users expect to be able to use the type information exposed by an object in order to make inferences about the object. But there's a problem, because the type of the patch part of the instruction is currently:

  type payload = { [key: string]: any};

It's completely untyped. Can't use that for anything :/

How ngrx can do better.

By explicitly including the payload of the instruction in the interface for Action,

 interface Action<T> {
    type: string;
    payload: T;
}

then we can actually read the payload.

@jinder

There's no reason you can't add getters/setters to your Actions so that they keep whatever arbitrary key/values you like to add to them:

 class SomeCoolAction<SomeCoolInterface> {
    constructor(public payload: SomeCoolInterface);

    get interestingProperty() { return this.payload.interestingProperty; }
 }

The only difference is that in your new actions, you've still got an explicitly typed exposed payloadto reflect on.

Not all instructions have a payload

No, they don't. But there's also no semantic difference between an instruction with a null payload and an instruction with no payload, so our model is still accurate in this case.

But there's still a problem if somebody just ignores the interface when creating an action

Even though payload: T exists on the interface, how do we know what to bind it to? A user could add any random properties to the object and payload becomes a hazard to type saftey.

The solution to this in typescript is to enforce a "static interface" (my terminology). A "static interface" is any interface which matches against the static type of a class (ie. It binds the constructor type using a new (<args>): <constructed_type> binding). This interface can be used to bind the constructor arguments to the payload

interface ActionType<T> {
   // This tells that all the configuration for an action is encapsulated
   // by a single constructor argument, the `payload`.
   // It makes it inconvenient to define a conforming action which isn't
   // initialised with an complete instruction patch.
   // It is assumed that actions are immutable.
   new (payload: T): Action<T>
}

Type checking against the static interface.

Unfortunately, while strings are used as proxies to the actual types, we never type check against this interface. So, in order for the static interface to be associated with the actions, we need to use the Types of the actions themselves (rather than arbitrary javascript strings), in order to type check our actions (this may be based on incorrect information) . If the type object is passed into a method which accepts ActionType<T>, then the type system will complain if the constructor arguments don't match.

Thus, instead of using a string to represent the type of an object, the runtime type of the object itself should instead be used as the token, in a fashion mimicking the angular dependency injection API (which is written to deal with a lot of the same problems). (this might also be incorrect, but using types would be a good stylistic change anyway)

**Note: ** To preserve interoperability with redux libraries , it should be always be valid to define a "type" string on an action.

Why typing Actions<T> isn't a solution.

Defining Actions<T> in Effects declares that all the Actions used here share a common interface. There's no reason to remove it, it's a good change.

However, it only provides a common base interface to use within the Effects module, it provides no way to reflect on the type of the payload. If you look at the turing reduction above, even that already has three different types of instruction payloads

  • void
  • {offset: number}
  • {value: number}

But what can accomplish by knowing the type of the patch?

The reducer is just a giant switch statement, and typescript doesn't infer that the type used in a case statement is the right restriction to the code inside the case (although conceivably in future such functionality could be added) (apparently the functionality has been added, it's just my editor that was broken), so the answer is not much, when it comes to typing the reducer. The typecasts will always be necessary.

But everywhere else? Go ahead, use those types

So that's why I wrote this proposal.

You're writing a typescript-first library, and you expect to distinguish yourselves from redux in some way. Allowing users to provide usable types to their instruction set payloads? That would be a great first step...

ovangle commented Jul 22, 2017

Some notes on my mental model of a library like ngrx/redux.

A library like ngrx is a von neumann machine emulator. At it's most basic, it consists of:

  • The application state, or memory of the machine.
  • The actions, or instruction set of the machine
  • The reducer, or CPU of the machine.

Each operation of the reducer on the memory applies a patch to the application state, contained in it's payload. But instead of an instruction set of basic math, memory access, jumps etc., the instructions of our machine can apply patches of structured data, into memory which is also structured data.

In order to clearly demonstrate the idea of ngrx as a von neumann machine emulator, I've implemented a turing machine:

class Tape {
   // The TERMINATE condition for our machine is moving the head to an invalid position
   static isTerminated(tape) { return tape.head < 0; }
   static isContinue(tape) { return !Tape.isTerminated(tape); }

   // Read the symbol at HEAD
   static read(tape) { return tape.contents[tape.head]; }
   // Get the head position
   static position(tape) { return tape.head; }

   readonly head: number;
   readonly contents: Array<number>;
   constructor(head, contents) {}
}

class Reset implements VoidAction {}
class Write implements Action<{value?: number}> {
  constructor(public payload: {value?: number}) {}
}
class Move implements Action<{offset: number}> {
  constructor(public payload: {offset: number}) {}
}

function turingReducer(tape: Tape, action: Reset | Move | Write) {
   switch (actionType(action)) {
       case Reset:
           return new Tape(0, []);
       case Write:
           contents = tape.contents.splice();
           contents[tape.head] = action.payload.value;
           return new Tape(tape.head, contents);
      case Move:
           return new Tape(tape.head + action.payload.offset, contents);
      default:
           return tape;
   }
}

// An example of using our machine to compute the fibonacci numbers
function fibonacci(store) {
    // Some boilerplate to step the machine through it's computation
    return doWhileAsync(
       () => store.select(Tape.isContinue).first().toPromise(),

       // The actual transition function.
       async function () {
          let position = await store.select(Tape.position).first().toPromise();
          if (position === 0) {
             // Write 0,1 at the head of the tape.
             store.dispatch(new Reset());
             store.dispatch(new Write({value: 0});
             store.dispatch(new Move({offset: 1});
             store.dispatch(new Write({value: 1});
             store.dispatch(new Move({offset: 1});
          } else {
             // Write the next fibonacci number at the head tape
             store.dispatch(new Move({offset: -2});
             let fib_a = await store.select(Tape.read).first().toPromise();
             store.dispatch(new Move({offset: 1});
             let fib_b = await store.select(Tape.read).first().toPromise();
             store.dispatch(new Move({offset: 1});
             store.dispatch(new Write({value: fib_a + fib_b});

             // And move to the position of the next write
             store.dispatch(new Move({offset: 1});
         }
    });
}

Notice how similar the reduction to a turing machine is to the reduction in the standard proof for turing completeness of a von neumann machine?

Why the hell am I telling you this?!?

You wrote the library, I expect you have a clearer idea of what your library is than I do. But sometimes it's important to be explicit about these things, to make sure we're all on the same page. That way if I have some grave misunderstanding about what the library is, you can correct me up front and we can both move on.

Von neumann instructions

All instructions share a similar scheme:

load                  $1 0x0234123
----                   --------------
Instruction        payload

The CPU reads a stream of instructions and each instruction patches the memory state. It's just that in our machine, our instructions can any semantically meaningful verbs, and our payloads can be arbitrary structured javascript objects.

So, what does this look like in the type system?

Well, using the current Action interface, it looks like

interface Action {
   type: string;                 // instruction
   [key: string]: any;        // patch to apply to the global state.
                                        // This is hidden in the current interface, because it is 
                                        // (rightly) assumed that all actions are javascript objects
}

Why this is fine in redux

Redux is javascript. All that a dynamic language cares about is that the data is structured, it doesn't care if it's possible to meaningfully reflect on the structure. So for javascript, Object.keys(action) is a perfectly reasonable way of reflecting on the payload.

Why this is a problem in ngrx

Ngrx is typescript. Users expect to be able to use the type information exposed by an object in order to make inferences about the object. But there's a problem, because the type of the patch part of the instruction is currently:

  type payload = { [key: string]: any};

It's completely untyped. Can't use that for anything :/

How ngrx can do better.

By explicitly including the payload of the instruction in the interface for Action,

 interface Action<T> {
    type: string;
    payload: T;
}

then we can actually read the payload.

@jinder

There's no reason you can't add getters/setters to your Actions so that they keep whatever arbitrary key/values you like to add to them:

 class SomeCoolAction<SomeCoolInterface> {
    constructor(public payload: SomeCoolInterface);

    get interestingProperty() { return this.payload.interestingProperty; }
 }

The only difference is that in your new actions, you've still got an explicitly typed exposed payloadto reflect on.

Not all instructions have a payload

No, they don't. But there's also no semantic difference between an instruction with a null payload and an instruction with no payload, so our model is still accurate in this case.

But there's still a problem if somebody just ignores the interface when creating an action

Even though payload: T exists on the interface, how do we know what to bind it to? A user could add any random properties to the object and payload becomes a hazard to type saftey.

The solution to this in typescript is to enforce a "static interface" (my terminology). A "static interface" is any interface which matches against the static type of a class (ie. It binds the constructor type using a new (<args>): <constructed_type> binding). This interface can be used to bind the constructor arguments to the payload

interface ActionType<T> {
   // This tells that all the configuration for an action is encapsulated
   // by a single constructor argument, the `payload`.
   // It makes it inconvenient to define a conforming action which isn't
   // initialised with an complete instruction patch.
   // It is assumed that actions are immutable.
   new (payload: T): Action<T>
}

Type checking against the static interface.

Unfortunately, while strings are used as proxies to the actual types, we never type check against this interface. So, in order for the static interface to be associated with the actions, we need to use the Types of the actions themselves (rather than arbitrary javascript strings), in order to type check our actions (this may be based on incorrect information) . If the type object is passed into a method which accepts ActionType<T>, then the type system will complain if the constructor arguments don't match.

Thus, instead of using a string to represent the type of an object, the runtime type of the object itself should instead be used as the token, in a fashion mimicking the angular dependency injection API (which is written to deal with a lot of the same problems). (this might also be incorrect, but using types would be a good stylistic change anyway)

**Note: ** To preserve interoperability with redux libraries , it should be always be valid to define a "type" string on an action.

Why typing Actions<T> isn't a solution.

Defining Actions<T> in Effects declares that all the Actions used here share a common interface. There's no reason to remove it, it's a good change.

However, it only provides a common base interface to use within the Effects module, it provides no way to reflect on the type of the payload. If you look at the turing reduction above, even that already has three different types of instruction payloads

  • void
  • {offset: number}
  • {value: number}

But what can accomplish by knowing the type of the patch?

The reducer is just a giant switch statement, and typescript doesn't infer that the type used in a case statement is the right restriction to the code inside the case (although conceivably in future such functionality could be added) (apparently the functionality has been added, it's just my editor that was broken), so the answer is not much, when it comes to typing the reducer. The typecasts will always be necessary.

But everywhere else? Go ahead, use those types

So that's why I wrote this proposal.

You're writing a typescript-first library, and you expect to distinguish yourselves from redux in some way. Allowing users to provide usable types to their instruction set payloads? That would be a great first step...

@jinder

This comment has been minimized.

Show comment
Hide comment
@jinder

jinder Jul 22, 2017

Contributor

@ovangle (FYI, I don't work on NGRX, I just submitted a PR to fix an issue). I was intrigued by this issue, because my actions don't have a payload property, and was wondering whether this was a good or bad decision on my part.

I still really don't see why NGRX needs to enforce the payload property. If you want to use Action<T>, why not use it where you see fit? Why does NGRX need to enforce this convention?

Also, typecasts are not necessary in a reducer. If you ensure that your action's type property is readonly, you can use discriminated unions to get the correct type inference for your actions.

Contributor

jinder commented Jul 22, 2017

@ovangle (FYI, I don't work on NGRX, I just submitted a PR to fix an issue). I was intrigued by this issue, because my actions don't have a payload property, and was wondering whether this was a good or bad decision on my part.

I still really don't see why NGRX needs to enforce the payload property. If you want to use Action<T>, why not use it where you see fit? Why does NGRX need to enforce this convention?

Also, typecasts are not necessary in a reducer. If you ensure that your action's type property is readonly, you can use discriminated unions to get the correct type inference for your actions.

@ovangle

This comment has been minimized.

Show comment
Hide comment
@ovangle

ovangle Jul 23, 2017

If you ensure that your action's type property is readonly, you can use discriminated unions to get the correct type inference for your actions.

Discriminated unions still just give a shared common base for your action types, the typecasts are still necessary

function reducer(state: Tape, action: Move | Write | Reset) {
   switch (action.type) {
       case "Move":
          // This is apparently fine
          return new Tape(tape.head + action.payload.offset, contents);
   }
}

This however, as I've noted, can't be improved upon if you correct the Action interface.

I still really don't see why NGRX needs to enforce the payload property. If you want to use Action, why not use it where you see fit? Why does NGRX need to enforce this convention?

Because most of the important times you actually want to reflect on the type is from within the ngrx library code. Actions.ofType from the effects module would be the prime example of where any type information you supply is erased by the library.

The type signature should be:

  `Actions.ofType<T>(type: ActionType<T>): Observable<Action<T>>`

But it can't be, because the payload type information is not exposed.

because my actions don't have a payload property, and was wondering whether this was a good or bad decision on my part.

It's fine that your actions don't have a "payload" property. There's nothing bad about it, indeed that is exactly the redux model for how the payload should be included (cause redux don't care about dem none-fancy types or nuffin). The Action is considered a black box of key value pairs, and the only thing we know about it is the type key.

But I'm arguing that the library should enforce a typed payload property. This doesn't change anything for you -- you just do a five minute refactor of your code to change the "black box" into a series of getters (probably in an abstract base class somewhere), like in the example I gave you above. Then everywhere else in your code you can ignore the existence of payload entirely.

And by exposing that small amount of type information (and by using typestrings only when interoperating with redux libs), you'll see the whole ngrx API become a whole lot simpler.

Well, apart for the reducers. You will never be able restrict the type by entering a case statement.

ovangle commented Jul 23, 2017

If you ensure that your action's type property is readonly, you can use discriminated unions to get the correct type inference for your actions.

Discriminated unions still just give a shared common base for your action types, the typecasts are still necessary

function reducer(state: Tape, action: Move | Write | Reset) {
   switch (action.type) {
       case "Move":
          // This is apparently fine
          return new Tape(tape.head + action.payload.offset, contents);
   }
}

This however, as I've noted, can't be improved upon if you correct the Action interface.

I still really don't see why NGRX needs to enforce the payload property. If you want to use Action, why not use it where you see fit? Why does NGRX need to enforce this convention?

Because most of the important times you actually want to reflect on the type is from within the ngrx library code. Actions.ofType from the effects module would be the prime example of where any type information you supply is erased by the library.

The type signature should be:

  `Actions.ofType<T>(type: ActionType<T>): Observable<Action<T>>`

But it can't be, because the payload type information is not exposed.

because my actions don't have a payload property, and was wondering whether this was a good or bad decision on my part.

It's fine that your actions don't have a "payload" property. There's nothing bad about it, indeed that is exactly the redux model for how the payload should be included (cause redux don't care about dem none-fancy types or nuffin). The Action is considered a black box of key value pairs, and the only thing we know about it is the type key.

But I'm arguing that the library should enforce a typed payload property. This doesn't change anything for you -- you just do a five minute refactor of your code to change the "black box" into a series of getters (probably in an abstract base class somewhere), like in the example I gave you above. Then everywhere else in your code you can ignore the existence of payload entirely.

And by exposing that small amount of type information (and by using typestrings only when interoperating with redux libs), you'll see the whole ngrx API become a whole lot simpler.

Well, apart for the reducers. You will never be able restrict the type by entering a case statement.

@karptonite

This comment has been minimized.

Show comment
Hide comment
@karptonite

karptonite Jul 23, 2017

Contributor

@ovangle

Well, apart for the reducers. You will never be able restrict the type by entering a case statement.

I believe I have a counter example to that here: #132 (comment)

Contributor

karptonite commented Jul 23, 2017

@ovangle

Well, apart for the reducers. You will never be able restrict the type by entering a case statement.

I believe I have a counter example to that here: #132 (comment)

@ovangle

This comment has been minimized.

Show comment
Hide comment
@ovangle

ovangle Jul 23, 2017

Well it seems that the whole strings -> type mapping was just an editor bug. Teaches me not to open my mouth in public, I guess.

Still, it doesn't invalidate my entire argument, it just means that the type inference would be improved in reducers as well, and that it may not be necessary to switch to using types instead of typestrings, although:

  • I don't know if using typestrings would type check against the static interface; and
  • ofType is still a problem; and
  • I think using the actual types results in a cleaner API

ovangle commented Jul 23, 2017

Well it seems that the whole strings -> type mapping was just an editor bug. Teaches me not to open my mouth in public, I guess.

Still, it doesn't invalidate my entire argument, it just means that the type inference would be improved in reducers as well, and that it may not be necessary to switch to using types instead of typestrings, although:

  • I don't know if using typestrings would type check against the static interface; and
  • ofType is still a problem; and
  • I think using the actual types results in a cleaner API
@ovangle

This comment has been minimized.

Show comment
Hide comment
@ovangle

ovangle Jul 23, 2017

Still, this issue is still closed and likely to remain closed now that I've been demonstrably wrong about a small point, so I'll shut the hell up and crawl back into my hole.

I've also updated my comments here to reflect the new information.

ovangle commented Jul 23, 2017

Still, this issue is still closed and likely to remain closed now that I've been demonstrably wrong about a small point, so I'll shut the hell up and crawl back into my hole.

I've also updated my comments here to reflect the new information.

@ovangle

This comment has been minimized.

Show comment
Hide comment
@ovangle

ovangle Jul 24, 2017

So...

When nobody wants to agree with you, you're probably wrong (not necessarily, but probably). So, after updating my editor and upgrading to immutable@4.0.0-rc.2 (which got rid of a lot of the typecasts that were cluttering the code), I refactored to get rid of all of the payloads and payload types.

And... it's good. I'm sorry for wasting your time with all this, although I'd gamble nobody has even bothered to read my essays (because it's painfully obvious that I'm an idiot), so it was only my time that has been wasted. I was so confident that I knew the answers to the old library's problems that I didn't spend the time actually figure out how you'd solved those problems in the new version, because it wasn't the way I suggested you'd solve them. I'm such a child sometime.

There's only one pain point I still have, and that's the multiple declarations of the type of an action:

export const MY_ACTION_TYPE = '[feature area] my action'; // (1)
export class MyAction implements Action {   // (2)
   readonly type = MY_ACTION_TYPE; // (3)
   /* etc. */
}

export type Actions // (4)
    = MyAction;

You have to declare the action four times for it to be useful (the fourth one is a stretch, but it's a necessary re-declaration of the type, so I thought I'd include it. That's horribly redundant. If I was to resubmit the proposal with just the "use type tokens instead of strings" part (getting rid of all the crap about generics), would you be more receptive?

ovangle commented Jul 24, 2017

So...

When nobody wants to agree with you, you're probably wrong (not necessarily, but probably). So, after updating my editor and upgrading to immutable@4.0.0-rc.2 (which got rid of a lot of the typecasts that were cluttering the code), I refactored to get rid of all of the payloads and payload types.

And... it's good. I'm sorry for wasting your time with all this, although I'd gamble nobody has even bothered to read my essays (because it's painfully obvious that I'm an idiot), so it was only my time that has been wasted. I was so confident that I knew the answers to the old library's problems that I didn't spend the time actually figure out how you'd solved those problems in the new version, because it wasn't the way I suggested you'd solve them. I'm such a child sometime.

There's only one pain point I still have, and that's the multiple declarations of the type of an action:

export const MY_ACTION_TYPE = '[feature area] my action'; // (1)
export class MyAction implements Action {   // (2)
   readonly type = MY_ACTION_TYPE; // (3)
   /* etc. */
}

export type Actions // (4)
    = MyAction;

You have to declare the action four times for it to be useful (the fourth one is a stretch, but it's a necessary re-declaration of the type, so I thought I'd include it. That's horribly redundant. If I was to resubmit the proposal with just the "use type tokens instead of strings" part (getting rid of all the crap about generics), would you be more receptive?

@jinder

This comment has been minimized.

Show comment
Hide comment
@jinder

jinder Jul 24, 2017

Contributor

@ovangle I've found using readonly, Readonly<> and ReadonlyArray<> is sufficient in my code to ensure immutability. So you may not even need or want to use immutable.js.

Contributor

jinder commented Jul 24, 2017

@ovangle I've found using readonly, Readonly<> and ReadonlyArray<> is sufficient in my code to ensure immutability. So you may not even need or want to use immutable.js.

@ovangle

This comment has been minimized.

Show comment
Hide comment
@ovangle

ovangle Jul 25, 2017

Thanks for that advice, but I use the immutable.js collection types everywhere, they're far more functional than their javascript counterparts. It's always the first library I install when I start a new project.

There were some problems with the libraries type declarations because they wrote it for a much older typescript, but they've fixed them all in 4.0.0 as far as I can see.

ovangle commented Jul 25, 2017

Thanks for that advice, but I use the immutable.js collection types everywhere, they're far more functional than their javascript counterparts. It's always the first library I install when I start a new project.

There were some problems with the libraries type declarations because they wrote it for a much older typescript, but they've fixed them all in 4.0.0 as far as I can see.

@jbonfante

This comment has been minimized.

Show comment
Hide comment
@jbonfante

jbonfante Dec 14, 2017

@ovangle I Read Them! (your essays) Being a fan of the payload myself, I was even rooting for you at time there, but casting when needed is definitely the way to go. And the pain points, redeem themselves when they save ya... that random chance or three.

jbonfante commented Dec 14, 2017

@ovangle I Read Them! (your essays) Being a fan of the payload myself, I was even rooting for you at time there, but casting when needed is definitely the way to go. And the pain points, redeem themselves when they save ya... that random chance or three.

@murpyslaw

This comment has been minimized.

Show comment
Hide comment
@murpyslaw

murpyslaw Jan 25, 2018

I got around this issue of a non-generic action by creating the following:

import { Action } from '@ngrx/store';

type GenericAction<T> = {
	[K in keyof T]: T[K]
}
export default interface Action<T> extends Action {
	payload: GenericAction<T>
}

It's much safer than an any typed payload, and seems pretty easy to implement. I'm probably missing something obvious though, it couldn't be THAT simple to do, could it?

murpyslaw commented Jan 25, 2018

I got around this issue of a non-generic action by creating the following:

import { Action } from '@ngrx/store';

type GenericAction<T> = {
	[K in keyof T]: T[K]
}
export default interface Action<T> extends Action {
	payload: GenericAction<T>
}

It's much safer than an any typed payload, and seems pretty easy to implement. I'm probably missing something obvious though, it couldn't be THAT simple to do, could it?

@jiqsaw

This comment has been minimized.

Show comment
Hide comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment