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 examples #160

Closed
ashishsc opened this issue Oct 10, 2017 · 35 comments
Closed

Typescript examples #160

ashishsc opened this issue Oct 10, 2017 · 35 comments
Labels

Comments

@ashishsc
Copy link

@ashishsc ashishsc commented Oct 10, 2017

I think we should try writing the example in typescript. There's some friction you run into when defining action creators unless you define it the way redux-loop is expecting it (using the ActionCreator type, etc.) I'll try and post a concrete issue of what I'm facing here in a bit. @willisplummer since you added the definitions, do you have any sample code using redux-loop in a ts project?

@willisplummer

This comment has been minimized.

Copy link
Contributor

@willisplummer willisplummer commented Oct 10, 2017

Yeah definitely. I'll open a PR tomorrow or later this week. Interested to see what you're running into though!

@bdwain

This comment has been minimized.

Copy link
Member

@bdwain bdwain commented Oct 10, 2017

Agreed. This is also related to #158. I think that's probably the next big task ahead of us. It would really lower the barrier to entry for people.

@ashishsc

This comment has been minimized.

Copy link
Author

@ashishsc ashishsc commented Oct 10, 2017

Ok so, first a question about the type for Cmd.run:

  static readonly run: <A extends Action>(
    f: Function,
    options?: {
      args?: any[];
      failActionCreator?: ActionCreator<A>;
      successActionCreator?: ActionCreator<A>;
      forceSync?: boolean;
    }
  ) => RunCmd<A>;

I'm getting type errors here because my fail and success action don't create the same action. Shouldn't the actual type be

  static readonly run: <A extends Action, F extends Action>(
    f: Function,
    options?: {
      args?: any[];
      failActionCreator?: ActionCreator<F>;
      successActionCreator?: ActionCreator<A>;
      forceSync?: boolean;
    }
  ) => RunCmd<A>;

I could be wrong because this seems like a cascading problem as we would need to change all of

export type CmdType<A extends Action> =
  | ActionCmd<A>
  | ListCmd<A>
  | MapCmd<A>
  | NoneCmd
  | RunCmd<A>
  | BatchCmd<A>
  | SequenceCmd<A>;

to

export type CmdType<A extends Action, F extends Action> =
  | ActionCmd<A,F>
  | ListCmd<A,F>
  | MapCmd<A,F>
  | NoneCmd
  | RunCmd<A,F>
  | BatchCmd<A,F>
  | SequenceCmd<A,F>;
@ashishsc

This comment has been minimized.

Copy link
Author

@ashishsc ashishsc commented Oct 10, 2017

Another issue:

export type ReducerMapObject<S, A extends Action = AnyAction> = {
    [K in keyof S]: LoopReducer<S[K], A>
};

declare function combineReducers<S, A extends Action = AnyAction>(
    reducers: ReducerMapObject<S, A>,
): LiftedLoopReducer<S, A>;

declare function mergeChildReducers<S, A extends Action = AnyAction>(
    parentResult: S | Loop<S, A>,
    action: AnyAction,
    childMap: ReducerMapObject<S, A>,
): Loop<S, A>;

including redux-loop in my project gives me errors:

ERROR at [repo]/node_modules/redux-loop/index.d.ts(123,50): 
TS1005: ',' expected.
ERROR at [repo]i/node_modules/redux-loop/index.d.ts(127,54): 
TS1005: ',' expected.
ERROR at [repo]/node_modules/redux-loop/index.d.ts(128,13): 
TS2314: Generic type 'ReducerMapObject' requires 3 type argument(s).
ERROR at [repo]/node_modules/redux-loop/index.d.ts(131,57): 
TS1005: ',' expected.
ERROR at [repo]/node_modules/redux-loop/index.d.ts(134,13): 
TS2314: Generic type 'ReducerMapObject' requires 3 type argument(s).

In might be a good idea to start actually rewriting the codebase in typescript as well to catch these sorts of things. I don't think we have any coverage around our typings right now, do we? otherwise this would have caught it, right?

@bdwain

This comment has been minimized.

Copy link
Member

@bdwain bdwain commented Oct 10, 2017

Having played around with typescript a bit to work with the typings, I feel like rewriting everything with typescript might make it harder for people who don't use typescript to contribute. That, combined with the effort it will takes makes me think it's probably not worth it.

I think we could improve our tests to catch issues with our typings though for sure. We really need to improve the examples folder and a few different kinds in there, including a typescript project. I think that'd be really helpful.

@gniquil

This comment has been minimized.

Copy link
Contributor

@gniquil gniquil commented Oct 10, 2017

@ashishsc For you first issue, Cmd.run, I've run into this as well. The solution is to make your action creators return the union Action type. That should solve your problem. However, if you want to adopt this approach, your action creators need to be able to import the root Action type, which implies that action creators need to exist completely separately from your action definitions. This is generally different from the way people code in normal javascript (say duck style).

Step back a bit, and being a super fan of Elm (but not be able to use it for various reasons), adopting this library with typescript, you should stick to the Elm architecture as much as possible. Strict typing points out many issues exist in the normal redux architecture. So, decompose your app into multiple sub reducers and use Cmd.map to wrap actions (generally the "fractal pattern"). At work, once we started doing this, most of our typing related issues are gone.

@ashishsc

This comment has been minimized.

Copy link
Author

@ashishsc ashishsc commented Oct 11, 2017

@gniquil I am also a huge fan of elm, hence why I came to loop. I am not sure that our action creators should return the union Action type. I take advantage of the fact that the creators can return a specific kind of action. Although now that you mention it, that's similar to how in elm, the view functions return Html Msg where Msg is a union type. @willisplummer was that the intent?

@bdwain I don't it would be too bad to rewrite in typescript since we already have the typings written, we just need to annotate our actual functions with them, but I do agree that it would make it harder for non-tsers to contribute. I also agree that we really need an example of using typescript in the examples folder. I could take that on after I get this working.

@willisplummer

This comment has been minimized.

Copy link
Contributor

@willisplummer willisplummer commented Oct 11, 2017

Yeah, @gniquil has the right idea here. Take a look at the tests i wrote for the typescript definitions to get a sense of how this is working: https://github.com/redux-loop/redux-loop/blob/master/test/typescript/loopReducer.ts

I agree that a TypeScript rewrite wouldn't actually be very hard to do. Adding function annotations is generally fairly straightforward, and it would give us more confidence that our types were accurate. It's easy for a declaration file to get out of sync with the JavaScript code if someone makes a change and doesn't remember to update the types, and actually using .ts files would prevent this possibility.

The ReducerMapObject errors you're running into are weird to me. AFAICT the compiler should be able to infer the type of K from S, right? (cc @jaredklewis )

@willisplummer

This comment has been minimized.

Copy link
Contributor

@willisplummer willisplummer commented Oct 11, 2017

@ashishsc I'm not sure what an action creator would return if not the union type of all the possible actions for a given reducer or more broadly redux's AnyAction type?

@willisplummer

This comment has been minimized.

Copy link
Contributor

@willisplummer willisplummer commented Oct 11, 2017

Another argument that I see for pursuing a TypeScript rewrite is that defining your API's can quickly make architectural complexity more apparent. (When a function accepts weird union types or is difficult to type, that's generally a good indicator that it's attempting to do too much).

@gniquil

This comment has been minimized.

Copy link
Contributor

@gniquil gniquil commented Oct 11, 2017

Like Cmd.run. Wouldn't it be awesome if it type checks the args against effect func arguments, and resolved promise wrapper against success/fail actionCreators. Too bad typescript can't do the following

Cmd.run: <T, R, A>(f: (...args: T) => Promise<R>, { args: T, successActionCreator: (result: R) => A, failActionCreator: (error: any) => A }): CmdType<...>

At work we reexport a more restricted version of Cmd.run, forcing effect func to only take 1 arg (and completely forbid getState and dispatch).

@ashishsc

This comment has been minimized.

Copy link
Author

@ashishsc ashishsc commented Oct 11, 2017

I take issue with the way the Redux uses AnyAction, I like to do in my reducers and subreducers

switch(action.type)
    case FOO: {
         ....
    },
    ...,
    default:
        thisFailsAtCompileTime(action)

but with the way redux defines AnyAction this isn't possible.

In order to address this in our work project, we instead define our own type for the reducer:

import { Loop as ConstructLoopType } from 'redux-loop';
type Loop = ConstructLoopType<State,Action> | State;
@ashishsc ashishsc added the discussion label Oct 11, 2017
@ashishsc

This comment has been minimized.

Copy link
Author

@ashishsc ashishsc commented Oct 11, 2017

Wow I'm so blind. Sorry guys I didn't even see https://github.com/redux-loop/redux-loop/blob/master/test/typescript/loopReducer.ts . It'd be worth moving that into the examples dir.

Also I think my type errors could be a tsc version mismatch? I'll confirm later.

@willisplummer

This comment has been minimized.

Copy link
Contributor

@willisplummer willisplummer commented Oct 11, 2017

@gniquil we use a stricter overloaded version too so that it can accept up to 6 args. i'll look into opening a PR for that shortly

@bdwain

This comment has been minimized.

Copy link
Member

@bdwain bdwain commented Oct 12, 2017

@ashishsc I think the typescript test is good for a test, but it's kind of just a bunch of random stuff. I think we should have a full fledged app example in the examples folder. Maybe that would also make a good test too.

My two main issues with making the whole library typescript is that it

  • Makes it harder for people who don't use typescript to contribute, while typescript users can easily contribute to a js library. Considering not many other redux/react libraries use typescript, I'm worried we'd cut off a lot of potential contributors.
  • Signals to users of the library that typescript is encouraged and the ideal way to use the library, which also would turn people off (that's what happened with angular 2 for a lot of people)

If the community started moving more in the direction of typescript, I think we'd see other libraries (especially redux) start doing the same thing and then I think those two issues would be much less of a concern.

For now, is it alright if we just work to improve the reliability of our typings using tests and examples? If that still ends up being a problem, then maybe converting the whole thing would be the answer. I'd rather take the least drastic step we need to have reliable typings for people using the library.

@ashishsc

This comment has been minimized.

Copy link
Author

@ashishsc ashishsc commented Oct 12, 2017

I think angular2's main issue was the confusing tooling, identity-crisis api, and several other issues indirectly related or maybe slightly compounded by typescript. I do agree that we shoul dhave an example for now though, I wouldn't want to force ts or make it required.

@ashishsc

This comment has been minimized.

Copy link
Author

@ashishsc ashishsc commented Oct 12, 2017

So I confirmed that my issues were from using typescript@2.4, the issues went away after, I should've realized that sooner. This thread can keep serving as a discussion for what we want to do with typescript, however.

@ashishsc ashishsc removed the bug label Oct 12, 2017
@gniquil

This comment has been minimized.

Copy link
Contributor

@gniquil gniquil commented Oct 13, 2017

@willisplummer Looking forward to seeing your PR. But just now I came across this:

function f<...T,...U>() {}
}
class C<...T> {
}

microsoft/TypeScript#5453

Variadic kinds in 2.7. It would solve so many problems...

@gniquil

This comment has been minimized.

Copy link
Contributor

@gniquil gniquil commented Oct 13, 2017

Oops mis-spoke, not 2.7, but at least in the roadmap.

https://github.com/Microsoft/TypeScript/wiki/Roadmap

@seethroughtrees

This comment has been minimized.

Copy link

@seethroughtrees seethroughtrees commented Nov 7, 2017

I appear to be having the same type issue as @ashishsc for Cmd. run in #160 (comment), where the output is expecting all actions to have the same type.

i'm not sure how if the example linked in here explains how this is any different? can someone elaborate on how we should type the output of loops that use Cmd.run ?

@seethroughtrees

This comment has been minimized.

Copy link

@seethroughtrees seethroughtrees commented Nov 8, 2017

It seems your response was deleted @willisplummer , but thanks for the response and the typings you provided for the library!

In case anyone has this problem in the future, I found typing the output of the individual action creators was creating the conflict for me. I changed each action creator output type to the same generic union type as I'm passing to the reducer. Not sure if this is ideal, but it now works as expected. Thanks again.

@willisplummer

This comment has been minimized.

Copy link
Contributor

@willisplummer willisplummer commented Nov 8, 2017

@seethroughtrees glad to hear you got it working - sounds like you're doing things correctly. i wrote that comment out and then deleted because i wanted to revisit when i had a little more time to articulate how it should be done 😬

if you wanted to help out, there have been a few requests for additional examples and documentation of how to use Loop w/ Typescript

@ashishsc

This comment has been minimized.

