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] Generic action creators fail to resolve through ThunkDispatch overloads #248

Closed
Philipp91 opened this issue May 18, 2019 · 6 comments

Comments

@Philipp91
Copy link
Contributor

ThunkDispatch has two overloads:

export interface ThunkDispatch<S, E, A extends Action> {
  <T extends A>(action: T): T;
  <R>(asyncAction: ThunkAction<R, S, E, A>): R;
}

This works fine if I'm calling dispatch(xyz) where xyz has either type T or type ThunkAction<...>, as is normally the case with action creators, as they can declare what type they return. However, I have an interface where users of that interface need to provide an action creator. And the caller of that action creator doesn't care which of the two kinds of action is returned, it just passes that action to dispatch(). But there is no single type that captures "whatever one can pass to dispatch()". So currently I'm having the function return T | ThunkAction<...>. But that's impossible to pass to a ThunkDispatch function, because neither of the overloads accepts this union type.

The problem can be illustrated with a smaller example, and has been discussed in the TypeScript repository before (microsoft/TypeScript#14107):

interface Func {
    (a: number): number;
    (a: string): string;
}
function outer(func: Func, value: number | string): void {
    func(value);
}

A potential solution is to add a third overload:

    (a: number | string): number | string;
    <R, T extends A>(action: T | ThunkAction<R, S, E, A>): T | R;

Would that solution also make sense for redux-thunk? Or is there another solution for my problem?

Philipp91 added a commit to Philipp91/redux-thunk that referenced this issue Jul 6, 2019
See discussion in reduxjs#248 and microsoft/TypeScript#14107. Without this explicit overload, TypeScript is unable to figure out that the function can be called with an argument of type `T|ThunkAction<...>`.
timdorr added a commit that referenced this issue Jul 6, 2019
* #248 Add union overload to ThunkDispatch

See discussion in #248 and microsoft/TypeScript#14107. Without this explicit overload, TypeScript is unable to figure out that the function can be called with an argument of type `T|ThunkAction<...>`.

* Merge ThunkDispatch union overload with renamed type parameters

Co-Authored-By: Tim Dorr <timdorr@users.noreply.github.com>
@timdorr timdorr closed this as completed Jul 6, 2019
@Yaojian
Copy link

Yaojian commented Jul 9, 2019

I think the correct declaration is:

    export interface ThunkDispatch<S, E, A extends Action> {
        <A>(action: A): A extends (...args: any) => infer R ? R : A;
    }

@Philipp91
Copy link
Contributor Author

Wow that looks like magic. I'm unable to parse it in my head. Can you explain how it works with the first colon and the => -- maybe you can add a few redundant parentheses for clarity? And can you reformulate it with the longer type parameter names?

Do you propose this as the single overload, or as an additional one (to replace the additional one that was committed in #255?

This overload uses any and never uses S or E. Is it less type-safe than the others?

@Yaojian
Copy link

Yaojian commented Jul 10, 2019

extends is a keyword for conditional type

(...args: any) => infer R declares a function type (...args: any): any which

  • may have any number of parameters
  • return a value.

A extends (...args: any) => infer R is an expression that

  • be true if A is a function, otherwise false
  • infer R when A is a function

A extends (...args: any) => infer R ? R : A can be think as:

  • if A is an function like (...args: any): any
    • use the result type of the function (which is inferred as R) as the value of the whole expression
  • otherwise, use A as the value of the expression

in redux-thunk words, A extends (...args: any) => infer R ? R : A means:

  • dispatch is a function which
    • have an action parameter of type A (inferred automatically)
    • the result type is determined as
      • when action is a function (aka. a thunk), the result type is the result type of that function (inferred as R)
      • otherwise, the result type is the same as action

@timdorr
Copy link
Member

timdorr commented Jul 10, 2019

Can we get a PR for that implementation instead? I agree it's probably more correct.

@Philipp91
Copy link
Contributor Author

Philipp91 commented Jul 10, 2019

I see, thanks! So at least conceptually, the parentheses are:

<A>(action: A): (
  A extends (...args: any) => infer R // If `action` is a function
    ? R // Then dispatch will return the what `action` returned
    : A // Otherwise it will return `action` itself
);

Can we involve ThunkAction in this? The thunk can't be any kind of function with any kinds of parameters, it needs to have a certain signature, and enforcing that in TypeScript will make IDEs hint that signature, which is really useful. Wild guess, haven't tried:

<A>(action: A): (
  A extends ThunkAction<infer R, S, E, A> // If `action` is a thunk function
    ? R // Then dispatch will return the what `action` returned
    : A // Otherwise it will return `action` itself
);

And can we bring back T? I think the A parameter is the same across the entire application (e.g. type MyAppBasicAction = Action<string>). But when we dispatch a specific kind of action, we know that the same type is returned, and not just MyAppBasicAction. I believe in your code, A is declared twice -- once on the interface and once on the function. We should rename to T (just without the extends A type bound), so yours becomes:

export interface ThunkDispatch<S, E, A extends Action> {
    <T>(action: T): T extends (...args: any) => infer R ? R : T;
}

And if my guess above was any good, we need to keep the outer A wired into ThunkAction, so that any kind of action can be dispatched inside the thunk, not just the same kind:

<T>(action: T): (
  T extends ThunkAction<infer R, S, E, A> // If `action` is a thunk function
    ? R // Then dispatch will return the what `action` returned
    : T // Otherwise it will return `action` itself
);

Actually I propose using another letter, because A means "action" and T can be misunderstood as "thunk", so it should be something else to show that <T> can be either one.

I can try putting this into a PR. Do you prefer code-only, or with inlined comments? I guess it depends on the readers experience with condition types whether they will understand from the code alone.

@Philipp91
Copy link
Contributor Author

@Yaojian If I apply your (or my modified) solution, it generally works, but there's a TypeScript error just below:

export type ThunkMiddleware<S = {}, A extends Action = AnyAction, E = undefined> = Middleware<ThunkDispatch<S, E, A>, S, ThunkDispatch<S, E, A>>;
                                                                                                                         ^^^^^^^^^^^^^^^^^^^^^^
TS2344: Type 'ThunkDispatch<S, E, A>' does not satisfy the constraint 'Dispatch<AnyAction>'.
   Type 'T extends (...args: any) => infer R ? R : T' is not assignable to type 'T'.
     'T extends (...args: any) => infer R ? R : T' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint 'AnyAction'.
       Type 'unknown' is not assignable to type 'T'.
         'T' could be instantiated with an arbitrary type which could be unrelated to 'unknown'.
           Type 'AnyAction' is not assignable to type 'T'.
             'AnyAction' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint 'AnyAction'.

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

No branches or pull requests

3 participants