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 Error in Middleware #82

Closed
unstubbable opened this issue Jul 7, 2016 · 37 comments
Closed

TypeScript Error in Middleware #82

unstubbable opened this issue Jul 7, 2016 · 37 comments

Comments

@unstubbable
Copy link

The overloading of Dispatch by redux-thunk breaks middlewares that only handle standard action objects (as apposed to thunks).

Example:

import {Action, Dispatch, Middleware, Store} from 'redux';
import {IAppState} from '../reducers';

export const middleware: Middleware = (store: Store<IAppState>) => (next: Dispatch<IAppState>) => (action: Action) => {
  // Do something with the action ...
  next(action);
};

The resulting error is:

Type '(store: Store<IAppState>) => (next: Dispatch<IAppState>) => (action: Action) => void' is not assignable to type 'Middleware'.
  Type '(next: Dispatch<IAppState>) => (action: Action) => void' is not assignable to type '(next: Dispatch<any>) => Dispatch<any>'.
    Type '(action: Action) => void' is not assignable to type 'Dispatch<any>'.
      Types of parameters 'action' and 'asyncAction' are incompatible.
        Type '(dispatch: Dispatch<any>, getState: () => any, extraArgument: any) => any' is not assignable to type 'Action'.
          Property 'type' is missing in type '(dispatch: Dispatch<any>, getState: () => any, extraArgument: any) => any'.

A possible fix to remove the compiler error would be to declare a union type for the Action, i.e. Action | Redux.ThunkAction<any, IAppState, any>:

export const middleware: Middleware = (store: Store<IAppState>) => (next: Dispatch<IAppState>) => (action: Action | Redux.ThunkAction<any, IAppState, any>) => {
  // Do something with the action ...
  next(action as Action);
};

But this is incorrect if your middleware only handles Action!

The better solution would be to declare a Dispatch interface within the redux-thunk module that extends Redux.Dispatch. This Dispatch could then be used in action creators that return a ThunkAction:

import {IAppState} from '../reducers';
import {ThunkAction, Dispatch} from 'redux-thunk';

export function thunkedActionCreator(): ThunkAction<void, IAppState, void> {
  return (dispatch: Dispatch<IAppState>, getState: () => IAppState): void => {
    // Do something async and dispatch actions or other thunks ...
  };
}

Middlewares on the other hand would use Redux.Dispatch (see middleware example above).

unstubbable pushed a commit to unstubbable/redux-thunk that referenced this issue Jul 7, 2016
The current implementation of the typings makes it impossible
to create a middleware that only handles standard actions. This means
a middleware must handle thunked actions even though it never actually
handles them during runtime. (see issue and examples in reduxjs#82)

By declaring the Dispatch and Store interfaces again instead of
overloading the original implementation of redux a consumer of
redux-thunk can choose the right implementation for his use case
(e.g. ActionCreator vs. Middleware).

fixes reduxjs#82
@unional
Copy link

unional commented Jul 19, 2016

I think something is missing here. You do need to augment the Dispatch<S> from redux so that you can call store.dispatch() with thunk.

@unstubbable
Copy link
Author

@unional Yeah, I didn't really explain this case. You can see in my PR that the Store needs to be imported from redux-thunk as well in order for this to work:

import thunk, { Dispatch, ThunkAction, Store } from '../index.d.ts';

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

store.dispatch((dispatch, getState, extraArg) => {
  console.log(extraArg);
});

This is of course a very synthetic example. In a real application you would probably have something like a configureStore function that would return an instance of the Store by redux-thunk, e.g.:

import {applyMiddleware, createStore} from 'redux';
import thunk, {Store} from 'redux-thunk';
import rootReducer, {IAppState} from './rootReducer';

export default function configureStore(initialState: IAppState): Store<IAppState> {
  // You could also configure devtools here and compose with middleware enhancer.
  return createStore(rootReducer, initialState, applyMiddleware(thunk));
}

This instance would then be able to dispatch thunks.

@unional
Copy link

unional commented Aug 2, 2016

Well, I think that's a problem. Store does not belongs to redux-thunk, so we should not export and require user to import Store from redux-thunk.

Think about it this way: If redux and redux-thunk are written in TS to begin with, what should have happened? It is an augmentation of a method and should be remain as so.

My two cents, 🌷

@unstubbable
Copy link
Author

@unional, maybe I'm misunderstanding something, but let me try to give you a different perspective.

The Store created by createStore in the above example is created by the enhancer returned by applyMiddleWare, which takes the Store instance from the original createStore and replaces the dispatch method with the composed version of all supplied middlewares. Since thunk is the first middleware in the chain, Store.dispatch is in fact not the base implementation by redux but the one by redux-thunk. This is expressed by

export interface Store<S> extends ReduxStore<S> {
  dispatch: Dispatch<S>;
}

in my PR and consequently the interface has to be imported from redux-thunk and used as the return type of configureStore if this is what configureStore is doing internally.

@unional
Copy link

unional commented Aug 2, 2016

I can understand what is your confusion.

If it is written in typescript, you would do module augmentation. And the Store is still imported from redux instead of redux-thunk.

If it follows your implementation, then other packages which augment Dispatch would need to do the same thing. If user wants to use both redux-thunk and redux-promise, where should he/she gets the Store from?

@unstubbable
Copy link
Author

unstubbable commented Aug 2, 2016

If it follows your implementation, then other packages which augment Dispatch would need to do the same thing. If user wants to use both redux-thunk and redux-promise, where should he/she gets the Store from?

From whichever is first in the middleware chain. After all the signature of this dispatch matches the one that applyMiddleWare assigns to the Store, doesn't it?

@unional
Copy link

unional commented Sep 27, 2016

From whichever is first in the middleware chain.

IMO is not a proper picture. Consider these:

import { createStore, applyMiddleware } from 'redux';
import thunk from 'redux-thunk';
import prom from 'redux-promise';
import rootReducer from './reducers';
import { Store } from '???'

// Note: this API requires redux@>=3.1.0
const store: Store = createStore(
  rootReducer,
  applyMiddleware(thunk, prom)
);

const store2: Store = createStore(
  rootReducer,
  applyMiddleware(prom, thunk)
);

Where should the Store come from? the order of the middleware should have nothing to do with where you get the Store from. That's why it is an augmentation.

@unstubbable
Copy link
Author

I see your point. But how can you write a middleware with augmentation then and solve this issue?

@born2net
Copy link

born2net commented Dec 1, 2016

I am using the latest ts and still same issue of:

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'.

as I am dispatching a thunk

any ideas?

@unional
Copy link

unional commented Dec 1, 2016

I see your point. But how can you write a middleware with augmentation then and solve this issue?

With module augmentation (as it is right now), you will get the Store from redux.

@unional
Copy link

unional commented Dec 1, 2016

@born2net I think you did not have the typings loaded. It is not distributed in 2.1
You need to get it from github somehow, or use typings for this purpose.

@born2net
Copy link

born2net commented Dec 1, 2016

you have a link to the latest / proper definition file I should use?

@unional
Copy link

unional commented Dec 1, 2016

@born2net
Copy link

born2net commented Dec 1, 2016

tx!!!!!

@born2net
Copy link

born2net commented Dec 1, 2016

unfortunately overriding still results with same error:

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'.

@unional
Copy link

unional commented Dec 1, 2016

Can you give a sample of how are you using it?

@born2net
Copy link

born2net commented Dec 1, 2016

sure...

export class MyComp {
  constructor(private store: NgRedux<any>) {
    this.store.dispatch(this.act('222'));
  }

  private act = (i_value) => {
    return (dispatch) => {
      setTimeout(() => {
        dispatch({type: '555', payload: i_value})
      }, 500)
    }
  }
}

@unional
Copy link

unional commented Dec 1, 2016

How do you get load the typings into your project? Are you using typings?

@born2net
Copy link

born2net commented Dec 1, 2016

FYI if I put :any it fixes it...

 private act = (i_value) :any

@born2net
Copy link

born2net commented Dec 1, 2016

I did not install any typings and just using my global typings.d.ts (which works great for all my other typings stuff) and pasted your code in... is that wrong?

@born2net
Copy link

born2net commented Dec 1, 2016

even after installing, commenting out and putting updated one, same issue

// import * as Redux from "redux";
// import { Middleware } from "redux";

// export as namespace ReduxThunk;
//
// declare const thunk: Middleware & {
// 	withExtraArgument(extraArgument: any): Middleware;
// };
// export default thunk;
//
// declare module 'redux' {
// 	export type ThunkAction<R, S, E> = (dispatch: Dispatch<S>, getState: () => S, extraArgument: E) => R;
//
// 	export interface Dispatch<S> {
// 		<R, E>(asyncAction: ThunkAction<R, S, E>): R;
// 	}
// }



import {Middleware, Dispatch} from "redux";
export type ThunkAction<R, S, E> = (dispatch: Dispatch<S>, getState: () => S,
                                    extraArgument: E) => R;
declare module "redux" {
  export interface Dispatch<S> {
    <R, E>(asyncAction: ThunkAction<R, S, E>): R;
  }
}
declare const thunk: Middleware & {
  withExtraArgument(extraArgument: any): Middleware;
};
export default thunk;

@unional
Copy link

unional commented Dec 1, 2016

Nope, that most likely would not work. Your typings.d.ts is probably a global script file while what's in there is a module file, you can wrap this in declare module redux-thunk { ... } or use `typings to do that for you.

@born2net
Copy link

born2net commented Dec 1, 2016

ok ya tried other way (see prev msg) ... same :/

@unional
Copy link

unional commented Dec 1, 2016

typings install github:gaearon/redux-thunk -S

Then in your tsconfig.json include typings/index.d.ts in the files section.

@born2net
Copy link

born2net commented Dec 1, 2016

ok I will be back in one hour and will try it ASAP thanks again for the support

@born2net
Copy link

born2net commented Dec 1, 2016

tried to install and got:

typings WARN badlocation "github:gaearon/redux-thunk" is mutable and may change, consider specifying a commit hash
typings ERR! message Unable to resolve "github:gaearon/redux-thunk"
typings ERR! caused by https://raw.githubusercontent.com/gaearon/redux-thunk/master/typings.json responded with 404, expected it to equal 200
typings ERR!
typings ERR! cwd C:\msweb\msbarcode
typings ERR! system Windows_NT 10.0.14393
typings ERR! command "C:\\Program Files\\nodejs\\node.exe" "C:\\Users\\root\\AppData\\Roaming\\npm\\node_modules\\typings\\dist\\bin.js" "install" "github:gaearon/redux-thunk" "-SD"
typings ERR! node -v v6.5.0
typings ERR! typings -v 2.0.0
typings ERR!
typings ERR! If you need help, you may report this error at:
typings ERR!   <https://github.com/typings/typings/issues>

@unional
Copy link

unional commented Dec 1, 2016

Forgot that this repo doesn't have typings.json.
Do this: typings install -S redux-thunk=github:gaearon/redux-thunk/index.d.ts

@born2net
Copy link

born2net commented Dec 1, 2016

still didn't do it

image

@unional
Copy link

unional commented Dec 1, 2016

Ar, its webstorm. Try to see if it compiles ok or not. It should be. At this point, it is webstorm issue.

@born2net
Copy link

born2net commented Dec 1, 2016

I understand... I appreciate the time...
regards

@born2net
Copy link

born2net commented Dec 1, 2016

not sure if u r into ng2, but if u r:
Angular 2 Kitchen sink: http://ng2.javascriptninja.io
and source@ https://github.com/born2net/Angular-kitchen-sink
Regards,

Sean

@paramsingh88
Copy link

@unional @born2net Same Issue! Have tried everything. Please help

@born2net
Copy link

param please follow my setup: https://github.com/born2net/Angular-kitchen-sink

@aikoven
Copy link
Collaborator

aikoven commented Jan 18, 2017

Released v2.2.0 with TypeScript definitions included.

@cwharris
Copy link

cwharris commented May 18, 2017

I would expect that Thunk would not overwrite the Redux API without also being backwards compatible. However, this is not the case. Instead, here's a workaround for Middleware type incompatibility found to be working in typescript@2.2.

Fair warning: There's a possibility this is not intended behavior for the TypeScript compiler.

import * as Redux from "redux";
import * as ReduxThunk from "redux-thunk";

const middleware: Redux.Middleware =
  (api: Redux.MiddlewareAPI<void>) => (next: Redux.Dispatch<void>) => <A extends Action>(action: A) => {
    return next(action);
  };

@JackSkylark figured this one out.

@athornburg
Copy link

Hello! I worked with @rodolfo2488 on a fix for this. We believe that this pull request would fix the issue.

@unstubbable
Copy link
Author

I think the original issue can be resolved with improved typings based on reduxjs/redux#2563 which hopefully lands soon.

unstubbable pushed a commit to unstubbable/redux-thunk that referenced this issue Sep 11, 2017
The current implementation of the typings makes it impossible
to create a middleware that only handles standard actions. This means
a middleware must handle thunked actions even though it never actually
handles them during runtime. (see issue and examples in reduxjs#82)

By declaring the Dispatch and Store interfaces again instead of
overloading the original implementation of redux a consumer of
redux-thunk can choose the right implementation for his use case
(e.g. ActionCreator vs. Middleware).

fixes reduxjs#82
@timdorr timdorr closed this as completed May 25, 2018
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 a pull request may close this issue.

8 participants