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

feat(typings): added dependencies generic type #250

Merged
merged 2 commits into from Aug 8, 2017

Conversation

Projects
None yet
3 participants
@rafaelkallis
Contributor

rafaelkallis commented May 23, 2017

This PR is a draft and should be considered as an extension to the discussion started in #231.
Feedback is mandatory ;)

Added a dependency generic type to typings.
The type defaults to any for backwards compatibility.

@jayphelps

This comment has been minimized.

Member

jayphelps commented May 23, 2017

Edit: ignore this comment


Hmm...interesting that this passes our tests--it shouldn't cause this didn't upgrade typescript to 2.3 which is when they added support for default generic params https://github.com/Microsoft/TypeScript/wiki/What's-new-in-TypeScript#generic-parameter-defaults

Perhaps the automated tests aren't running the typescript tests or I'm otherwise confused?

@jayphelps

This comment has been minimized.

Member

jayphelps commented May 23, 2017

OH our semver range for it is ^2.1.4 which will match 2.3.0 (basically 2.x.x). Whoops, we should probably restrict this to patch only, so we intentionally upgrade since typescript doesn't follow semver strictly.

@rafaelkallis

This comment has been minimized.

Contributor

rafaelkallis commented May 24, 2017

When would you consider upgrading redux-observable to typescript >= 2.3? Besides that, is there anything else from your side @jayphelps ?

@jayphelps

This comment has been minimized.

Member

jayphelps commented May 24, 2017

@rafaelkallis I'm okay upgrading to 2.3 now, since Angular 4 did in April. I think we should open a ticket to see if there are any objections for at least a week or so--I actually just did that: #251

Regarding this PR, I still need to find some time to truly dive into and test it.

@rafaelkallis

This comment has been minimized.

Contributor

rafaelkallis commented May 24, 2017

Allow me present a summary of all options we have right now. The snippets refer to these typings, and I work under the assumption that all changes apply to types having <T, S> as generic parameters.


1: PR gets rejected, no changes.

/* current implementation */
interface Epic<T, S> {
  (action$: ActionsObservable<T>, store: MiddlewareAPI<S>): Observable<T>;
}

2: New dependencies generic type without default type. (Works with current typescript version)

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

This would force users to define a dependencies type and may cause redux-observable to look quite verbose.


3: No new dependencies generic type but dependencies get any type. (Works with current typescript version)

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

4: New dependencies type defaulting to any. (Needs typescript 2.3) The current PR implements this option

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

5: New dependencies type defaulting to void or undefined or {}. (Needs typescript 2.3)

interface Epic<T, S, D = {}> { /* one of: void, undefined, {} */
  (action$: ActionsObservable<T>, store: MiddlewareAPI<S>, dependencies: D): Observable<T>;
}
@jayphelps

This comment has been minimized.

Member

jayphelps commented May 24, 2017

Would we want dependencies to be optional?

interface Epic<T, S, D = any> {
  (action$: ActionsObservable<T>, store: MiddlewareAPI<S>, dependencies?: D): Observable<T>;
}
@rafaelkallis

This comment has been minimized.

Contributor

rafaelkallis commented May 25, 2017

@jayphelps thanks for the question.

Generally my thought is: If store isn't optional, why should dependencies be?

While having dependencies as a optional argument, there are only 2 ways I'm aware of to use dependencies, without causing compilation or linting errors, such as: [ts] Object is possibly 'undefined'.

// optional dependencies
interface Epic<T, S, D = any> {
  (action$: ActionsObservable<T>, store: MiddlewareAPI<S>, dependencies?: D): Observable<T>;
}

// our dependencies type
interface Delta {
  api: { call(): void };
}

// compilation error, dependencies has Type 'Delta | undefined'
const errorEpic: Epic<Tau, Sigma, Delta> = (action$, store, dependencies) => action$.ofType( ... ).do(() => { dependencies.api.call(); } ).map(() => ... );

// 1: works, explicitly declaring dependencies as Delta inside arguments
const worksEpic1: Epic<Tau, Sigma, Delta> = (action$, store, dependencies: Delta) => action$.ofType( ... ).do(() => { dependencies.api.call(); } ).map(() => ... )

// 2: also works, casting dependencies
const worksEpic2: Epic<Tau, Sigma, Delta> = (action$, store, dependencies) => action$.ofType( ... ).do(() => { (dependencies as Delta).api.call(); } ).map(() => ... )

Code would be better readable when having the dependencies argument as non-optional, like the following snippet can hopefully demonstrate:

// non-optional dependencies
interface Epic<T, S, D = any> {
  (action$: ActionsObservable<T>, store: MiddlewareAPI<S>, dependencies: D): Observable<T>;
}

// our dependencies type
interface Delta {
  api: { call(): void };
}

// no compilation errors
const fooEpic: Epic<Tau, Sigma, Delta> = (action$, store, dependencies) => action$.ofType( ... ).do(() => { dependencies.api.call(); } ).map(() => ... );

// no compilation errors
const barEpic: Epic<Tau, Sigma, Delta> = (action$, store) => action$.ofType( ... ).map(() => ... );

Please excuse my verbose answer, but I believe it's better to have all thoughts and examples written down so everybody can follow.

@jayphelps

This comment has been minimized.

Member

jayphelps commented May 25, 2017

// no compilation errors
const fooEpic: Epic<Tau, Sigma, Delta> = (action$, store) => action$.ofType( ... ).map(() => ... );

Oh, I incorrectly assumed this would be an error, since the signature is in fact not the same. I'm not sure it matters, in that case. The reason I wanted it optional was because not everyone will in fact use it.

FWIW I checked flow and it doesn't treat this as an error either. I do worry TS will eventually change their stance on this but we can't be held hostage by what ifs so I'm down.

@rafaelkallis

This comment has been minimized.

Contributor

rafaelkallis commented May 25, 2017

Citation from typescript's official documentation:

It’s always legal to provide a callback that accepts fewer arguments.

It is currently considered a best practise to write callback parameters as non-optional, even if their usage is optional (EDIT: assuming the parameter is always defined). Having parameters which are always defined as optional is considered a bad practise. Hopefully this will make you worry less @jayphelps 👍.

Reference: Typescript documentation, Do's and Dont's, under section callback types, optional parameters in callbacks.

However, there is one complication. The current implementation allows a possibility of having dependencies as undefined, see here and here. In that case, having dependencies as a non-optional parameter would be considered a bad-practise according to the typescript community.

EDIT: dependencies would always be defined if:

// current implementation, dependencies is per default undefined
const defaultOptions = {
  adapter: defaultAdapter
};

// suggested implementation if dependencies should be non-optional, dependencies always defined
const defaultOptions = {
  adapter: defaultAdapter,
  dependencies: {}
};

And in this case, using dependencies as a non-optional parameter would be quite safe.

@jayphelps

This comment has been minimized.

Member

jayphelps commented May 25, 2017

I can't recall why I did dependencies: {}. IMO it should be undefined if not provided. 🤔

@rafaelkallis

This comment has been minimized.

Contributor

rafaelkallis commented May 26, 2017

Small misunderstanding, the current implementation has dependencies undefined per default. The example I gave with dependencies: {}, should be used in case we decide for the non-optional parameter.

I have updated my comment above to reflect the change.

@mrceperka

This comment has been minimized.

mrceperka commented Jun 6, 2017

Is there any way, how to extend already declared typings in redux-observables?
I really need this one:

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

So I've tried this:

// my custom redux-observable.d.ts
import { Observable } from 'rxjs';
import { Epic, ActionsObservable } from 'redux-observable';

declare module 'redux-obsevable' {
  export interface Epic<T, S, D> {
    (action$: ActionsObservable<T>, store: MiddlewareAPI<S>, deps: D): Observable<T>;
  }
}

But it seems, that I can not merge declarations this way.
Because when I try to use it like this:

const epic: Epic<any, any, any> = /* code */;

I got an error Generic type 'Epic<T, S>' requires 2 type argument

Are you planning to add missing definitions soon? Or how can I patch it locally?

Thank you.

@mrceperka

This comment has been minimized.

mrceperka commented Jun 6, 2017

Figured out something.
I've used this shamefull recast to fix warnings and maintain some type safety.
Here is an example.

import { TRootState } from '../types/reducers/root';
import { TAuthAction } from '../types/actions/auth';
import { Dependencies } from '../types/epics';
import { combineEpics, ActionsObservable, Epic } from 'redux-observable';
import { Observable } from 'rxjs';
import { Store } from 'redux';

import { login, loginSuccess, loginFail } from '../actions/auth';
import { TLoginAction, TLoginActionPayload } from '../types/actions/auth';

const authEpic = (
  action$: ActionsObservable<TAuthAction>,
  store: Store<TRootState>,
  { axios }: Dependencies
) =>
  action$
    .ofType('LOGIN')
    .mergeMap<TLoginAction, any>(({ payload: { email, password } }) =>
      Observable.from(axios.post('/login', JSON.stringify({ email, password })))
        .mergeMap(response => Observable.of(loginSuccess(response.data)))
        .catch(() => Observable.of(loginFail()))
    );

const authEpicCasted = (authEpic as any) as Epic<TAuthAction, TRootState>;
export default combineEpics(authEpicCasted);
@rafaelkallis

This comment has been minimized.

Contributor

rafaelkallis commented Jun 6, 2017

Hey @mrceperka, this is how I'm doing it for the moment:

type EpicFactory<T, S, D> = (dependencies: D) => Epic<T, S>; // <- the magic part

type DependenciesT = { restApi: RestApi; storageApi: StorageApi };

// this epic needs dependencies
const searchPeopleEpic: EpicFactory<ActionT, StateT, DependenciesT> = ({ restApi }) => 
  (action$, { getState }) => action$
    .ofType("SEARCH_PEOPLE_STARTED")
    .switchMap(({ payload: { name } }) =>
      Observable.fromPromise(restApi.searchPeople(name))
        .map(people => searchPeopleFulfilled(people)),
    );

// this epic does not need dependencies
const epicWithoutDependencies: Epic<ActionT, StateT> = 
  (action$, { getState }) => action$
    .ofType("PING")
    .map(() => pong());

export const rootEpic: EpicFactory<ActionT, StateT, DependenciesT> = dependencies =>
  combineEpics(
    searchPeopleEpic(dependencies), // <- injection
    epicWithoutDependencies, // <- no injection
    ...
  );

const epicMiddleware = createEpicMiddleware(
  rootEpic({ storageApi, restApi }), // <- injecting dependencies here
);
@jayphelps

This comment has been minimized.

Member

jayphelps commented Jun 6, 2017

@rafaelkallis Could you add additional test cases in test/typings.ts? I think after that, it will be ready for final review.

@rafaelkallis

This comment has been minimized.

Contributor

rafaelkallis commented Jun 6, 2017

@jayphelps added a basic test case, let me know if you wish to see more/better tests.

@rafaelkallis rafaelkallis changed the title from [WIP] feat(typings): added dependencies generic type, Closes #231 to feat(typings): added dependencies generic type, Closes #231 Jun 6, 2017

@jayphelps jayphelps referenced this pull request Jun 9, 2017

Open

Flow types #258

@rafaelkallis

This comment has been minimized.

Contributor

rafaelkallis commented Jun 19, 2017

@jayphelps is there anything else keeping this from final review?

@rafaelkallis rafaelkallis changed the title from feat(typings): added dependencies generic type, Closes #231 to feat(typings): added dependencies generic type Jul 12, 2017

@rafaelkallis

This comment has been minimized.

Contributor

rafaelkallis commented Aug 8, 2017

ping @jayphelps! the PR is ready to be merged!

@jayphelps

This comment has been minimized.

Member

jayphelps commented Aug 8, 2017

@rafaelkallis Thanks for pinging me 👍 sorry for the delays

@jayphelps jayphelps merged commit b690902 into redux-observable:master Aug 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rafaelkallis rafaelkallis deleted the rafaelkallis:dependencies-type branch Aug 8, 2017

@jayphelps

This comment has been minimized.

Member

jayphelps commented Aug 8, 2017

released as part of 0.15.0

@jayphelps jayphelps referenced this pull request Aug 13, 2017

Merged

Fix typing of `ofType` #289

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