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 Epic interface is missing dependencies parameter #231

Closed
jchapuis opened this Issue Mar 31, 2017 · 12 comments

Comments

Projects
None yet
5 participants
@jchapuis
Contributor

jchapuis commented Mar 31, 2017

Current declaration for the Epic<T, S> type is missing the last dependencies parameter

Instead of:
export declare interface Epic<T, S> { (action$: ActionsObservable<T>, store: MiddlewareAPI<S>): Observable<T>; }
it should probably be
export declare interface Epic<T, S> { (action$: ActionsObservable<T>, store: MiddlewareAPI<S>, dependencies: any): Observable<T>; }

@jayphelps

This comment has been minimized.

Member

jayphelps commented Mar 31, 2017

I've been trying to think of the best way to handle this before TS gets generic param defaults (coming in the next version).

I'd rather not do dependencies?: any long term for a couple reasons. I'll try to find some time to gather them here and the alternatives I've tried. I could prolly be convinced to ship it temporarily... dependencies?: any (optional is important)

The real dependencies type needs to be inferred from what you pass to createEpicMiddleware as well and some how passed along correctly to the options, again causing issues without default generic params cause you can't have generic param overloads

We also have to juggle this with the requests of #219 which is asking for a separate type for input vs output.

@jchapuis

This comment has been minimized.

Contributor

jchapuis commented Apr 3, 2017

Thanks! yes I can definitely see the issue. For now thanks to the combineEpics<E>(...epics: E[]): E; overload I can actually have epics typed with my dependency, which is the important part, so from my perspective there no urgency in propagating the type all the way.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Apr 3, 2017

Great to know. Thanks for reporting back.

@rehia

This comment has been minimized.

rehia commented Apr 4, 2017

I'm facing the same issue.
For the moment, I applied a workaround with a higher order function to build the epics, inject the dependencies, and use them in the epics, as they are scoped within this higher order function. They are not part of the epics signature anymore, but at least the build does not scream anymore 😁
That's only a temporary workaround, and it would be preferable to use the library feature directly.

@veeramarni

This comment has been minimized.

veeramarni commented Apr 21, 2017

@rehia can you share the workaround you tried?

@rehia

This comment has been minimized.

rehia commented Apr 27, 2017

@veeramarni sorry didn't see your post.
That's quite simple actually.
It's pure dependency injection.

Here I declare the epics, in which I use my dependencies:

const prepareEpics = (dependencies) => ({
  epic1: ...,
  epic2: ...
});

And I export a function to combine them:

export const buildEpic = (dependencies) =>
  combineEpics(
    ...values(prepareEpics(dependencies))
  );

(values() comes from lodash)

Then I use it as a middleware:

const dependencies = {dependency1, dependency2};

applyMiddleware(
  createEpicMiddleware(
    buildEpic(dependencies)
  )
)

Hope this helps.

@rafaelkallis

This comment has been minimized.

Contributor

rafaelkallis commented May 22, 2017

Wouldn't something like my example below be totally fine, or do I miss something?

/**
 * D represents dependencies type
 * for example:
 *
 * interface IDependencies {
 *   fooApi: FooApi;
 *   barApi: BarApi;
 * }
 */

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

export interface EpicMiddleware<T, S, D> extends Middleware {
  replaceEpic(nextEpic: Epic<T, S, D>): void;
}

interface Options<D> {
  adapter?: Adapter;
  dependencies?: D;
}

export declare function createEpicMiddleware<T, S, D>(rootEpic: Epic<T, S, D>, options?: Options<D>): EpicMiddleware<T, S, D>;

export function combineEpics<T, S, D>(...epics: Epic<T, S, D>[]): Epic<T, S, D>;

Many thanks for the great library!

@jayphelps

This comment has been minimized.

Member

jayphelps commented May 22, 2017

@rafaelkallis That would force people to have to pass a type for dependencies and include it in their epic function signatures, even if they don't use it..which isn't ideal but certainly not the end of the world.

Default type param would solve this once enough people have switched to that TS version. There is also #219 which conflicts.

@rafaelkallis

This comment has been minimized.

Contributor

rafaelkallis commented May 22, 2017

Thanks @jayphelps for the very good argument, I also support that.

May I also ask what the use case of this definition of combineEpics is?

export declare function combineEpics<E>(...epics: E[]): E;

It perfectly solves the dependency type issue if used like follows:

export type CustomEpic = (
    action$: ActionsObservable<any>,
    store: MiddlewareAPI<IRootState>,
    dependencies: IDependencies,
) => Observable<any>;

const rootEpic = combineEpics<CustomEpic>(
  (action$, store, dependencies) => ... ,
  ...
);

Yet I'm struggling finding a way to use rootEpic with createEpicMiddleware

@jayphelps

This comment has been minimized.

Member

jayphelps commented May 22, 2017

export declare function combineEpics<E>(...epics: E[]): E; is so people can provide their own Epic signature, if they don't use the dependencies feature and instead inject dependencies via something like:

const rootEpic: CustomEpic = (action$, store) => combineEpics<CustomEpic>(epic1, epic2)(action$, store, custom1, custom2);

I unfortunately don't have much time right now to dive deeper into this, but if you can get something working and it's ergonomic, definitely feel free to PR! I would love you long time.

@rafaelkallis

This comment has been minimized.

Contributor

rafaelkallis commented May 22, 2017

@jayphelps thanks for the interesting snippet provided.

I was trying to hint towards something like this with my question:

export declare function createEpicMiddleware<E>(rootEpic: E, options?: Options): EpicMiddleware<E>;

such that it can be used like so:

createEpicMiddleware<CustomEpic>(combineEpics<CustomEpic>(epic1, epic2), {dependencies: {fooApi, barApi}});

But I guess it boils down to those default type params again.

Nevertheless, thanks again for your response. If I find some spare time and hack something useful I may open a PR.

@rafaelkallis

This comment has been minimized.

Contributor

rafaelkallis commented May 26, 2017

@jchapuis @rehia have a look at the current PR and let me know if it accommodates your needs. Feedback is welcome!

@jayphelps jayphelps closed this in b690902 Aug 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment