Skip to content
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

typescript: propose passing MiddlewareAPI state type to Epic #172

Closed
billba opened this issue Jan 12, 2017 · 10 comments

Comments

@billba
Copy link
Contributor

commented Jan 12, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 12, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 12, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2017

Will do, thanks.

@kwonoj

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2017

@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

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2017

@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 pushed a commit to billba/redux-observable that referenced this issue Jan 19, 2017

@billba billba referenced this issue Jan 20, 2017

Merged

fix(typescript): adds store type to Epic #174

2 of 2 tasks complete

jayphelps added a commit that referenced this issue Jan 20, 2017

fix(typings): adds store type to Epic (#174)
fixes #172

BREAKING CHANGE: TypeScript users only, the type interface for Epics now requires a second generic argument, your store's state interface. `interface Epic<ActionShape, StateShape>`. If you don't to strictly type your state, you can pass `any`
@billba

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.