Skip to content

Allowing action.type to be Type<T> #168

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

Closed
ovangle opened this issue Jul 24, 2017 · 4 comments
Closed

Allowing action.type to be Type<T> #168

ovangle opened this issue Jul 24, 2017 · 4 comments

Comments

@ovangle
Copy link

ovangle commented Jul 24, 2017

There's one issue that I raised that has I think has merit as a more limited proposal, and that is to use the declared type of an action, rather than insisting the user provide a string proxy.

Less code, more readable

In order to define an action, the type needs to be declared at least three different times.

const MY_ACTION_STRING = 'my action string'; // 1
class MyAction implements Action { // 2
   readonly type = MY_ACTION_STRING; // 3
} 

If Type<T> was used instead, the definition would become

class MyAction implements Action {
  readonly type = MyAction;
}

Readability improvements are not limited to the action definition either, reducers have a slight improvement:

function reducer(state, action) {
  switch (action.type) {
     case MyAction:
       // Use MyAction directly, rather than MY_ACTION. 
       // Since a class defn is implicitly const and the `type` readonly, 
       // you still get the same inference you had with string types
     default:
         return state;
   }
}

In general, any place that the string proxy MY_ACTION is used, it could be replaced with the more direct MyAction.

The actions aren't serialisable

Two problems exist which prevent these actions from ever being serializable:

  • They requires eval of untrusted strings
  • The type of the deserialised function is non-canonical (ie. MyAction !== deserialize(MyAction.toString())), so it can't be matched in reducers

The notion of using a unserialisable key has been discussed elsewhere (#143), but it was noted that a clear motivation would be required. However, while I feel that the uniqueness of Symbols does not provide a clear enough motivation, Type<T> is similarly unique, already available as a token
as part of the class declaration, and provides further benefits to type safety and readability.

Any feature which depends on the serialisability of actions could confine itself to using type strings exclusively.

The signature of Actions.ofType can be fixed

Improving the type safety of effects today, rather than waiting for the typescript team to support binding the type via the typestring. (which I'm still surprised to know is possible in switch statements).

class Actions<V = Action> extends Observable<V> {
  ofType<V2 extends V = V>(...type: (string | Type<V2>)[]): Observable<V2>;
}

I'm still doubtful that binding the type by typestring at the callsite of ofType will ever be possible though. A switch statement knows that you're switching on action.type (and can bind appropriately), whereas there is no way for typescript to know that the internals of the function ofType even refer to action.type, so even though the string at the callsite matches the readonly action.type, it cannot make an inference of the type (a function is a lot more complex than a switch). But I was wrong about switch statements, so I might be wrong here too. (this is all nonsense).

Actions must be passed to devtools

If devtools tried to log action.type, the output might be MyAction.toString(), rather than the preferable MyAction.name. I don't know enough about devtools to know whether it is possible to ask devtools to use the better string.

Update: Removed needless self-criticism from the proposal.

@ovangle ovangle changed the title Allowing Action.type to be Type<T> Allowing action.type to be Type<T> Jul 25, 2017
@ovangle
Copy link
Author

ovangle commented Jul 25, 2017

Apparently you don't get the same inferences in switch statements at all. I've filed a bug with typescript about it.

@ovangle
Copy link
Author

ovangle commented Jul 26, 2017

So yeah, this won't work, of course. Stupid me, sorry.

@ovangle ovangle closed this as completed Jul 26, 2017
@ovangle ovangle reopened this Jul 26, 2017
@MikeRyanDev
Copy link
Member

MikeRyanDev commented Aug 9, 2017

Going to close this. We are committed to keeping actions serializable.

Thanks!

@ovangle
Copy link
Author

ovangle commented Aug 9, 2017

👍

Thank you for this library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants