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

Add TypeScript definitions #77

Merged
merged 2 commits into from
Jun 24, 2016
Merged

Add TypeScript definitions #77

merged 2 commits into from
Jun 24, 2016

Conversation

aikoven
Copy link
Collaborator

@aikoven aikoven commented Jun 9, 2016

@Igorbek
Copy link

Igorbek commented Jun 11, 2016

Looks good to me. Does it compile well?

@aikoven
Copy link
Collaborator Author

aikoven commented Jun 14, 2016

@Igorbek It does and it infers types correctly, e.g. if I change const foo type to number here, I'll get an error:

declare const store: Store<{foo: string}>;

store.dispatch((dispatch, getState) => {
  const state = getState();

  const foo: string = state.foo;
});

@JabX
Copy link

JabX commented Jun 14, 2016

Could you also export the ThunkAction type (this one : <R>(dispatch: Dispatch<any>, getState?: () => any, extraArgument?: any) => R)? This way, we could annotate our action creators with it.

And wouldn't it be better if you also added a second type parameter for the extraArgument ?
Something like <R, E>(dispatch: Dispatch<any>, getState?: () => any, extraArgument?: E) => R. The types will be inferred anyway.

This way, we could write something like

function actionCreator(): ThunkAction<Api> {
    return async (dispatch, getState, api) => {
        // Our code here with complete inference
    }
}

I know I removed the state from the typings, but it could be added back with overloads.

@aikoven
Copy link
Collaborator Author

aikoven commented Jun 15, 2016

@JabX I'll do it, but I need some clarification.

To be able to annotate actionCreator with ThunkAction<Api> it should be defined as:

type ThunkAction<E> = <S, R>(dispatch: Dispatch<S>,
                             getState?: () => S,
                             extraArgument?: E) => R;

But now there's no way compiler could infer State type inside of actionCreator.

The most generic version would be:

type ThunkAction<S, E, R> = (dispatch: Dispatch<S>,
                             getState?: () => S,
                             extraArgument?: E) => R;

declare module "redux" {
  export interface Dispatch<S> {
    <E, R>(asyncAction: ThunkAction<S, E, R>): R;
  }
}

Now you can add your specific action type by narrowing ThunkAction:

type MyThunkAction<R> = ThunkAction<MyState, Api, R>;

What do you say?

@JabX
Copy link

JabX commented Jun 15, 2016

That's what I'd like to have, yeah.

But maybe we could have some overloads if other people want to specify some types but not all because they don't use them.

Maybe something like

type ThunkAction<R> = (dispatch: Dispatch<any>) => R
type ThunkAction<R, S> = (dispatch: Dispatch<S>, getState?: () => S) => R
type ThunkAction<R, S, E> = (dispatch: Dispatch<S>, getState?: () => S, extraArgument?: E) => R

(I'm not sure about removing the arguments when removing types).

By the way, why do we need a R type? I thought the general use of async actions wasn't to return anything?

@aikoven
Copy link
Collaborator Author

aikoven commented Jun 15, 2016

It's not possible to overload generic types, only call signatures.

R type is needed because dispatch call returns the result of thunk function.

@JabX
Copy link

JabX commented Jun 15, 2016

Oh, I thought we could, my mistake, I should have checked before.
Okay for your suggestion as is then.

@Igorbek
Copy link

Igorbek commented Jun 15, 2016

The following signature/type makes much more sense to me:

type ThunkAction<R, S, E> = (dispatch: Dispatch<S>, getState: () => S, extraArgument: E) => R;

Please note that both getState and extraArgument parameters are not optional. However, an actual thunk action can ignore it:

const tunkAction: ThunkAction<void /* have no return data */,
  {} /* don't care about state type*/,
  {} /* don't care about extra argument*/> = dispatch => {
  dispatch(a1());
  dispatch(a2());
};

@Strate
Copy link

Strate commented Jun 16, 2016

This style of augmentation works like global monkey patching. Even if I just require redux-thunk, but not use it in my store, Dispatch interface became augmented.
It would be much better, if augmentation of store.dispatch happens inside createStore method, based on applied store enhancers.

@JabX
Copy link

JabX commented Jun 16, 2016

Yes, that's correct, but there's absolutely no way of doing otherwise. It would require some kind of special support from Typescript for this scenario, which is terribly specific (well, there are other librairies which work like that (hello express), but still).

@joewood
Copy link

joewood commented Jun 23, 2016

Any chance of a merge?

@aikoven
Copy link
Collaborator Author

aikoven commented Jun 24, 2016

@ulfryk Could you please review the latest version and merge if everything's ok?

@ulfryk
Copy link
Contributor

ulfryk commented Jun 24, 2016

@aikoven - IMO it looks great! :)

@ulfryk ulfryk merged commit 00cc012 into reduxjs:master Jun 24, 2016
@unional
Copy link

unional commented Jul 6, 2016

Do you see the need of type S in Dispatch<S>? I suggest redux should remove the extra type.


declare module "redux" {
export interface Dispatch<S> {
<R, E>(asyncAction: ThunkAction<R, S, E>): R;
Copy link

Choose a reason for hiding this comment

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

@unional S is used here.

Copy link

Choose a reason for hiding this comment

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

Yes, but does it really help?

It can simply be:

type ThunkAction<R, S, E>  = (dispatch: Dispatch, getState: () => S, extraArgument: E) => R;
declare module 'redux' {
  export interface Dispatch {
    <R, S, E>(asyncAction: ThunkAction<R, S, E>): R;
  }
}

The point is the Dispatch does not gain anything by having the type S.

Copy link

Choose a reason for hiding this comment

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

Of course, it does. Your Dispatch is tied with your store which is typed by state type. With your signature one could redefine the state type, which would not be allowed in reality. I have the same concern with E extra argument type, but cannot be well defined with current design. I've been experimenting with some new approach that could address this issue as well.

Copy link

Choose a reason for hiding this comment

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

I see. That make sense.

I'm experimenting alternative approach that Store is not typed with <S>. Only reduce and getState is typed with <S> because the producer (which reduce) and consumer (which getState) have separate concerns.

@jimsugg
Copy link

jimsugg commented Sep 27, 2016

I am trying to use this, and I noticed that the npm version of this package still (as of today) does not include the index.d.ts file. Version numbers are the same (2.1.0) and index.d.ts was checked in three months ago, but it doesn't get downloaded on npm install. I tried getting the index.d.ts file from DefinitelyTyped, but that is different in that it wraps the ThunkAction and Dispatch<S> declarations into the 'Redux' namespace, without an export declaration to allow them to be visible outside the namespace. All of this is pretty confusing to those trying to use this for the first time. (I'm still trying to work it out.) Best would be to have the npm installer actually distribute the correct type declaration file. I am building a project that has to be usable by other dev teams, so I need something predictable in terms of package management.

@Igorbek
Copy link

Igorbek commented Sep 29, 2016

@jimsugg 2.1.0 was released on 10th May 2016, the PR got merged on 23 June.

@jimsugg
Copy link

jimsugg commented Sep 29, 2016

Yes, I noticed that. I am not familiar enough with npm publishing to know for sure why the npm build didn't get updated. I suspect you have to bump the version number, but there may be more to it than that. I solved my problem by pointing typings directly at the committed file in the github repo here. It would definitely be much better to get the npm build updated.

@jimsugg
Copy link

jimsugg commented Sep 29, 2016

It wouldn't be so bad if the one in DefinitelyTyped wasn't a problematic file. That one wraps the new interface and type in a namespace and then doesn't export them from that. It would be good to get that file updated with a good one as well.

@unional
Copy link

unional commented Sep 29, 2016

@jimsugg , what @Igorbek saying is the PR is merged after the release. we need @gaearon or anyone have access to make a new release on npm.

@jimsugg
Copy link

jimsugg commented Sep 30, 2016

Understood - that's what I suspected. Just wanted to point out the issues while they were fresh in my mind. Thanks!

@born2net
Copy link

born2net commented Dec 1, 2016

tried all the combinations and still getting:

Error:(13, 29) TS2345:Argument of type '(dispatch: any) => void' is not assignable to parameter of type 'Action'.
  Property 'type' is missing in type '(dispatch: any) => void'.

regards

Sean

@aikoven
Copy link
Collaborator Author

aikoven commented Jan 18, 2017

Released v2.1.1 with TypeScript definitions included.

@dzenzes
Copy link

dzenzes commented Jan 18, 2017

Thank you for your work and thank you for adding better TypeScript Support to redux-thunk. Sadly, for me the switch from 2.1.0 to 2.1.1 broke the build.

Error message:

Generic type 'ThunkAction' requires 3 type argument(s).

This was my fault because I defined the redux-thunk version as ^2.1.0 in the package.json. Anyway, was this behavior intended? I rolled back to 2.1.0 and will update my code.

@aikoven
Copy link
Collaborator Author

aikoven commented Jan 18, 2017

@dmies This is strange, where did your old typings come from? AFAIK, bundled typings have lower priority than local or installed from @types.

@dzenzes
Copy link

dzenzes commented Jan 18, 2017

@aikoven I had no other redux-thunk typings installed (tsconfig with allowJS=true). So I think the problem is, that there are suddenly new typings, the compiler can use.

@aikoven
Copy link
Collaborator Author

aikoven commented Jan 18, 2017

Ok, sorry, that's not good at all. I'll make another patch release with typings removed, and add them with minor release.

@aikoven
Copy link
Collaborator Author

aikoven commented Jan 18, 2017

Done: v2.2.0

@ashishsc
Copy link

Any idea how to describe ThunkActions that can dispatch more than one type of action?

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

Successfully merging this pull request may close these issues.