Copy link
Author

@ashishsc ashishsc commented Nov 12, 2017

@seethroughtrees That is in fact how it is meant to be used. I didn't realize it at first, but it is the same as the way Elm defines its Cmds in that respect. The benefit of making it generic to the union Action type in the actions is that it allows typescript to do the work to figure out which action is being called, letting you change which one of your specific actions you want to call without having to update the signature. This makes sense because in practice, you generally want your actions to be called within the same scope (in other words, same set of actions relevant to your module.)

Sorry if I'm not articulating that well.

@seethroughtrees

This comment has been minimized.

Copy link

@seethroughtrees seethroughtrees commented Nov 12, 2017

Makes perfect sense @ashishsc , thanks for the perspective.

@willisplummer , i can definitely help there. this is my first project using typescript instead of flow. when i get my patterns finalized, i'll be happy to submit some examples. :)

@bdwain

This comment has been minimized.

Copy link
Member

@bdwain bdwain commented Feb 7, 2018

@barberousse

This comment has been minimized.

Copy link

@barberousse barberousse commented Feb 17, 2018

Another thing about y'alls types is that this:

export interface LoopReducer<S, A extends Action> {
  (state: S | undefined, action: AnyAction): S | Loop<S, A>;
}

is really bad in that a reducer should never return undefined nor should the state parameter ever be undefined. You have to realize you're forcing everyone downstream to type their Reducer to expect undefined as well insert type guards everywhere to satisfy tsc.

@bdwain

This comment has been minimized.

Copy link
Member

@bdwain bdwain commented Feb 17, 2018

@barberousse Redux will pass undefined to a reducer initially so that you can return your initial state.

We'll start by specifying the initial state. Redux will call our reducer with an undefined state for the first time. This is our chance to return the initial state of our app:

(from redux docs)

@barberousse

This comment has been minimized.

Copy link

@barberousse barberousse commented Feb 18, 2018

@bdwain

This comment has been minimized.

Copy link
Member

@bdwain bdwain commented Feb 18, 2018

How can a reducer accept an undefined state if it's explicitly typed to not accept it?

@bdwain

This comment has been minimized.

Copy link
Member

@bdwain bdwain commented Feb 18, 2018

@barberousse

This comment has been minimized.

Copy link

@barberousse barberousse commented Feb 18, 2018

@bdwain

This comment has been minimized.

Copy link
Member

@bdwain bdwain commented Feb 18, 2018

export interface LoopReducer<S, A extends Action> {
  (state: S | undefined, action: AnyAction): S | Loop<S, A>;
}

The return value does not allow for an undefined return value as far as I can tell. Only a state or a loop. Undefined is only mentioned as an input type for the state input.

Also, if the reducer was not typed to allow undefined as a State value, then this bad code would get through the type checking.

//no default param for initialState
function reducer(state, action){
   switch(action.type){ //pretty sure there is no action param when redux passes undefined
      //handle some action types
     default:
        return state; //would return undefined if state was undefined
   }
}

That would break immediately at runtime but be valid according to the type system. If you instead say that undefined is a valid input for parameter 1, but not a valid return value, it would detect that you tried to return a value that was potentially undefined and fail typechecking.

@barberousse

This comment has been minimized.

Copy link

@barberousse barberousse commented Feb 18, 2018

That's fair

@bdwain

This comment has been minimized.

Copy link
Member

@bdwain bdwain commented May 5, 2018

I'm going to close this since there's an example on the site now.

@bdwain bdwain closed this May 5, 2018
@RichieAHB

This comment has been minimized.

Copy link

@RichieAHB RichieAHB commented Nov 23, 2018

As an addendum to this I'm using reduceReducers to bolt on a side effecting loop reducer that has access to the whole state tree (using selectors to access what it needs). I had the above issue with typescript complaining about undefined. I figured that to get the initial state from my plain reducer I'd just do what redux does under the hood and call the reducer for the first time:

const initialState = root(undefined, { type: '@@@@@' });

const effects = (
  state: State = initialState,
  action: Action
): State | Loop<State, Action> => { /* ... */ }

It won't break at runtime and will keep up to date with any changes in my other reducers 👍 - thought it was worth sharing as it seems obvious but the fact I'm on this issue suggests I had to do some searching to get around this issue myself!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.