feat(Store/Effects): Make toPayload type safe

[karptonite]

This is a followup for PR #157, and should probably replace it.

It makes the toPayload() function type safe, such that it infers the type of the payload, as long as either the type is specified when ofType is called, or the new ofClass method (see #160, assuming it is merged) is used to filter the action.

It still requires the addition of the ActionWithPayload interface, but the user shouldn't have to declare their actions with that interface (although it might be best to do so)--it should be able to be inferred.

[coveralls]

Coverage increased (+0.1%) to 90.086% when pulling f903fc5 on karptonite:patch-make-to-payload-typed into 27cdd36 on ngrx:master.

[bfricka]

Nice job figuring out the toPayload thing! Very useful.

[karptonite]

@bfricka Thanks! I hope it is OK for the project--I wasn't thrilled with having to add another Interface. I'm also kind of surprised that it worked--I thought that action as any would break the type inference, but it seems not to be a problem.

[MikeRyanDev]

I think we would rather just deprecate toPayload in v5 and remove in v6. Is toPayload helpful enough to keep around?

[karptonite]

@MikeRyanDev
toPayload is a pretty trivial utility function. If you were asking if it should be added (and it weren't already in the library), I'd say no.

Because it has been in ngrx for some time, and because the example app encourages its use, I'd say we should be more careful about removing it and forcing all users to adapt. Asking users to pay for a serious mistake by adapting is one thing; this is a trivial utility function that arguably doesn't hurt the library overall.

On the other hand, because it IS a utility function, it is really easy for a user to reimplement it and just change their import statements.

If you don't decide to deprecate and remove this, this PR, or something like it, should be merged; because ofType now allows you to declare a type, it makes sense for toPayload to be type safe as well. If you don't like the addition of the ActionWithPayload interface (I'm not crazy about it) we could just declare the type of action as ({payload:T} & Action) | Action instead, and accomplish the same thing.

In the balance, I could go either way, but lean slightly toward deprecation, with a caveat: Deprecating in 5 and removing in 6, unless you plan to release 5 almost immediately, is unfair to the users who may be moving to ngrx today. I'd say you should deprecate in 4.1 (compatible with semver) and remove in 5, and release 4.1 without delay.

[bfricka]

I agree w/ everything @karptonite just said :)

[jotatoledo]

@MikeRyanDev so you guys are basically aiming to actions without payloads? where everything should be in the app state and retrieved from it to make a state transition?

[karptonite]

@jotatoledo I'm pretty sure that is not what he is going for.

Removing toPayload wouldn't mean that actions have no payloads, just that there is no built in convenience function for extracting a payload field from the action. In fact, removing the presumption that the payload is (only) in a field called payload could make the actions more flexible. You can add whatever additional fields you want to your actions, as long as they implement the Action interface. toPayload was never more than a convenience function to keep from writing action => action.payload. Nothing to stop you from doing that, or importing your own version of the function.

[karptonite]

@MikeRyanDev any further thoughts on this? If you still think that the best option is to deprecate toPayload, are you willing to do so in a minor (4.1) release? If so, I could work up a pull request.

[yngvebn]

@karptonite I'm not quite sure I understand the need for the ActionWithPayload interface. Imho, that's something to be left to the user. Same goes for toPayload. In the least it could probably be a separate helper-project, but not part of core...

[karptonite]

@yngvebn yes, I lean toward deprecating toPayload. But if it won't be deprecated, it might as well be type safe, and, as I wrote a couple comments above, there is no need for the extra interface.

[MikeRyanDev]

@karptonite Let's merge in a deprecation PR instead.

[alex-okrushko]

Just in case someone is missing toPayload you can easily destructure the action:
instead of

map(action => action.payload), // this the same as toPayload(),
switchMap(payload => this.myService.fetch(payload.blah)),

do the following:

switchMap(({payload}) => this.myService.fetch(payload.blah)),