typescript: propose passing MiddlewareAPI state type to Epic

[billba]

Epic is currently declared as:

export declare interface Epic<T> { (action$: ActionsObservable<T>, store: MiddlewareAPI<any>): Observable<T>; }

This means if I want to access the contents of a store's state in a typed way, I need to declare it in the parameters, e.g.:

const myEpic: Epic<MyActions> = (action$, store: MiddlewareAPI<MyState>) => { const state = store.getStore() // now I can access state.foo in a typed way

It would be a little more elegant, and shorter in this case, if the type of the state was passed to the Epic:

export declare interface Epic<T, S> { (action$: ActionsObservable<T>, store: MiddlewareAPI<S>): Observable<T>; }

Then I could call it like:

const myEpic: Epic<MyActions, MyState> = (action$, store) => { const state = store.getStore() // now I can access state.foo in a typed way

The downside, of course, is that folks who don't care about the state type now need to pass any:

const myStatelessEpic: Epic<MyActions, any> = (action$, store) => { // my code doesn't even use the state, why must I pay the price of four extra characters? WHY????

In my limited experience with Epics so far, most of them have used that state, so for me the choice is clear.

[jayphelps]

Interesting. I have rarely seen people need to access the state in their epics. It happens, but it's definitely the minority. Out of curiosity, if you could shoot me some of your examples it would help me (unrelated to this ticket though).

My gut thinks the second generic param is the way it should be indeed. I'll let this ticket sit for a little time so others have a chance to object.

[rgbkrk]

We certainly use the state in some of our epics, though I've been ok with store: MiddlewareAPI<State> when an epic requires the store.

[kwonoj]

We are also accessing store / state quite often in epic. This is obviously interesting, but given aspect most of users probably wouldn't care about specifying strict type of state it'll be possibly some kind of roadblocks. I always desperately wanted to have default generic type to solve this easily which isn't available at this moment (and even no rough ETA).

[billba]

Agreed that a default generic type (even just defaulting to any) would be the ideal solution. But I don't think that forcing epic users to add any as a second generic type is too bad, and it would be a nice hint that they can optionally access the store/state in a typed way (it took me a while to figure out that I could specify the store type when listing parameters).

[jayphelps]

@billba unless I'm misunderstanding, seems like we have general consensus that this change would be okay--sounds like most are indifferent? I'm inclined to think this your suggested interface is the way to go. Wanna put together the PR?

[billba]

Will do, thanks.

[kwonoj]

@jayphelps @billba , one thing - will this not be breaking changes to typescript users? if they've used Epic<T> I assume they'll encounter compilation error after installing package with updates type Epic<T, S>?

[jayphelps]

@kwonoj yep, this would be a breaking change. I don't want to introduce breaking changes all willy-nilly, but we're pre-1.0. I follow the common pre-1.0 versioning of bumping the minor number to mean possible breaking changes, 0.BREAKING.PATCH. Lmk if there are concerns. Honestly, things are pretty simple and stable, so I'm considering just going 1.0.0 here soon. I've mostly been waiting because of RxJS v5 (which is now obv released)

[kwonoj]

@jayphelps I don't mind it as long as if it's noted down, just checking. It is interesting debate about how to deal with type definition with semver after public release though. (same for RxJS)

[billba]

Good news: TypeScript 2.3 will support generic defaults: https://github.com/Microsoft/TypeScript/wiki/Roadmap#23-may-2017