RFC: Type pinning and partial inference for reateAsyncThunks

[garronej]

Hello @markerikson and @phryneas,

This proposal suggests that typescript users may get the createAsyncThunks function via a factory to improve DX and code readability.

Benefits

• It encapsulates the AsyncThunkConfig, users get a createAsyncThunk function specific for their store.
• Users only have to explicitly provide the type annotations that can't be inferred, namely
  • The argument of the thunk.
  • (Optionally) specific expected type for rejectValue, serializedErrorType , pendingMeta, fulfilledMeta and rejectedMeta.
• It couples the name of the function and the last portion of the typePrefix. (It relies on template literal types)

Playground

Code sandbox link

• counter_vanilla.ts: State of the art with the regular createAsyncThunks.
• counter.ts: Use of the alternative createAsyncThuks that is the object of this proposal.
• setup.ts: Where createStore() is supposed to be called.
• createAsyncThunks.ts: File that export the createAsyncThuks returned by our factory function.
• RTK-ext.ts: Implementation of the factory function.

Screen.Recording.2022-02-02.at.00.41.29.mov

Drawbacks

• Users must call the factory function in a different file than the one where createStore() is called or they will get require
  cycles. They should also use import type to import their AsyncThunkConfig type.
• Template literal types aren't backward compatible. This utility, as it is, couldn't be indexed. It would have to be available under @redux/toolkit/factories for example.
• Having to provide an extra () is what unlocks partial argument inference but it will look a bit weird, especially to vanilla JavaScript users. That said, JavaScript users wouldn't have to know about this factory. They could keep using the createAsyncThunks exported from @redux/toolkit as they would see no benefit using the factory.

Available implementation

createAsyncThunksFactory is already available in redux-clean-architecture and here is an example DIFF induced by using this proposal.

---

We started from the wrong foot on reddit I would understand if you are not interested in me contributing any further but I you are interested I can come up with a PR.

Best regards,

[markerikson]

Hey, thank you for taking the time to write this up!

My first couple thoughts just glancing at this:

• In some ways this is similar to the "pre-typed hooks" concept we teach in the docs, so I can see some precedent there
• Is the return : { [thunkName] : thunk } part critical, or is it more intended as a convenience for naming/merging into another object? I generally haven't seen that used as a pattern for returning a single item before
• There's a prior PR at RFC add store.withCurriedTypes helper #684 that was attempting to do something sorta-vaguely similar, but more focused on useSelector/useDispatch. I'm not sure if there's enough overlap that something could be shared, but it's relevant.

[phryneas]

Personally, I'd prefer to add an api like

const cATwithTypes = createAsyncThunk.pinTypes<{ state: RootState, dispatch: AppDispatch }>()

which could either be further refined

const withStringReject = cATwithTypes .pinTypes<{ rejectValue: string }>()

or implicitly be used with different fallbacks:

const myThunk = cATwithTypes<string, string, { rejectValue: string }>( 
    'someUsage',
    (arg, { getState, rejectWithValue}) => {
      // ...
    }
 )

that would keep the ()( call out - because that one is one that will cause endless confusion. A lot of our users are following video tutorials and would even miss that visually - also it is very difficult to actually convey the need for that to someone who is just learning JS/TS - and most advanced users probably as well.

The "naming the returned thunk on an object" part is one I'm not particularly fond of to be honest. There is no real requirement that a thunk type prefix is 'slice/name', it could also be "FoO_UUU" or just "🙃🙃🙃". The latter would not even result in a valid variable name and lead to very irritating destructuring.
Also, while we will soon drop support for pre-4.1 TS this would add a needless breaking change on that front. Tbh., I'd just let people choose their variable names as they make sense for them.

PS: mind that both your and my suggestion above can in many situations introduce circular type references, since the slice depends on the thunk (and TS often assumes that the type of the slice also depends on the type of the thunk, even if that's not the case), so the type of the RootState depends on the thunk and vice versa in the end, unless you manually break the circle e.g. by export default slice.reducer as Reducer<typeof initialState>.

PPS: those circularities are hard to construct when you want to show one. Here you have an example: https://codesandbox.io/s/agitated-lumiere-pl659?file=/src/counter.ts
Note that in this specific case, the problem goes away by just adding a method body and not returning anything in the extraReducers, but we also had lots of cases where that was not enough and real "manual type breaking" was necessary. The example right now just goes to show that there are many cases where we not only have to worry about runtime circularity, but about type circularity as well.
If you want to see a clusterfuck of circular thunk types, you can take a look at #637 - note that there we also used a builder pattern, which is similar to your ()( but gives a little more legibility to the user.

[garronej]

Hi @markerikson,
Thank you for your quick answer.

Is the return : { [thunkName] : thunk } part critical, or is it more intended as a convenience for naming/merging into another
object? I generally haven't seen that used as a pattern for returning a single item before.

No, it's not critical. And it's not only for convenience, it's for dryness. There is no requirement for the pathPrefix to match the name of the function but generally it's a rule of thumbs.
Returning the function wrapped makes sure this rule is always enforced.
And no, it's not common practice but it's something I am pushing. Having to find names for things is hard, often there is an obvious good candidate. Returning the function wrapped enables to:

1. Avoid users for going to the documentation to see how things are supposed to be named, they just have to hit Ctrl+space.
2. It helps with consistency.
   I guess it’s analogous to the question: "Should we use named export or export default when we have only one export in a file?" In my opinion, we should always use named export and never export default but not everyone agrees.
   I have received a couple of emails from users explicitly so say that they love this pattern but to be fair I also received some backlash from users considering that wrapping should only be when there is more than one value returned.

There's a prior PR at RFC add store.withCurriedTypes helper #684 that was attempting to do something sorta-vaguely
similar, but more focused on useSelector/useDispatch. I'm not sure if there's enough overlap that something could be
shared, but it's relevant.

Yes, it's similar but it couldn't work for createAsyncThunk because of require cycles. It can't come from the store.

[phryneas]

Yes, it's similar but it couldn't work for createAsyncThunk because of require cycles. It can't come from the store.

That part can actually be fine, depending on how you structure your files. (if you have the thunk in a different file from the slice)
But generally I'd prefer the approach I outlined above.

[garronej]

Hi @phryneas,

that would keep the ()( call out - because that one is one that will cause endless confusion. A lot of our users are following
video tutorials and would even miss that visually - also it is very difficult to actually convey the need for that to someone
who is just learning JS/TS - and most advanced users probably as well.

Fairly good points. pinTypes would make it look less cryptic to beginners while still allowing for this kind of pattern:

createAsyncThunk.ts

import { createAsyncThunk as createAsyncThunk_base } from "@redux/toolkit";
import type { AsyncThunkConfig } from "./store";

export const createAsyncThunk = createAsyncThunk_base.pinTypes<AsyncThunkConfig>();

counter.ts

import { createAsyncThunk } from "../createAsyncThunk";

export const incrementAsync = createAsyncThunk.pinTypes<{ rejectValue: number }>()(
  `${name}/incrementAsync`,
  async (amount: number, { extra, rejectWithValue }) => {
    const { data } = await extra.counterApi.fetchCount(amount);

    if (data < 0) {
      rejectWithValue(data);
    }

    return data;
  }
);

In general, I tend to favour the factory pattern because I don't like having to come up with new names.
createAsyncThunk -> cATwithTypes -> withStringReject

It's like do I do:

import { useDispach } from "react-redux";

export const useAppDispatch = useDispatch<AppDispatch>();

or:

import { useDispach as useDispatch_base } from "react-redux";

export const useDispatch = useDispatch_base<AppDispatch>();

I would rather have:

import { createUseDispatch } from "react-redux";

export const { useDispatch } = createUseDispatch<AppDispatch>();

But again, that's a matter of personal preference and I perfectly understand going for a more beginner friendly approach.

The "naming the returned thunk on an object" part is one I'm not particularly fond of to be honest. There is no real
requirement that a thunk type prefix is 'slice/name', it could also be "FoO_UUU" or just "🙃🙃🙃". The latter would not
even result in a valid variable name and lead to very irritating destructuring.

I think RTK being opinionated it wouldn't be a bad thing to make it a little bit harder to use smiley in the typePrefix but the opinion is yours, not mine.

mind that both your and my suggestion above can in many situations introduce circular type references.

I don't think you are correct on this particular point.. Neither your proposal nor mine introduce any new require circle as long as we don't export the createAsyncThunks with the pinned types from the file in which configureStore is called.

setup.ts → import type { AsyncThunkConfig } → createAsyncThunk.ts → import { createAsyncThunk } → counter.ts
Adds one link to the chain compared to:
setup.ts → import type { AsyncThunkConfig } → counter.ts
But does not induce new require cycles considering that createAsyncThunk.ts would import nothing extra but import { createAsyncThunk } from "@redux/toolkit".

The example right now just goes to show that there are many cases where we not only have to worry about runtime
circularity, but about type circularity as well.

Yes, you are 100% correct.

note that there we also used a builder pattern, which is similar to your ()(

When I said "Just one that knows better". I meant compared to the average RTK user. I am well aware that you guys knows every tick of the book already.

[phryneas]

But does not induce new require cycles considering that createAsyncThunk.ts would import nothing extra but import { createAsyncThunk } from "@redux/toolkit".

The cycle I'm hinting at is introduced by adding RootState to a asyncThunk definition and then adding that thunk to extraReducers, which makes the thunk depend on the store type and the store on the thunk type.

It doesn't happen always (as you were seeing, adding a method body can resolve that in some cases) and it did happen before, too.
The difference is that with a helper like that, we would show adding the state type to the thunk per part of the "default RTK setup", making those problems much more likely to occur.
Before, it would only occur for people who actually were using getState() using the generic, after this change there would be a chance of this occuring for everyone.

So it's not about having the tool per se, it's also about how we go teaching it afterwards. But adding the tool without teaching it is also quite pointless.

[garronej]

Okay I get it. Now we are on the same page.
It is true that that type requires cycles are particularly tricky to debug.
RegardingextraReducers, wouldn't it worth it to make the builder API the only option? I see that it's already the option that you recommend.
I played around a little bit to see what was possible to do with an untyped createAsyncThunk. It is true that, if you don’t need the states and don't dispatch other thunks you can get things done.
To me the pros of implementing this RFC still outweigh the cons as the compromise is done to the detriment of power-users but I understand the reasoning behind you not wanting to go forward with this.

[markerikson]

Regarding extraReducers, wouldn't it worth it to make the builder API the only option? I see that it's already the option that you recommend.

I am actually seriously considering this for a future RTK 2.0, yeah:

#958 (comment)

Being able to define extraReducers as an object (and the implicit stringification of action creators) was a neat trick that maybe helped with adoption, but the builder syntax only requires changing a few lines of code and is guaranteed to have proper types.

(I just tried to put up a Twitter poll asking which form people use, but Twitter isn't letting me post it atm. Will try again later.)

[markerikson]

We implemented something akin to this in 1.9.0 and https://github.com/reduxjs/redux-toolkit/releases/tag/v1.9.1 .

[garronej]

@markerikson Thanks for the update :)