allow definition of async thunks from createSlice

[phryneas]

This is still a WIP, but it's getting complete enough to collect some feedback.

createSlice itself started to become quite complex, so I did some refactoring & split the reducer handling out.

[phryneas]

If this is not possible (which I'm not sure about), it would not be possible to test the result of the builder notation if the payload of reducers created with the prepare notation would be correct between the reducer & the prepare function.

=> Maybe we should forbid that definition here and add another withSpecial.reducer function for usage with the builder notation? Would be more "in theme" anyways.

[phryneas]

I've added create.preparedReducer as a new method and throw an error if the prepare notation is used without that in combination with the create notation, as in that case we cannot correctly check if the payload returned from prepare is the same payload as expected by the reducer.
It's unfortunate, but when I try, the inference breaks in all directions due to the type being inferred from a method return value in the first place.

On the other hand, it's probably more consistent anyways.

[netlify]

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

Built with commit df00e1e

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

[codesandbox-ci]

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 df00e1e:

Sandbox	Source
Vanilla	Configuration
Vanilla Typescript	Configuration
rsk-github-issues-example	Configuration

[markerikson]

Just to check... were we going to try to make it asyncThunks to allow multiple entries with only having to specify the state/extra types once?

Also, what's the need for having the payload arg defined separately?

Any chance something like this would work?

builder.addAsyncThunks<State, Extra, Whatever>([
  {payload, pending, rejected, fulfilled},
  {payload, pending, rejected, fulfilled},
])

[phryneas]

Something like

  const slice = createSlice({
    name: 'test',
    initialState: {} as TestState,
    reducers: withSpecial => ({
      ...withSpecial.asyncThunks<RootState, Extra, Whatever>({
        thunk1Name: {payloadCreator, pending, rejected, fulfilled, options},
        thunk2Name: {payloadCreator, pending, rejected, fulfilled, options},
      }),

could potentially work, but type inference from the payloadCreator to the pending/rejected/fulfilled would be very flimsy - the prepare notation at the moment often requires manual type annotation and only checks that nothing wrong was annotated; we'd probably be running into similar problems and very complex types that way.

Also, what's the need for having the payload arg defined separately?

The splitting up was necessary as otherwise in fulfilledReducer(state, action), action.payload would always infer to unknown and I didn't find another way around that.

[phryneas]

PS: also, the Rejected value might be different between the thunks, making this even more difficult, as that can never be inferred.

[phryneas]

Okay, aside from this breaking in older TS versions, I think this is pretty much where I would want this to go.

@markerikson & @msutkowski, want to discuss this some more before I start to clean this up? :)

[markerikson]

Okay, so, lemme see if I have this usage right:

createSlice({
  name,
  initialState,
  reducers: create => ({ // callback form
    fetchTodos: create.asyncThunk(
      todosAPI.fetchTodos,
      {pending, fulfilled, rejected} // thunk reducers here
    ),
    toggleTodo(state, action) { // "normal" reducer in this object
      state[index].completed = !state[index].completed;
    },
    addTodo: create.preparedReducer({
      prepare: (text) => ({payload: {text, id: nanoid()}}),
      reducer: (state, action) => void state.push(action.payload);
    })
  })
})

So:

• normal reducers are still inline, it's just that we're returning an object from this callback
• Unlike extraReducers, you do have to return something here (the object full of reducers)
• create.asyncThunk appears to take the payload arg separate from an object that contains any combination of the pending/fulfilled/rejected reducers + the options field
• Prepared reducers need create.preparedReducer when used with the callback form

I think my biggest questions are around naming and docs / teaching. What do we call this form? How do we teach it? How do we differentiate the behavior and naming between this and the "builder callback" form?

(Oh, and I do really like the create name, btw.)

Great job as always, Lenz!

[msutkowski]

@markerikson @phryneas I went ahead and just tried it out on that generic CRUD entity adapter thing as a quick demo for usage - https://codesandbox.io/s/create-asyncthunk-ex-y9n03?file=/src/crud-factory.ts

[phryneas]

Okay, so, lemme see if I have this usage right:

...

Almost, it is:

createSlice({
  name,
  initialState,
  reducers: create => ({ // callback form
    fetchTodos: create.asyncThunk(
      todosAPI.fetchTodos,
      {pending, fulfilled, rejected} // thunk reducers here
    ),
    toggleTodo(state, action) { // "normal" reducer in this object
      state[index].completed = !state[index].completed;
    },
-    addTodo: create.preparedReducer({
-      prepare: (text) => ({payload: {text, id: nanoid()}}),
-      reducer: (state, action) => void state.push(action.payload);
-    })
+    addTodo: create.preparedReducer(
+      (text) => ({payload: {text, id: nanoid()}}),
+      (state, action) => void state.push(action.payload);
+    )
  })
})

As inferring a type from one argument into a second is a lot more simple than doing so within one object argument. Also, IDE type hinting seems to work better that way. (Same reason why we have that split in create.asyncThunk)

That said: it might be easier here than elsewhere. If you prefer that other notation, I can give it a try.

* normal reducers are still inline, it's just that we're returning an object from this callback

* Unlike `extraReducers`, you _do_ have to return something here (the object full of reducers)

* `create.asyncThunk` appears to take the payload arg separate from an object that contains any combination of the `pending/fulfilled/rejected` reducers + the `options` field

* Prepared reducers need `create.preparedReducer` when used with the callback form

exactly 👍

I think my biggest questions are around naming and docs / teaching. What do we call this form? How do we teach it? How do we differentiate the behavior and naming between this and the "builder callback" form?

Yeah. Writing/adjusting the docs for this is my personal nightmare to be honest 🙁

(Oh, and I do really like the create name, btw.)

nice 😄

Great job as always, Lenz!

🙃

[phryneas]

Taking a break for now.

I've added create.reducer, as specifying a normal reducer was breaking inference of the State type in older TS versions. Also, it's more consistent this way.

Stuff I am still concerned with:

• there is a degradation of error quality in older TS versions. While in TS3.8, we get an error like this (as expected)
  , in TS 3.7 we are getting this:
  
  That is probably not going away and we'll need to decide if this feature is worth that degradation in older TS versions for us. If it is, I'll still have to tweak a few tests.

• while I am now enforcing that normal map object definitions cannot be mixed with definitions created by create.foo, I can not enforce that the create callback is returning only an object of definitions. So a developer could also do create => pure map object.
  This has no runtime consequences but might lead to a very subpar TS experience, as in that case the State type will break in older TS versions.
  This is similar to the problems we had with the builder notation and the plain prepare definition. In that case, type checks were just not stringent enough, so I'm throwing a runtime error if I see that this is used.
  We could generalize that check to catch this whole thing, but it would still be confusing at a TS level and there's not much to be done about that.

[nathggns]

Random question: will it possible to create a action creator thunk without using createAsync? There are cases where you don't want all of the pending/rejected/fulfilled actions, but you do want to be able to arbitrarily dispatch actions within an asynchronous process.

[markerikson]

@nathggns you've always been able to define thunks outside of createSlice, whether they're handwritten (sync or async) or using createAsyncThunk.

If a handwritten thunk is defined outside, then it can use action creators that were generated inside createSlice, like:

const todosSlice = createSlice({
  name: "todos",
  initialState: [],
  reducers: {
    fetchTodosStarted(state, action) {},
    fetchTodosFailed(state, action) {},
    fetchTodosSucceeeded(state, action) {
      return action.payload
    },

  }
});

export const {fetchTodoStarted, fetchTodosFailed, fetchTodosSucceeded} = todosSlice.actions;

export default todosSlice.reducer;

export const fetchTodos = () => async dispatch => {
  dispatch(fetchTodosStarted());
  try {
    const response =  await todosAPI.getAll();
    dispatch(fetchTodosSucceeded(response.todos));
  } catch (err) {
    dispatch(fetchTodosFailed(err));
  }
}

The problem is that createAsyncThunk generates its own pending/fulfilled/rejected action types, which means that:

• you have to use extraReducers to handle those actions, since they weren't generated inside of createSlice
• you have to hand-write the base action type string, which often overlaps with the name field from the slice

So we specifically want to add syntax to support the case of defining thunks with createAsyncThunk inside of createSlice, but that won't prevent any of the existing patterns from working.

[markerikson]

@phryneas so where does this stand atm? I'd really like to push this through in the near future.

[phryneas]

Yeah, I just pushed https://github.com/reduxjs/redux-toolkit/commit/df00e1ea24709faf304932909f7f0159ef7f0002 which showcases a circular type reference problem and the solution.

Please take a look. If we decide that we can tackle that in documentation, good. If not - I'm not sure if we can merge this feature at all then.

[msutkowski]

I have a few thoughts here that ultimately boil down to: this is a good API for JS users for sure, but the additional overhead/downsides for TS users would make me question recommending it.

Downsides:

• RFC add store.withCurriedTypes helper #684 offers a way to easily bind types to createAsyncThunk but unfortunately would not offer any help with this syntax
• The create.asyncThunk syntax still requires typing the ThunkAPI, which is solved by ☝️ . So it would be weird to have one experience when using createAsyncThunk, then another when using create.asyncThunk
• Casting the reducer for slices that leverage this feature seems like it could be confusing, even with good documentation.

We could possibly get around those downsides by explicitly not recommending that syntax for TS users, but I'm not sure that's a good idea. We put notices about the Usage with TypeScript guide everywhere and lots of folks still skip that.

[markerikson]

I really, really want to be able to define thunks inside of createSlice, somehow. It's too obvious a thing to do to ignore it, and having to define thunks outside of createSlice is annoying.

Unfortunately, as we're seeing here, the issue is TS definitions vs trying to infer things. If the RootState is explicitly defined, this can work, but we really want folks to infer RootState instead, and that gets us into trouble here.

I don't have any suggestions atm, but I sure hope we can figure something out... :(

[phryneas]

Well, I have to say that I would really like to get this out as well - funny enough, not even necessarily for the asyncThunk bit, but for the prepareReducer bit.
While the current prepare notation works, it requires both the prepare arguments and the action to be typed. The first causes some trouble with pure JavaScript, where that cannot be typed without a JSDoc comment as showcased by #680 - the second should just be inferrable from the prepare return value, but in the current notation all we can do is warn if they are not compatible - very dissatisfying.

So I believe it comes down to a matter of teaching. If we keep the pattern export const reducer: Reducer<SliceState> = slice.reducer during the whole documentation, this can be hopefully give enough of an example that most people do that from the start - and for the rest, we can keep a FAQ entry.
Also, this does only affect the people that use create.asyncThunk in the first place and try to use getState, which probably is a minority in the first place (although going for this argument is somewhat dissatistfying).

One point though: I would like to suggest that we do this in a major release and get some other stuff in (and some out).

My suggestions would be:

• remove (or at least deprecate) the existing prepare notation. It is a hassle with JS and TS and we can provide a codemod for this transistion. Also, removing that will cut down on the type complexity of createSlice quite a lot.
• deprecate the extraReducers map notation in favor of the builder notation. Realistically, not only TS users benefit from that, but also JS users will get a lot better autocomplete. Also, having only one of these notations in the documentation will be far less confusing. Again, we can offer a codemod for that.
• add a createThunk helper that does nothing but attach types to things, like suggested in RFC add store.withCurriedTypes helper #684. My obvervation from reactiflux is that having createAsyncThunk, but not createThunk makes people forget (or never learn) that they could just write normal thunks as well. Also, people find the weirdest ways to type their thunks and have quite many problems with that, so having a helper for that migh be of benefit for that use case.
• add a type currying helper like suggested in RFC add store.withCurriedTypes helper #684. This will help TS users correctly type their stuff and will lead to one common pattern for a correct type of useSelector, useDispatch, createAsyncThunk (if used outside of createSlice) and we can easily extend that for reselect's types as well. Name to be discussed, I believe Matt and I preferred const addTypesTo = bindStoreType<Store>() in the end.
• move the RejectValue value generic out of AsyncThunkConfig. This would make it possible for people to define their AppThunkConfig type and use it in all create.asyncThunk calls, regardless of their rejected value
• evaluate the possibility of adding selectors back into createSlice. These could be slice-centered selectors and in the end, an api like in createEntityAdapter could be used, so export selectors = slice.getSelectors(rootState => rootState.slice)
• re-evaluate rejectWithValue, maybe come up for a replacement that addresses issues we have seen over time come up in reactiflux.

This will also require quite some work on the documentation as well, so it would not be a "release tomorrow" thing, but I believe we could get this done in... dunno, two months? We could also target the birthday of RTK as a release for 2.0 ;)

I could also try a few of my friends and colleagues if one of them would be willing to pair up on some of the documentation stuff, that could help things go forward, we can probably need a few more hands for that.

[markerikson]

Hmm. Lot of stuff to unpack there.

Quick thoughts:

• Agreed that prepare adds type complexity. On the other hand, this sounds like you'd have to switch over to the callback form just to add a prepare callback?
• Agreed on the extraReducers notation. The "action creator as key" thing is neat, but too many folks are running into trouble there.
• Not sure where you're going with the export const reducer: Reducer<SliceState> = slice.reducer thing, on two levels. Why is that extra typedef necessary? Also, we've generally shown export default slice.reducer.
• Not completely against the selectors bit, and the addition of createEntityAdapter.getSelectors has shown a useful pattern. But, what specific benefit are we getting out of defining them inside of createSlice?
• This means we'd have to do a lot of docs work:
  • Already been talking about rewriting the RTK tutorials anyway, and I still don't know exactly what they should cover anyway.
  • Rewriting the API docs and usage guides
  • Updating the "Redux Essentials" core docs tutorial
• There's also the as-yet-unreleased Redux 5.0 rewrite in TS, which likely has some changes in typedefs that might necessitate a major release anyway.

So... I kinda see where you're going with most of this, but that's a pretty big list of changes :)

[phryneas]

Hmm. Lot of stuff to unpack there.

Quick thoughts:

• Agreed that prepare adds type complexity. On the other hand, this sounds like you'd have to switch over to the callback form just to add a prepare callback?

Yes, that would be the downside to that. Personally, I believe this would be worth it - stuff like #680 is just too obscure and I'd really root that out.

• Agreed on the extraReducers notation. The "action creator as key" thing is neat, but too many folks are running into trouble there.

• Not sure where you're going with the export const reducer: Reducer<SliceState> = slice.reducer thing, on two levels. Why is that extra typedef necessary? Also, we've generally shown export default slice.reducer.

It's the definitive way to prevent that circular import problem when using create.asyncThunk. I don't believe it's a good idea to have a "if you're using create.asyncThunk with the ThunkAPI argument, please do this" in the docs. I'd just show the pattern everywhere to prevent the confusion from the start.
Of course, it could also be export default slice.reducer as Reducer<SliceState>, but a as is always a stronger cast than a variable type, so I'd try to avoid that if I have the chance.

• Not completely against the selectors bit, and the addition of createEntityAdapter.getSelectors has shown a useful pattern. But, what specific benefit are we getting out of defining them inside of createSlice?

Primarily: teaching. It's a bit like the createThunk thing. People don't use stuff that doesn't danlge in front of their nose and the number of times I've had to link the "derived state" part in the redux docs in the last two months is absurd. Having that more out in the open there might help.

• This means we'd have to do a lot of docs work:

  • Already been talking about rewriting the RTK tutorials anyway, and I still don't know exactly what they should cover anyway.

  • Rewriting the API docs and usage guides

  • Updating the "Redux Essentials" core docs tutorial

  • There's also the as-yet-unreleased Redux 5.0 rewrite in TS, which likely has some changes in typedefs that might necessitate a major release anyway.

Yup :/ But it's kinda getting inevitable to do a partial docs revamp at this point.

So... I kinda see where you're going with most of this, but that's a pretty big list of changes :)

Yup, but on the other hand: if we're doing a major release (and we should, we are getting quite far away from the initial 1.0 anyways), it's gotta have some goodies ;)

By the way: the person I had in mind for helping me is up for it, so I have someone poking me in real life to do something, which always helps ^^

[phryneas]

Of course we can always decide to cut down on the scope or just release some minor things while delaying a major version, but I'd suggest planning kinda big for now :)

[msutkowski]

To add to that list when re-evaluating rejectWithValue, we'd want to address unwrapResult and how it behaves. I know the format of the error payload and unwrapping behavior throws everyone off.

[phryneas]

To add to that list when re-evaluating rejectWithValue, we'd want to address unwrapResult and how it behaves. I know the format of the error payload and unwrapping behavior throws everyone off.

Umh yeah, I meant to write unwrapResult there :D

But yes, we should also take another stab at rejectWithValue itself. Maybe it doesn't need to be typed at all and the rejection type could just be inferred from the return value. That would solve a lot of headaches.

[markerikson]

This seems to be A) unworkable, and B) semi-obsolete due to the upcoming RTK Query addon.

As much as it'd still be neat to have this, it doesn't seem like that's going to happen, so I'm going to close the PR.