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

Improve typings for Middleware #2530

Closed

Conversation

davidka
Copy link

@davidka davidka commented Jul 26, 2017

This PR fixes Issue #2481

@timdorr
Copy link
Member

timdorr commented Jul 27, 2017

Can I get some eyes on this to confirm this fixes the issue? Thanks!

@tbo
Copy link

tbo commented Jul 27, 2017

This doesn't fix the issue for me. I get additional error messages:

    ERROR in [...]/node_modules/redux-thunk/index.d.ts
    (14,22): error TS2314: Generic type 'Middleware<S>' requires 1 type argument(s).
    ERROR in [...]/node_modules/redux-thunk/index.d.ts
    (15,42): error TS2314: Generic type 'Middleware<S>' requires 1 type argument(s).
    ERROR in [...]/shared/createStore.ts
    (27,3): error TS2345: Argument of type '(store: MiddlewareAPI<IState>) => (next: any) => (action: any) => any' is not assignable to parameter
 of type 'GenericMiddleware'.
      Types of parameters 'store' and 'api' are incompatible.
        Type 'MiddlewareAPI<S>' is not assignable to type 'MiddlewareAPI<IState>'.
          Type 'S' is not assignable to type 'IState'.

@aikoven
Copy link
Collaborator

aikoven commented Jul 27, 2017

This will break any existing usage of Middleware type since it now became a generic type.

We should rather add a new type without touching existing ones.

@davidka
Copy link
Author

davidka commented Jul 27, 2017

I could do it the other way and introduce a new type SpecificMiddleware:

export interface SpecificMiddleware<S> {
  (api: MiddlewareAPI<S>): (next: Dispatch<S>) => Dispatch<S>;
}

export interface Middleware {
  <S>(api: MiddlewareAPI<S>): (next: Dispatch<S>) => Dispatch<S>;
}

But if we would update the typescript dependency from 1.8.0 to 2.4.2, we could make it even more elegant with only one type:

export interface Middleware<T=any> {
    <S>(api: MiddlewareAPI<S>): (next: Dispatch<S>) => Dispatch<S>;
    (api: MiddlewareAPI<T>): (next: Dispatch<T>) => Dispatch<T>;
  }

@timdorr Does anything stand against?

@tbo
Copy link

tbo commented Jul 27, 2017

@davidka I don't think we should force everybody to use the latest typescript version. Some people might run into problems with unresolved conflicts in other dependencies or have other restrictions in place.

@davidka davidka force-pushed the improve-typings-for-middleware branch from d59de69 to d749ec1 Compare July 27, 2017 13:18
@davidka
Copy link
Author

davidka commented Jul 27, 2017

@tbo yeah that make sense.
I have now added an extra typing SpecificMiddleware which accepts a specific type.
But I do not really like that.

@davidka
Copy link
Author

davidka commented Aug 1, 2017

@timdorr Could we merge that?

@tbo
Copy link

tbo commented Aug 1, 2017

I retested this and it still doesn't fix the issue for me. Maybe it is limited to my specific setup, so I would suggest, that somebody else should test this as well.

@timdorr
Copy link
Member

timdorr commented Sep 1, 2017

Superceded by #2563

@timdorr timdorr closed this Sep 1, 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
Development

Successfully merging this pull request may close these issues.

4 participants