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

Type definition of Epic<T = Action,S,D = any> is wrong? #392

Closed
kyasbal-1994 opened this Issue Jan 5, 2018 · 7 comments

Comments

Projects
None yet
3 participants
@kyasbal-1994

kyasbal-1994 commented Jan 5, 2018

Do you want to request a feature or report a bug?

BUG

What is the current behavior?

Type definition should be provided correctly.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsbin.com or similar.

I think Type definitions of redux-observable is wrong.

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

You can reproduce type error with following code.

enum ActionTypes{
     EXAMPLE_ACTION = "EXAMPLE_ACTION",
     EXAMPLE_ACTION2 = "EXAMPLE_ACTION2"
}

interface IState{

}

interface IExampleAction extends Action{
   type: ActionTypes.EXAMPLE_ACTION;
}

interface IExampleAction2 extends Action{
   type: ActionTypes.EXAMPLE_ACTION2;
}

type Actions = IExampleAction | IExampleAction2;

function createAction2():IExampleAction2{
   return {
      type: ActionTypes.EXAMPLE_ACTION2
   };
}

export const sampleEpic: Epic<IExampleAction, IState, any> = (action$: ActionsObservable<IExampleAction>, store: MiddlewareAPI<IState>) =>
    action$.ofType(ActionTypes.EXAMPLE_ACTION)
        .mapTo(createAction2())

This code should be generate following error.

        [ts]
Type '(action$: ActionsObservable<IExampleAction>, store: MiddlewareAPI<IState>) => Observable<IExample...' is not assignable to type 'Epic<IExampleAction, IState, any, IExampleAction>'.
  Type 'Observable<IExampleAction2>' is not assignable to type 'Observable<IExampleAction>'.
    Type 'IExampleAction2' is not assignable to type 'IExampleAction'.
      Types of property 'type' are incompatible.
        Type 'ActionTypes.EXAMPLE_ACTION2' is not assignable to type 'ActionTypes.EXAMPLE_ACTION'.

What is the expected behavior?

I guess declaration of Epic should be like this.

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

Which versions of redux-observable, and which browser and OS are affected by this issue? Did this work in previous versions of redux-observable?

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jan 5, 2018

Hey there, thanks for reporting! Can you describe what in the type signature is wrong specifically?

@kyasbal-1994

This comment has been minimized.

kyasbal-1994 commented Jan 5, 2018

@jayphelps Thank you for replying me rapidly.
I think an epic can return an observable that returns another type of action.

I think ping pong example is good for describing this problem.

const pingEpic = action$ =>
  action$.ofType('PING')
    .delay(1000) // Asynchronously wait 1000ms then continue
    .mapTo({ type: 'PONG' });

In this exmaple, the input is ActionObservable<{type:'PING'}> and output is Observable<{type:'PONG'}>. When we use current definition of Epic, we should make these type corresponded with.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jan 5, 2018

@kyasbal-1994 I think this is a duplicate of #219 and #390, can you confirm?

The major point is that, although it might not be obvious, an epic always recursively receives any actions itself emits, as well as every other action dispatched. It is true that you most often will filter them out with ofType, but that's not always the case so we can't make that assumption.

I still think there would be benefit being able to specify output actions separately since they are indeed a subset. But no matter what I try TypeScript doesn't seem to correctly type check the return value generic param. It makes me have an Observable, but doesn't care what type it is.

Full code: https://gist.github.com/jayphelps/9c527880a16b1470e22c6006a584d7b3

interface PingAction { type: 'PING' }
interface PongAction { type: 'PONG' }
interface SomethingElseAction { type: 'SOMETHING_ELSE' }
interface AnotherThingAction { type: 'ANOTHER_THING' }

type Actions
  = PingAction
  | PongAction
  | SomethingElseAction
  | AnotherThingAction
  ;

// The real output is PongAction but TypeScript doesn't
// seem to care and will accept anything. e.g. number
type OutputActions = number;

function pingEpic(action$: ActionsObservable<Actions>): Observable<OutputActions> {
  return action$.ofType<PingAction>('PING')
    .mapTo<PingAction, PongAction>({ type: 'PONG' });
}

I have to think that this isn't a bug in TS but rather something in the type definitions of redux-observable or rxjs cause TS to not care, possibly because of structural typing vs nominal but I don't see anything.

@moajo

This comment has been minimized.

Contributor

moajo commented Jan 7, 2018

@jayphelps
In this example, no type error occurs because type parameter T is not used in Observable<T> definition.
In TypeScript,function return type is covariant and arguments is bivariant .
Therefore, the type of the return value must be compatible.
For example, add a function declaration to the Observable type definition as follows.

declare class Observable<T> {
  mapTo<T, R>(this: Observable<T>, value: R): Observable<R>;
  someFunc(arg:T):any; // this function use T as argument.
}

Then an error will occur.

function pingEpic(action$: ActionsObservable<Actions>): Observable<OutputActions> {
  return action$.ofType<PingAction>('PING')
    .mapTo<PingAction, PongAction>({ type: 'PONG' }); // Error:Type 'Observable<PongAction>' is not assignable to type Observable<OutputActions>.
}

This is equivalent to replacing the definition with import statement as follows.

import { Observable } from "rxjs/Observable";
// declare class Observable<T> {
//   mapTo<T, R>(this: Observable<T>, value: R): Observable<R>;
// }
@moajo

This comment has been minimized.

Contributor

moajo commented Jan 7, 2018

Also for the problem that EPIC receives actions recursively, the return type must be a subset of the input.

The definition of Epic proposed by @kyasbal-1994 should be modified as follows.

export declare interface Epic<T extends Action, S, D = any, O extends T = T> {
  (action$: ActionsObservable<T>, store: MiddlewareAPI<S>, dependencies: D): Observable<O>;
}
@jayphelps

This comment has been minimized.

Member

jayphelps commented Jan 7, 2018

moajo added a commit to moajo/redux-observable that referenced this issue Jan 8, 2018

@moajo

This comment has been minimized.

Contributor

moajo commented Jan 8, 2018

@jayphelps sure!
i have submitted PR: #396

jayphelps added a commit that referenced this issue Jan 10, 2018

@jayphelps jayphelps closed this Jan 12, 2018

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