payloadCreator arg argument => asyncThunk arg type #502

phryneas · 2020-04-13T22:50:12Z

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 · 2020-04-13T22:51:35Z

Deploy preview for redux-starter-kit-docs ready!

Built with commit 380dc1d

https://deploy-preview-502--redux-starter-kit-docs.netlify.com

netlify · 2020-04-13T22:53:00Z

Deploy preview for redux-starter-kit-docs ready!

Built with commit 267274b

https://deploy-preview-502--redux-starter-kit-docs.netlify.app

codesandbox-ci · 2020-04-13T22:54:04Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

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

Sandbox	Source
strange-chatterjee-gfs1g	Configuration
determined-architecture-zkvod	Configuration
blue-wave-y6yk1	Configuration

codesandbox-ci · 2020-04-13T22:55:03Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 267274b:

Sandbox	Source
billowing-wave-2pu87	Configuration
cranky-brattain-ltgeo	Configuration
kind-tharp-z57cf	Configuration

msutkowski · 2020-04-14T04:50:28Z

@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 😅

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 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 · 2020-04-14T07:19:02Z

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

looks like this solves the simpler form of my problem. didn't see any test cases for when the thunkApi is pulled in though. is that case all good also?

stephen-dahl · 2020-04-14T14:40:12Z

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 since we cant have an optional param before a required one.

figuring out types for some of these edge cases looks like a pain 😂

msutkowski · 2020-04-14T14:45:44Z

@phryneas Gotcha. In that case, the only issue I see with the current types is this:

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:

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'
    }
  }
)

// 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 · 2020-04-14T22:11:10Z

I've added lots of tests for usage with a seconds argument. Optional arguments there can now be typed |undefined or |void.

The "optional" or |undefined cases will now be optional with strictNullChecks: true, but require explicit | void declaration with strictNullChecks: false so not all arguments become optional in all cases. I believe this is the best compromise we can get.

Have I missed any weird case?

stephen-dahl

I think you left off thunkApi on your last few tests for that case.

type-tests/files/createAsyncThunk.typetest.ts

phryneas · 2020-04-15T06:18:44Z

Thanks... guess I was tired. Added those, I hope that were all.

stephen-dahl · 2020-04-15T14:24:40Z

thanks @phryneas I think this is the best experience I have had with getting something updated in a project. lgtm

phryneas · 2020-04-15T14:34:14Z

@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

Confirmed all common typing use cases work as expected per https://gist.github.com/msutkowski/81a69be15e96028c848bf3764334a323

msutkowski · 2020-04-15T15:08:20Z

@phryneas Once #486 is addressed, I'll open a PR for the docs with common typing recipes, and include an example of typing a generic helper per that thread.

phryneas · 2020-04-18T21:15:55Z

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 running, I believe this is ready to merge from my side. @markerikson what do you think?

markerikson · 2020-04-19T17:12:11Z

src/createAsyncThunk.ts

+  abort(reason?: string): void
+}
+
+type AsyncThunkActionCreator<


I sure hope you know what you're doing here, because my eyes are glazing over

I added more tests than I added code, guess we'll be fine :)

…trictNullChecks: false

markerikson · 2020-04-19T17:28:52Z

Rebased on master after merging #512 .

phryneas force-pushed the asyncThunkOptionalArg branch from 380dc1d to 47eac02 Compare April 13, 2020 22:51

stephen-dahl reviewed Apr 14, 2020

View reviewed changes

phryneas force-pushed the asyncThunkOptionalArg branch from c2e8ee3 to d9fdd70 Compare April 14, 2020 22:15

stephen-dahl suggested changes Apr 15, 2020

View reviewed changes

type-tests/files/createAsyncThunk.typetest.ts Outdated Show resolved Hide resolved

stephen-dahl approved these changes Apr 15, 2020

View reviewed changes

msutkowski approved these changes Apr 15, 2020

View reviewed changes

phryneas mentioned this pull request Apr 19, 2020

allow to skip AsyncThunks using a condition callback #513

Merged

markerikson reviewed Apr 19, 2020

View reviewed changes

phryneas added 4 commits April 19, 2020 13:20

payloadCreator arg argument => asyncThunk arg type

f2c369d

allow |void typing for optional argument, handle |undefined for s…

62a0c15

…trictNullChecks: false

fix tests

049cf89

extract types, export AsyncThunkAction type

267274b

markerikson force-pushed the asyncThunkOptionalArg branch from 9209999 to 267274b Compare April 19, 2020 17:24

markerikson merged commit 74a0249 into reduxjs:master Apr 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

payloadCreator arg argument => asyncThunk arg type #502

payloadCreator arg argument => asyncThunk arg type #502

phryneas commented Apr 13, 2020 •

edited

Loading

netlify bot commented Apr 13, 2020

netlify bot commented Apr 13, 2020 •

edited

Loading

codesandbox-ci bot commented Apr 13, 2020

codesandbox-ci bot commented Apr 13, 2020 •

edited

Loading

msutkowski commented Apr 14, 2020

phryneas commented Apr 14, 2020

stephen-dahl left a comment

stephen-dahl commented Apr 14, 2020 •

edited

Loading

msutkowski commented Apr 14, 2020

phryneas commented Apr 14, 2020 •

edited

Loading

stephen-dahl left a comment

phryneas commented Apr 15, 2020 •

edited

Loading

stephen-dahl commented Apr 15, 2020

phryneas commented Apr 15, 2020

msutkowski left a comment

msutkowski commented Apr 15, 2020

phryneas commented Apr 18, 2020 •

edited

Loading

markerikson Apr 19, 2020

phryneas Apr 19, 2020

markerikson commented Apr 19, 2020

payloadCreator arg argument => asyncThunk arg type #502

payloadCreator arg argument => asyncThunk arg type #502

Conversation

phryneas commented Apr 13, 2020 • edited Loading

netlify bot commented Apr 13, 2020

netlify bot commented Apr 13, 2020 • edited Loading

codesandbox-ci bot commented Apr 13, 2020

codesandbox-ci bot commented Apr 13, 2020 • edited Loading

msutkowski commented Apr 14, 2020

phryneas commented Apr 14, 2020

stephen-dahl left a comment

Choose a reason for hiding this comment

stephen-dahl commented Apr 14, 2020 • edited Loading

msutkowski commented Apr 14, 2020

phryneas commented Apr 14, 2020 • edited Loading

stephen-dahl left a comment

Choose a reason for hiding this comment

phryneas commented Apr 15, 2020 • edited Loading

stephen-dahl commented Apr 15, 2020

phryneas commented Apr 15, 2020

msutkowski left a comment

Choose a reason for hiding this comment

msutkowski commented Apr 15, 2020

phryneas commented Apr 18, 2020 • edited Loading

markerikson Apr 19, 2020

Choose a reason for hiding this comment

phryneas Apr 19, 2020

Choose a reason for hiding this comment

markerikson commented Apr 19, 2020

phryneas commented Apr 13, 2020 •

edited

Loading

netlify bot commented Apr 13, 2020 •

edited

Loading

codesandbox-ci bot commented Apr 13, 2020 •

edited

Loading

stephen-dahl commented Apr 14, 2020 •

edited

Loading

phryneas commented Apr 14, 2020 •

edited

Loading

phryneas commented Apr 15, 2020 •

edited

Loading

phryneas commented Apr 18, 2020 •

edited

Loading