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

Return type of dispatch is unknown in beta #1705

Closed
thorn0 opened this issue Nov 7, 2021 · 11 comments
Closed

Return type of dispatch is unknown in beta #1705

thorn0 opened this issue Nov 7, 2021 · 11 comments
Labels
bug Something isn't working
Milestone

Comments

@thorn0
Copy link
Contributor

thorn0 commented Nov 7, 2021

See https://codesandbox.io/s/reverent-firefly-ru026?file=/src/index.tsx
With @reduxjs/toolkit 1.6.2, the type error goes away as the type of dispatchResult is inferred correctly. With 1.7.0-beta.x, it's unknown.

@markerikson markerikson added this to the 1.7 milestone Nov 7, 2021
@markerikson markerikson added the bug Something isn't working label Nov 7, 2021
@phryneas
Copy link
Member

phryneas commented Nov 7, 2021

Since we did not change anything around those types in RTK, could this be related to the bump of the thunk version @markerikson ?

@markerikson
Copy link
Collaborator

That was my thought, yes, especially since when I hover over store.dispatch in the sandbox I see the "this is a union overload to work around a TS issue" comment that I added in 2.4.

Not sure where the unknown is coming from, though.

@Grapedge
Copy link
Contributor

I think this may be caused by these lines: https://github.com/reduxjs/redux-thunk/blob/ce76464960d5f1236460352fd3f2454e930f3665/src/types.ts#L30-L32

const dispatch: ThunkDispatch<{}, {}, AnyAction>
const v = dispatch({
  type: 'abc'
})
typeof v  // => unknown

But if I add this line to ThunkDispatch:

<Action extends BasicAction>(action: Action): Action

Now looks like this:

export interface ThunkDispatch<State, ExtraThunkArg, BasicAction extends Action>
  extends Dispatch<BasicAction> {
  <ReturnType>(
    thunkAction: ThunkAction<ReturnType, State, ExtraThunkArg, BasicAction>
  ): ReturnType
  <Action extends BasicAction>(action: Action): Action
  <ReturnType, Action extends BasicAction>(
    action: Action | ThunkAction<ReturnType, State, ExtraThunkArg, BasicAction>
  ): Action | ReturnType
}

it works:

typeof v // => { type: string }

I don't know why, and I found a more strange solution:

// rtk/configureStore.ts
export interface EnhancedStore<
  S = any,
  A extends Action = AnyAction,
  M extends Middlewares<S> = Middlewares<S>
> extends Store<S, A> {
  // from:
  dispatch: DispatchForMiddlewares<M> & Dispatch<A>
  // to:
  dispatch:  Dispatch<A> & DispatchForMiddlewares<M>
}

After I experimented, I found this also works.

@markerikson
Copy link
Collaborator

Interestingly, it seems like the "dispatch a thunk" aspect of the types is working fine here:

const dispatchResult = store.dispatch((dispatch, getState) => 42)
// hover dispatchResult: 'number'

it's the "dispatched a plain action" case that's returning unknown.

FWIW, the <Action extends BasicAction>(action: Action): Action line is supposed to be what's in the base Dispatch interface already, so not sure why that isn't helping here already.

@markerikson
Copy link
Collaborator

This might even be a problem with configureStore / getDefaultMiddleware specifically?

In that same sandbox, if I do middleware: [thunk], the returned action infers correctly.

@markerikson
Copy link
Collaborator

markerikson commented Nov 26, 2021

Hmm. A basic configureStore()-created store has this type:

const store: EnhancedStore<{
    counter: {
        value: number;
    };
}, AnyAction, [ThunkMiddleware<{
    counter: {
        value: number;
    };
}, AnyAction, null> | ThunkMiddleware<{
    counter: {
        value: number;
    };
}, AnyAction, undefined>]>

That seems.... sketchy? Why would there be two different ThunkMiddleware types in here, with different ExtraThunkArg types?

Oh. Maybe it comes from here?

export type ThunkMiddlewareFor<
  S,
  O extends GetDefaultMiddlewareOptions = {}
> = O extends {
  thunk: false
}
  ? never
  : O extends { thunk: { extraArgument: infer E } }
  ? ThunkMiddleware<S, AnyAction, E>
  :
      | ThunkMiddleware<S, AnyAction, null> //The ThunkMiddleware with a `null` ExtraArgument is here to provide backwards-compatibility.
      | ThunkMiddleware<S, AnyAction>

@markerikson
Copy link
Collaborator

I can confirm that if I add a couple additional typetests for this case in configureStore.typetest.ts that fail, and paste <Action extends BasicAction>(action: Action): Action into the local node_modules/redux-thunk/es/types.d.ts file, the typetests pass. So, I might as well go ahead and publish an updated redux-thunk version with that change. Can't see a reason not to.

@markerikson
Copy link
Collaborator

It does seem like ordering of these overloads is very fragile. Swapping them back and forth makes several of the thunk typetests fail depending on how they're ordered.

@markerikson
Copy link
Collaborator

Huh. I tried removing the hand-edits to node_modules/redux-thunk/es/types.d.ts so that it's back to 2.4.0 normal, and then confirmed that swapping the order of the intersection in EnhancedStore does also fix this issue.

With that order swapped, the type of dispatch with a counter is:

const dispatch: Dispatch<AnyAction> & ThunkDispatch<{
    counter: {
        value: number;
    };
}, null, AnyAction> & ThunkDispatch<{
    counter: {
        value: number;
    };
}, undefined, AnyAction>

My guess is that the order of the intersections relates to the order of overloads being compared. If Dispatch<AnyAction> is last, TS hits one of the other overloads first.

I think I'm seeing the same unknown problem with RTK 1.6.2 and thunk 2.4.0, which makes sense.

Given that, I think I'm going to publish both the tweak to the thunk types, and the tweak to EnhancedStore as well.

@markerikson
Copy link
Collaborator

Just published https://github.com/reduxjs/redux-thunk/releases/tag/v2.4.1 containing the additional action overload.

@markerikson
Copy link
Collaborator

And also just merged #1772 which should fix it on the RTK side as well, and that will go out in the next 1.7 beta (or final).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants