Platform: Deprecate or fix toPayload()?

[karptonite]

This is related to the discussion of the Action interface, here.
While there may be type safety issues with having action on the interface, not having payload leads to the odd implementation of toPayload, which both requires casting of action to any and breaks type safety on the return value:

export function toPayload(action: Action): any {
  return (action as any).payload;
}

If we want ngrx to be agnostic about how payloads are handled, and to help with type safety, the toPayload function seems to work against that. perhaps toPayload() should be deprecated? It will usually be preferable to use something like:

.map( action: SpecificActionWithPayload => action.payload )

Alternatively, it might be possible to create an interface for ngrx

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

Then implement toPayload as follows:

export function toPayload(action: ActionWithPayload<T>): T {
  return action.payload;
}

That should allow for type safety, assuming that the type of the action itself is previously known, maybe with something like:

.map( action => action as SpecificActionWithPayload )

clunky, but until typescript can infer the action type via ofType, maybe an OK option?

[ovangle]

until typescript can infer the action type via ofType

Typescript will never be able to infer the action type via ofType while ofType accepts a string argument. (Type information is erased by the signature of ofType --) that was one of the benefits of the changes I proposed, because once you're using the actual types (instead of strings) as tokens, it becomes possible to begin making inferences _like this one_.

It's impossible for the compiler to associate the value of a string at an arbitrary point in code (the callsite of ofType) with a declared constant on a type definition elsewhere (a "constant" which is actually a property of the type and could conceivably be mutated almost anywhere for that matter). (readonly actually provides a usable mapping between the string and the type)

Unfortunately, I had forgotten about that consideration sometime between when I made my comment on the original issue and when I wrote it all up yesterday. Thanks for reminding me.

EDIT: I was a bit of an arse in this comment, I'm sorry.

[karptonite]

@ovangle I'm not sure you are correct about that.

TypeScript CAN infer the type in a reducer, when using a switch statement to filter on the action type, which, as you say, is only a string. For example, for an action like this:

export class LoadBegunAction implements Action {
    readonly type = LOAD_BEGUN;
    constructor( public payload: { item: ApiId } ) {}
}

Note the readonly and the use of a const for the type. TypeScript infers the type of the action in this switch statement:

switch ( action.type ) {
    case essentialItemActions.LOAD_BEGUN:
        // Do stuff with action.payload, where the compiler has inferred the type of action

Obviously, ofType is a different situation, but it doesn't seem so different to me that I couldn't imagine the TypeScript devs figuring out a way to infer type in a function like ofType at some point.

[ovangle]

There is no type inference by the typescript compiler going on there, it's just javascript inspections performed by your editor of choice.

Consider the following:

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

interface PayloadAction<T> extends Action {
  readonly payload: T;
}

const LOAD_BEGUN = 'load begun';
const LOAD_END = 'load end';

class LoadBegunAction implements PayloadAction<{item1: string}> {
  readonly type = LOAD_BEGUN;
  constructor(public payload: {item1: string}) {}
}

class LoadEndAction implements PayloadAction<{item2: string}> {
  readonly type = LOAD_END;
  constructor(public payload: {item2: string}) {}
}

function reducer(state: any, action: PayloadAction<any>) {
  switch (action.type) {
    case LOAD_BEGUN:
      // This is fine
      return action.payload.item1;
  }
}

[ovangle]

You're looking to get something for nothing. The typescript compiler isn't magic, it's a library just like this one. It has no idea what you _intend_ to mean when you use the typestring in a case statement.

If you want stuff like this to be possible, you actually have to _expose type information that the compiler can use_. And adding a PayloadAction<T> interface in user code doesn't help when the library is just going to cast your generic to any the first chance it gets (e.g.ofType). You're using strings as proxies for the types and you've provided no way for the compiler to make an intuitive leap from the proxy to the type.

Typescript will be able to do what you want it to do if you _give it more/better information_. See my (prematurely closed) issue for an example of how to accomplish this.

Note
Please at least read the comment I made on the closed issue today, which gives some further motivation and background for the changes. This will be the last comment I make on this repo if the issue isn't reopened, but it would be a shame if I went to all that effort for nobody to ever read it. As for myself, I'll just fork ngrx and get on with my life, knowing that I did what I could.

[karptonite]

@ovangle

There is no type inference by the typescript compiler going on there, it's just javascript inspections performed by your editor of choice.

I believe you are mistaken. Your example didn't work work for two reasons: First, you didn't sufficiently restrict the type of your reducer. Second, you used PHPStorm, which does not appear to use the typescript compiler and other tools for type inference. I suggest you try VS Code if you want to see the types that TypeScript actually infers.

Consider the following example:

class Foo1 {
    readonly type = 'foo1';
    public bar = 'bar';
}

class Foo2 {
    readonly type = 'foo2';
}

type Foos = Foo1 | Foo2;

function processFoo ( foo: Foos) {
    switch (foo.type) {
        case 'foo1':
            console.log(foo.bar);
    }
}

This compiles fine. But if you change the readonly's to public, it doesn't compile at all, and errors with Property 'bar' does not exist on type 'Foos'. Property 'bar' does not exist on type 'Foo2'.. If this were just an issue of javascript inspections, the compiler would not fail to compile in the second case.

if the incoming type is sufficiently restricted, toPayload can use the compiler to infer the type of payload. I have a working example here: #161

[bfricka]

@karptonite, Quick niggle: JetBrains stuff _does_ use TypeScript service, but in older versions (or projects created in older versions), you had to specifically enable the TypeScript service (Settings -> TypeScript -> Use TypeScript Service)

[karptonite]

@bfricka Maybe the use Typescript service for some stuff, but they don't seem to use it for their type inferences, at least not exclusively. If they did, they wouldn't be having bugs like this one: https://youtrack.jetbrains.com/issue/WEB-27891. I have the correct setting to use Typescript services set.

[bfricka]

That's correct. They don't use the VS Code language protocol (there is an open issue for that), and they do a lot of their own processing. I wish they'd just go to use the language protocol :(

[ovangle]

I stand corrected on that point. I didn't think such an inference would be reliable enough to add to the compiler, but maybe I'm just unclear on the semantics of readonly (I don't change editors every day lol)

I've fixed my comments here to reflect my newfound wisdom.

[brandonroberts]

Closed via #266