Repository: reduxjs/redux-toolkit
Pull request #502 (2020-04-13 22:50:12)

payloadCreator arg argument => asyncThunk arg type
This would address #489 without changing any external signatures to this point. So @stephen-dahl and @msutkowski  please take a look.

The complete behavioral change should be covered by this test:

https://github.com/phryneas/redux-toolkit/blob/47eac023cd18e86cf888224381a77b9776a97677/type-tests/files/createAsyncThunk.typetest.ts#L185-L230

My only concern is the handling of optional parameters. For people with `strictNullChecks: false` in their `tsconfig.json`, this would make the parameter optional for all their asyncThunks - which is only consequent, but may be undesirable.
I've gotta take a second look if this can be done better - on the other hand, we have the same behavior with `createSlice` actions already, so I would not try too much there.

Also, this *might* make extending createAsyncThunk with Typescript (#486) a little more difficult (we'd have to do some things for that anyways, but it might be simpler before this change), so I've got to evaluate that before we merge it.

But please already take a look.

netlify[bot]: Deploy preview for *redux-starter-kit-docs* ready!

Built with commit 380dc1d1252f3ea8fce5036629a6dc6355fe296b

https://deploy-preview-502--redux-starter-kit-docs.netlify.com
netlify[bot]: Deploy preview for *redux-starter-kit-docs* ready!

Built with commit 47eac023cd18e86cf888224381a77b9776a97677

https://deploy-preview-502--redux-starter-kit-docs.netlify.com
codesandbox[bot]: This pull request is automatically built and testable in [CodeSandbox](https://codesandbox.io).

  To see build info of the built libraries, click [here](https://ci.codesandbox.io/status/reduxjs/redux-toolkit/pr/502/builds/19655) or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 380dc1d1252f3ea8fce5036629a6dc6355fe296b:

|Sandbox| Source |
|--|--|
|[strange-chatterjee-gfs1g](https://codesandbox.io/s/strange-chatterjee-gfs1g)| Configuration |
|[determined-architecture-zkvod](https://codesandbox.io/s/determined-architecture-zkvod)| Configuration |
|[blue-wave-y6yk1](https://codesandbox.io/s/blue-wave-y6yk1)| Configuration |

codesandbox[bot]: This pull request is automatically built and testable in [CodeSandbox](https://codesandbox.io).

  To see build info of the built libraries, click [here](https://ci.codesandbox.io/status/reduxjs/redux-toolkit/pr/502/builds/19656) or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 47eac023cd18e86cf888224381a77b9776a97677:

|Sandbox| Source |
|--|--|
|[brave-herschel-237r0](https://codesandbox.io/s/brave-herschel-237r0)| Configuration |
|[jolly-jones-mcgrz](https://codesandbox.io/s/jolly-jones-mcgrz)| Configuration |
|[delicate-rgb-84ytk](https://codesandbox.io/s/delicate-rgb-84ytk)| Configuration |

msutkowski: @phryneas This looks pretty good! Would it be a bad idea to change `ActionCreator` to the type below _(just swapping the last two lines)_? I believe this would address the issue you mentioned with parameters being optional - but I'm not 100% sure 😅 

```ts
type ActionCreator = IsAny<
    ThunkArg,
    (arg: ThunkArg) => AsyncActionThunkAction,
    unknown extends ThunkArg
      ? (arg: ThunkArg) => AsyncActionThunkAction
      : [ThunkArg] extends [void] | [undefined]
      ? () => AsyncActionThunkAction
      : [undefined] extends [ThunkArg] | []
      ? (arg: ThunkArg) => AsyncActionThunkAction
      : (arg?: ThunkArg) => AsyncActionThunkAction
  >
```

I'm also not sure at all about how this would impact the issue you referenced with extending the types for `createAsyncThunk`, but I can help test that tomorrow. 

My 🧠 has trouble processing those type tests this late at night, so I made [a gist of the common typing patterns](https://gist.github.com/msutkowski/81a69be15e96028c848bf3764334a323) to check against as well. (I just dropped this in a `recipes` folder locally against your branch - if you have a better way of doing something similar for these purposes, please let me know).

If you look at the last sample in that gist, I did `arg: string | void`, and I don't feel like that's too awful to type to mimic `arg?: string`.

Thanks again for putting this up, and sorry in advance if this isn't helpful!

phryneas: > ```ts
> type ActionCreator = IsAny<
>     ThunkArg,
>     (arg: ThunkArg) => AsyncActionThunkAction,
>     unknown extends ThunkArg
>       ? (arg: ThunkArg) => AsyncActionThunkAction
>       : [ThunkArg] extends [void] | [undefined]
>       ? () => AsyncActionThunkAction
>       : [undefined] extends [ThunkArg] | []
>       ? (arg: ThunkArg) => AsyncActionThunkAction
>       : (arg?: ThunkArg) => AsyncActionThunkAction
>   >
> ```

That would not prevent the issue, but introduce two new ones:

* a asyncThunk resulting from a payloadGenerator like `(arg?: number) => 0` would look like `(arg: number) => ...`
* a asyncThunk resulting from a payloadGenerator like `(arg: number) => 0` would look like `(arg?: number) => ...`

Essentially, the question is if we handle optional arguments or just replace the last three lines with `(arg: ThunkArg) => AsyncActionThunkAction`, making the argument mandatory in all cases where it *could* be different than `undefined`.

Something like `number | void` leading to `(arg?: number) => AsyncActionThunkAction` would be a possibility, too, of course, but that would not be very intuitive so I'm not sure if it would even be worth it to have it there.

Gotta run to work now :)
stephen-dahl: > Something like `number | void` leading to `(arg?: number) => AsyncActionThunkAction` would be a possibility, too, of course, but that would not be very intuitive so I'm not sure if it would even be worth it to have it there.

would there be another way to handle `(arg: number | void, thunkApi) => {}`?
`arg` cant be `arg?` in this case we cant have an optional param before a required one.

figuring out types for some of these edge cases looks like a pain 😂 
msutkowski: @phryneas Gotcha. In that case, the only issue I see with the current types is this:

```ts
const usingInline = createAsyncThunk(
  'test/usingInline',
  (arg: string, { getState }) => {
    const { test } = getState() as RootState

    return {
      banana: 'test'
    }
  }
)

usingInline() // i would expect an error being that we say `arg: string`, but this doesn't get caught as the resulting type is `arg?: string`
```

If there is a way to get `arg: string` and `arg?: string` to work as expected, that'd be the ideal thing. If not, I think I would _personally_ prefer the `string | void` to imply an optional argument. The reason being is the typing can be weird. Consider these:

```ts
const usingInline = createAsyncThunk(
  'test/usingInline',
  (arg?: string, { getState }) => { // results in - A required parameter cannot follow an optional parameter.
    const { test } = getState() as RootState

    return {
      banana: 'test'
    }
  }
)
```

```ts
// you can't do `string?` here as the `arg` param, so how would a user accomplish this?
const usingInline = createAsyncThunk<{ banana: string }, string, { state: RootState }>(
  'test/usingInline',
  (arg, { getState }) => {
    const { test } = getState()

    return {
      banana: 'test'
    }
  }
)
```

My only other input is that I don't think `arg` will be typed as optional by a user very often. It seems it is either well-defined or void. I could be wrong here, but that's what I've seen so far in Discord. Thanks!

phryneas: I've added lots of tests for usage with a seconds argument.

The "optional" cases will now be optional with `strictNullChecks: true`, but require `| void` declaration with `strictNullChecks: false` so not all arguments become optional. I believe this is the best compromise we can get.

Have I missed any weird case?
phryneas: Thanks... guess I was tired. Added those, I hopen that were all.
stephen-dahl: thanks @phryneas I think this is the best experience I have had with getting something updated in a project. lgtm
phryneas: @stephen-dahl  I still have to extract some helper types to make #486 less painful, but now with the tests in place & thus the behaviour reviewed from you it's gonna be pretty straightforward - I'll try to finish this up today or tomorrow :)
msutkowski: @phryneas Once #486 is addressed, I'll open a PR for the docs with [common typing recipes](https://gist.github.com/msutkowski/81a69be15e96028c848bf3764334a323), and include an example of typing a generic helper per that thread.  
phryneas: Okay, I've just extracted these types out and exported one of them, `AsyncThunkAction` - that should help make things more readable in tooltips and error messages.

When the tests are finished, I believe this is ready to merge from my side. @markerikson what do you think?
markerikson: Rebased on master after merging #512 .

Patch:
diff --git a/etc/redux-toolkit.api.md b/etc/redux-toolkit.api.md
index 623fa7e1..fedfb949 100644
--- a/etc/redux-toolkit.api.md
+++ b/etc/redux-toolkit.api.md
@@ -59,6 +59,11 @@ export interface ActionReducerMapBuilder<State> {
 // @public @deprecated
 export type Actions<T extends keyof any = string> = Record<T, Action>;
 
+// @public
+export type AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig extends AsyncThunkConfig> = (dispatch: GetDispatch<ThunkApiConfig>, getState: () => GetState<ThunkApiConfig>, extra: GetExtra<ThunkApiConfig>) => Promise<AsyncThunkReturnValue<ThunkArg, Returned, GetRejectValue<ThunkApiConfig>>> & {
+    abort(reason?: string): void;
+};
+
 // @public
 export type AsyncThunkPayloadCreator<Returned, ThunkArg = void, ThunkApiConfig extends AsyncThunkConfig = {}> = (arg: ThunkArg, thunkAPI: GetThunkAPI<ThunkApiConfig>) => AsyncThunkPayloadCreatorReturnValue<Returned, ThunkApiConfig>;
 
@@ -111,16 +116,7 @@ export function createAction<P = void, T extends string = string>(type: T): Payl
 export function createAction<PA extends PrepareAction<any>, T extends string = string>(type: T, prepareAction: PA): PayloadActionCreator<ReturnType<PA>['payload'], T, PA>;
 
 // @public (undocumented)
-export function createAsyncThunk<Returned, ThunkArg = void, ThunkApiConfig extends AsyncThunkConfig = {}>(type: string, payloadCreator: AsyncThunkPayloadCreator<Returned, ThunkArg, ThunkApiConfig>): ((arg: ThunkArg) => (dispatch: GetDispatch<ThunkApiConfig>, getState: () => GetState<ThunkApiConfig>, extra: GetExtra<ThunkApiConfig>) => Promise<PayloadAction<Returned, string, {
-    arg: ThunkArg;
-    requestId: string;
-}, never> | PayloadAction<GetRejectValue<ThunkApiConfig> | undefined, string, {
-    arg: ThunkArg;
-    requestId: string;
-    aborted: boolean;
-}, SerializedError>> & {
-    abort: (reason?: string | undefined) => void;
-}) & {
+export function createAsyncThunk<Returned, ThunkArg = void, ThunkApiConfig extends AsyncThunkConfig = {}>(type: string, payloadCreator: AsyncThunkPayloadCreator<Returned, ThunkArg, ThunkApiConfig>): IsAny<ThunkArg, (arg: ThunkArg) => AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig>, unknown extends ThunkArg ? (arg: ThunkArg) => AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig> : [ThunkArg] extends [void] | [undefined] ? () => AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig> : [void] extends [ThunkArg] ? (arg?: ThunkArg | undefined) => AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig> : [undefined] extends [ThunkArg] ? (arg?: ThunkArg | undefined) => AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig> : (arg: ThunkArg) => AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig>> & {
     pending: ActionCreatorWithPreparedPayload<[string, ThunkArg], undefined, string, never, {
         arg: ThunkArg;
         requestId: string;
diff --git a/src/createAsyncThunk.ts b/src/createAsyncThunk.ts
index facec30e..924826e5 100644
--- a/src/createAsyncThunk.ts
+++ b/src/createAsyncThunk.ts
@@ -5,7 +5,7 @@ import {
   ActionCreatorWithPreparedPayload
 } from './createAction'
 import { ThunkDispatch } from 'redux-thunk'
-import { FallbackIfUnknown } from './tsHelpers'
+import { FallbackIfUnknown, IsAny } from './tsHelpers'
 import { nanoid } from './nanoid'
 
 // @ts-ignore we need the import of these types due to a bundling issue.
@@ -130,6 +130,64 @@ export type AsyncThunkPayloadCreator<
   thunkAPI: GetThunkAPI<ThunkApiConfig>
 ) => AsyncThunkPayloadCreatorReturnValue<Returned, ThunkApiConfig>
 
+type AsyncThunkReturnValue<ThunkArg, FulfilledValue, RejectedValue> =
+  | PayloadAction<FulfilledValue, string, { arg: ThunkArg; requestId: string }>
+  | PayloadAction<
+      undefined | RejectedValue,
+      string,
+      { arg: ThunkArg; requestId: string; aborted: boolean },
+      SerializedError
+    >
+/**
+ * A ThunkAction created by `createAsyncThunk`.
+ * Dispatching it returns a Promise for either a
+ * fulfilled or rejected action.
+ * Also, the returned value contains a `abort()` method
+ * that allows the asyncAction to be cancelled from the outside.
+ *
+ * @public
+ */
+export type AsyncThunkAction<
+  Returned,
+  ThunkArg,
+  ThunkApiConfig extends AsyncThunkConfig
+> = (
+  dispatch: GetDispatch<ThunkApiConfig>,
+  getState: () => GetState<ThunkApiConfig>,
+  extra: GetExtra<ThunkApiConfig>
+) => Promise<
+  AsyncThunkReturnValue<ThunkArg, Returned, GetRejectValue<ThunkApiConfig>>
+> & {
+  abort(reason?: string): void
+}
+
+type AsyncThunkActionCreator<
+  Returned,
+  ThunkArg,
+  ThunkApiConfig extends AsyncThunkConfig
+> = IsAny<
+  ThunkArg,
+  // any handling
+  (arg: ThunkArg) => AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig>,
+  // unknown handling
+  unknown extends ThunkArg
+    ? (arg: ThunkArg) => AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig> // argument not specified or specified as void or undefined
+    : [ThunkArg] extends [void] | [undefined]
+    ? () => AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig> // argument contains void
+    : [void] extends [ThunkArg] // make optional
+    ? (arg?: ThunkArg) => AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig> // argument contains undefined
+    : [undefined] extends [ThunkArg]
+    ? WithStrictNullChecks<
+        // with strict nullChecks: make optional
+        (
+          arg?: ThunkArg
+        ) => AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig>,
+        // without strict null checks this will match everything, so don't make it optional
+        (arg: ThunkArg) => AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig>
+      > // default case: normal argument
+    : (arg: ThunkArg) => AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig>
+>
+
 /**
  *
  * @param type
@@ -216,12 +274,10 @@ If you want to use the AbortController to react to \`abort\` events, please cons
           }
         }
 
-  function actionCreator(arg: ThunkArg) {
-    return (
-      dispatch: GetDispatch<ThunkApiConfig>,
-      getState: () => GetState<ThunkApiConfig>,
-      extra: GetExtra<ThunkApiConfig>
-    ) => {
+  function actionCreator(
+    arg: ThunkArg
+  ): AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig> {
+    return (dispatch, getState, extra) => {
       const requestId = nanoid()
 
       const abortController = new AC()
@@ -277,11 +333,18 @@ If you want to use the AbortController to react to \`abort\` events, please cons
     }
   }
 
-  return Object.assign(actionCreator, {
-    pending,
-    rejected,
-    fulfilled
-  })
+  return Object.assign(
+    actionCreator as AsyncThunkActionCreator<
+      Returned,
+      ThunkArg,
+      ThunkApiConfig
+    >,
+    {
+      pending,
+      rejected,
+      fulfilled
+    }
+  )
 }
 
 type ActionTypesWithOptionalErrorAction =
@@ -304,3 +367,7 @@ export function unwrapResult<R extends ActionTypesWithOptionalErrorAction>(
   }
   return (returned as any).payload
 }
+
+type WithStrictNullChecks<True, False> = undefined extends boolean
+  ? False
+  : True
diff --git a/src/index.ts b/src/index.ts
index 3d1f1527..f4a1843d 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -96,6 +96,7 @@ export {
 } from './entities/models'
 
 export {
+  AsyncThunkAction,
   AsyncThunkPayloadCreatorReturnValue,
   AsyncThunkPayloadCreator,
   createAsyncThunk,


Test patch:
diff --git a/type-tests/files/createAsyncThunk.typetest.ts b/type-tests/files/createAsyncThunk.typetest.ts
index 261f459e..2695d6f7 100644
--- a/type-tests/files/createAsyncThunk.typetest.ts
+++ b/type-tests/files/createAsyncThunk.typetest.ts
@@ -3,7 +3,7 @@ import { ThunkDispatch } from 'redux-thunk'
 import { unwrapResult, SerializedError } from 'src/createAsyncThunk'
 
 import apiRequest, { AxiosError } from 'axios'
-import { IsAny } from 'src/tsHelpers'
+import { IsAny, IsUnknown } from 'src/tsHelpers'
 
 function expectType<T>(t: T) {
   return t
@@ -181,3 +181,164 @@ const defaultDispatch = (() => {}) as ThunkDispatch<{}, any, AnyAction>
     }
   })
 }
+
+/**
+ * payloadCreator first argument type has impact on asyncThunk argument
+ */
+{
+  // no argument: asyncThunk has no argument
+  {
+    const asyncThunk = createAsyncThunk('test', () => 0)
+    expectType<() => any>(asyncThunk)
+    // typings:expect-error cannot be called with an argument
+    asyncThunk(0 as any)
+  }
+
+  // one argument, specified as undefined: asyncThunk has no argument
+  {
+    const asyncThunk = createAsyncThunk('test', (arg: undefined) => 0)
+    expectType<() => any>(asyncThunk)
+    // typings:expect-error cannot be called with an argument
+    asyncThunk(0 as any)
+  }
+
+  // one argument, specified as void: asyncThunk has no argument
+  {
+    const asyncThunk = createAsyncThunk('test', (arg: void) => 0)
+    expectType<() => any>(asyncThunk)
+    // typings:expect-error cannot be called with an argument
+    asyncThunk(0 as any)
+  }
+
+  // one argument, specified as optional number: asyncThunk has optional number argument
+  // this test will fail with strictNullChecks: false, that is to be expected
+  // in that case, we have to forbid this behaviour or it will make arguments optional everywhere
+  {
+    const asyncThunk = createAsyncThunk('test', (arg?: number) => 0)
+    expectType<(arg?: number) => any>(asyncThunk)
+    asyncThunk()
+    asyncThunk(5)
+    // typings:expect-error
+    asyncThunk('string')
+  }
+
+  // one argument, specified as number|undefined: asyncThunk has optional number argument
+  // this test will fail with strictNullChecks: false, that is to be expected
+  // in that case, we have to forbid this behaviour or it will make arguments optional everywhere
+  {
+    const asyncThunk = createAsyncThunk('test', (arg: number | undefined) => 0)
+    expectType<(arg?: number) => any>(asyncThunk)
+    asyncThunk()
+    asyncThunk(5)
+    // typings:expect-error
+    asyncThunk('string')
+  }
+
+  // one argument, specified as number|void: asyncThunk has optional number argument
+  {
+    const asyncThunk = createAsyncThunk('test', (arg: number | void) => 0)
+    expectType<(arg?: number) => any>(asyncThunk)
+    asyncThunk()
+    asyncThunk(5)
+    // typings:expect-error
+    asyncThunk('string')
+  }
+
+  // one argument, specified as any: asyncThunk has required any argument
+  {
+    const asyncThunk = createAsyncThunk('test', (arg: any) => 0)
+    expectType<IsAny<Parameters<typeof asyncThunk>[0], true, false>>(true)
+    asyncThunk(5)
+    // typings:expect-error
+    asyncThunk()
+  }
+
+  // one argument, specified as unknown: asyncThunk has required unknown argument
+  {
+    const asyncThunk = createAsyncThunk('test', (arg: unknown) => 0)
+    expectType<IsUnknown<Parameters<typeof asyncThunk>[0], true, false>>(true)
+    asyncThunk(5)
+    // typings:expect-error
+    asyncThunk()
+  }
+
+  // one argument, specified as number: asyncThunk has required number argument
+  {
+    const asyncThunk = createAsyncThunk('test', (arg: number) => 0)
+    expectType<(arg: number) => any>(asyncThunk)
+    asyncThunk(5)
+    // typings:expect-error
+    asyncThunk()
+  }
+
+  // two arguments, first specified as undefined: asyncThunk has no argument
+  {
+    const asyncThunk = createAsyncThunk('test', (arg: undefined, thunkApi) => 0)
+    expectType<() => any>(asyncThunk)
+    // typings:expect-error cannot be called with an argument
+    asyncThunk(0 as any)
+  }
+
+  // two arguments, first specified as void: asyncThunk has no argument
+  {
+    const asyncThunk = createAsyncThunk('test', (arg: void, thunkApi) => 0)
+    expectType<() => any>(asyncThunk)
+    // typings:expect-error cannot be called with an argument
+    asyncThunk(0 as any)
+  }
+
+  // two arguments, first specified as number|undefined: asyncThunk has optional number argument
+  // this test will fail with strictNullChecks: false, that is to be expected
+  // in that case, we have to forbid this behaviour or it will make arguments optional everywhere
+  {
+    const asyncThunk = createAsyncThunk(
+      'test',
+      (arg: number | undefined, thunkApi) => 0
+    )
+    expectType<(arg?: number) => any>(asyncThunk)
+    asyncThunk()
+    asyncThunk(5)
+    // typings:expect-error
+    asyncThunk('string')
+  }
+
+  // two arguments, first specified as number|void: asyncThunk has optional number argument
+  {
+    const asyncThunk = createAsyncThunk(
+      'test',
+      (arg: number | void, thunkApi) => 0
+    )
+    expectType<(arg?: number) => any>(asyncThunk)
+    asyncThunk()
+    asyncThunk(5)
+    // typings:expect-error
+    asyncThunk('string')
+  }
+
+  // two arguments, first specified as any: asyncThunk has required any argument
+  {
+    const asyncThunk = createAsyncThunk('test', (arg: any, thunkApi) => 0)
+    expectType<IsAny<Parameters<typeof asyncThunk>[0], true, false>>(true)
+    asyncThunk(5)
+    // typings:expect-error
+    asyncThunk()
+  }
+
+  // two arguments, first specified as unknown: asyncThunk has required unknown argument
+  {
+    const asyncThunk = createAsyncThunk('test', (arg: unknown, thunkApi) => 0)
+    expectType<IsUnknown<Parameters<typeof asyncThunk>[0], true, false>>(true)
+    asyncThunk(5)
+    // typings:expect-error
+    asyncThunk()
+  }
+
+  // two arguments, first specified as number: asyncThunk has required number argument
+  {
+    const asyncThunk = createAsyncThunk('test', (arg: number, thunkApi) => 0)
+    expectType<(arg: number) => any>(asyncThunk)
+    asyncThunk(5)
+    // typings:expect-error
+    asyncThunk()
+  }
+}