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

fix(typings): Add combineEpics overload for custom Epic signatures #197

Merged

Conversation

@terrierscript
Copy link
Contributor

@terrierscript terrierscript commented Feb 16, 2017

I try to use API injection epic pattern that like #84 (comment) and #163.

But current typing is not accept combineEpics with extra argument epics.

@jayphelps
Copy link
Member

@jayphelps jayphelps commented Feb 16, 2017

hmmm this would still lose type safety on those extra args since they're all any.

What about instead creating an overload for combineEpics which allows you pass in your own implementation of the Epic interface (or it inferred), so you can have complete safety.

declare function combineEpics<T, S>(...epics: Epic<T, S>[]): Epic<T, S>;
// new overload
declare function combineEpics<T>(...epics: T[]): T;
interface CustomEpic {
  (action$: ActionsObservable<Action>, store: MiddlewareAPI<State>, api: SomeApi): Observable<Action>;
}

const epic1: CustomEpic = (action$, store, api: SomeApi) =>
  action$.ofType('STUFF').etc();

const rootEpic: Epic<Action, State> = (action$, store) =>
  combineEpics<CustomEpic>(epic1)(action$, store, api);

@terrierscript terrierscript force-pushed the epic-rest-param-type branch from c039c95 to 9cfdd05 Feb 16, 2017
@terrierscript
Copy link
Contributor Author

@terrierscript terrierscript commented Feb 16, 2017

@jayphelps
Thank you for good advice. I change to this.

@terrierscript terrierscript force-pushed the epic-rest-param-type branch from 9cfdd05 to 74cf8d4 Feb 16, 2017
@jayphelps
Copy link
Member

@jayphelps jayphelps commented Feb 16, 2017

Seems good to me. Wonder if @kwonoj could spare a second to confirm this solution is kosher?

@jayphelps jayphelps changed the title fix(typings): Add extra epic params pattern fix(typings): Add combineEpics overload for custom Epic signatures Feb 16, 2017
index.d.ts Outdated
@@ -45,5 +45,7 @@ interface Options {
}

export declare function createEpicMiddleware<T, S>(rootEpic: Epic<T, S>, options?: Options): EpicMiddleware<T, S>;
export declare function createEpicMiddleware<E, T, S>(rootEpic: E, options?: Options): EpicMiddleware<T, S>;
Copy link
Member

@jayphelps jayphelps Feb 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this overload necessary? Shouldn't the rootEpic always be (action$, store) only?

Copy link
Contributor Author

@terrierscript terrierscript Feb 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I didn't know this.
https://github.com/redux-observable/redux-observable/blob/master/src/createEpicMiddleware.js#L34

It's not good overload. I removed it.

@terrierscript terrierscript force-pushed the epic-rest-param-type branch from 74cf8d4 to 566b1c9 Feb 17, 2017
@kwonoj
Copy link
Contributor

@kwonoj kwonoj commented Feb 19, 2017

This seems bits complicated to me, so this is based on assumption custom epics are following <E> always, is that correct? Though it's general problem of generic that can't carry multiple types easily, if there's non-formed type of epic than it won't be accepted. Like, even default epic definition falls into those category like

const defaultEpic = (a, s) => {};
const customEpic: CustomEpic<U> = (...) => {};

combineEpics<CustomEpic<U>>(defaultEpic, customEpic); //can't apply both epics?

@jayphelps
Copy link
Member

@jayphelps jayphelps commented Feb 19, 2017

@kwonoj hmm I'm not 100% sure what you mean. This new overload is so that people can provide a custom signature for epics--usually because they're doing dependency injection. All of their epics have to have the same signature.

@kwonoj
Copy link
Contributor

@kwonoj kwonoj commented Feb 19, 2017

Yes, my initial assumption was someone would expect mixed types between different types of Epics. If this overload is intended to be used for constraint to epic have all same custom interfaces, I think it'll be fine.

@jayphelps
Copy link
Member

@jayphelps jayphelps commented Feb 19, 2017

@kwonoj Someone could presumably provide a union of multiple signatures as well AFAICT

@jayphelps jayphelps merged commit 88c0bf9 into redux-observable:master Feb 20, 2017
1 check passed
@jayphelps
Copy link
Member

@jayphelps jayphelps commented Feb 20, 2017

Thanks @inuscript ! 🎉

@terrierscript terrierscript deleted the epic-rest-param-type branch Feb 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants