Redux-toolkit: Updating URL Best Practice

[mattschutt]

Hi, can you provide any guidance for updating a URL after a successful thunk call? It seems like it would be nice if in my
"action.fullfilled" reducer I could just do a history.push there but since reducers aren't supposed to make those kind of changes I'm not sure the best way to handle it.

I could have an action type of "redirect" and set it to true or a url string when I want to redirect but then I'd have to be responsible for making sure I reset the state value after I redirect and that seems like a lot of logic just to handle a simple redirect.

I can't do it by using my 'isSubmitting' state on the form because when it's done the submit may have failed, which puts me back as to the ideal place seems to be in the success handler that is specific to the action I want to redirect.

My use case is filling out a form to add a new item, say a todo, to a list of todos. I'd click 'Add new' from the /list page which directs me the form, then click submit, and on the success fulfill I need to redirect to the /list screen and refresh.

Is there any official guidance on the best way to handle this with redux-toolkit?

Thanks

[mattschutt]

Below is an example of where I think it would be a perfect place to handle.

extraReducers: {
  [create.rejected]: (state, action) => {
    console.error("Create: Rejected");
  },
  [create.fulfilled]: (state, action) => {
    console.log("Create: Success");

    // THIS SEEMS LIKE THE IDEAL PLACE TO DO SOMETHING LIKE history.push
  },
},

[markerikson]

The first couple options would be:

• Wait for the returned promise in the component, and then update the URL once that promise resolves: https://redux.js.org/tutorials/essentials/part-5-async-logic#checking-thunk-results-in-components
• Create a second thunk that dispatches the createAsyncThunk thunk, and then updates the URL once that completes

You could also consider putting it in a middleware of some kind.

But yeah, that can't be done in reducers, as it's a side effect.

[mattschutt]

Do you happen to have any reference code for the second thunk option? Not sure I’m catching what you’re throwing there...

thanks!

[markerikson]

function fetchThenUpdateURL() {
  await dispatch(fetchSomeData());
  history.push("/new-url");
}

function MyComponent() {
  const dispatch = useDispatch();

  const onClick = () => {
     dispatch(fetchThenUpdateURL());
  }
}

The two options I described are basically the same, it's just whether the history update happens in the thunk or the component.

[shinynet]

I've also noticed this same issue. It appears that, some logic I'd have usually handled in a classic thunk, createAsyncThunk requires I move it to the component, which is not desirable to my team. Take, for example, a typical classic thunk

export const fetchEmployees = () => dispatch => {
    // update pending flag
    dispatch(fetchEmployeesRequest())
    http.get('employees')
        .then(result => {
            // update pending flag and data
            dispatch(fetchEmployeesSuccess(result.data))
            return result
        })
        .then(result => {
            // display snackbar
            dispatch(showSnackbar(result.data.message))
            return result
        })
        // update pending flag and error
        .catch(error => {
            dispatch(fetchEmployeesFailure(error.message))
            return error
        })
}

Trying to do the same in createAsyncThunk will result in executing the _showSnackbar_ dispatch before executing the fetchEmployees.fulfilled reducer.

export const fetchEmployees = createAsyncThunk(
    'employees/fetch',
    (_, {dispatch}) => http.get('employees')
            .then(result => {
                // this executes before fetchEmployees.fulfilled reducer!
                dispatch(showSnackbar(result.data.message))
                return result
            })
)

In addition, it's very possible that one may want to transform the _result_ as it makes its way through a chain of .then()'s, and it's the final transformation that is given to the getEmployees.fulfilled's payload, which may be a transformation that's no longer compatible with the fulfilled's payload. This was also never an issue with a classic thunk.

Based on the documentation examples, it appears that RTK expects these "extra" things to be handled in the calling component. While this certainly works, I feel it's forcing an architectural pattern. It's our team's desire to keep components simple and doing just one thing. Having logic like showing a snackbar shouldn't be something a component is concerned with (in our opinions).

The idea of creating a 2nd thunk which calls the first thunk, while that would also work... the end result of doing that results in even more boilerplate code compared to using a classic thunk (which seems to be what RTK is trying to eliminate).

Maybe I'm just missing something (hopefully), but as RTK fans, and excited when given the go-ahead to upgrade from RTK 1.04, we were disappointed when realizing we wouldn't be able to incorporate createAsyncThunk in our refactoring plans.

[markerikson]

To some extent, that _is_ the downside of using an abstraction like createAsyncThunk. You're trading simplification of the most common use cases, for a loss of flexibility in the other 20%.

I'm tentatively open to suggestions on potential tweaks to createAsyncThunk's API, but as currently designed I'm not immediately sure what a viable solution would look like here. About the only thing I could think of is some kind of onSuccess callback in the options argument, and I'm not sure how well that would fit in or what the right semantics should be here.

[shinynet]

About the only thing I could think of is some kind of onSuccess callback in the options argument

That was actually the first thing I looked for, when I first noticed this problem. In addition it would also need an onError callback to handle custom dispatches you wanted to fire after the _rejected_ reducer. Then I guess you'd need an onPending for completeness. In that case, it could be better to have a single callback that is reused for all status and provided with that status.

But I'm with you.. I'm not sure if that's the ideal way it should be solved.

[phryneas]

What about just writing a thunk about both asyncThunks? There is no rule something like that would need to be done in the component.

const fetchEmployeesAndShowSnackbar = () => async dispatch => {
  const result = await dispatch(fetchEmployees()).then(unwrapResult)
  dispatch(showSnackbar(result))  
}

this could easily be abstracted into a wrapWithSnackbar higher order function like

const wrapWithSnackbar = (asyncThunk) => (arg) => async dispatch => {
  const result = await dispatch(asyncThunk(arg)).then(unwrapResult)
  dispatch(showSnackbar(result))  
}

to be used like

  const myAsyncThunk = createAsyncThunk(...)
  export const myAsyncThunkWithSnackbar = wrapWithSnackbar(myAsyncThunk)

[shinynet]

Yeah, that's, essentially, an option that markerikson suggested. The main issue with that is... we go from having a single classic thunk function, which does everything we needed, to having two thunks to pull off the same thing. If anything, we'd probably be increasing boiler-plate. Not to mention, I could see that making things a tad more difficult to reason about.

If it were between that or having the calling component deal with the additional steps, I'd probably lean towards the calling component.

[markerikson]

I could potentially see some sort of a onCompleted callback option, which could receive something like (finalAction, thunkApi) as its arguments?

const fetchUserById = createAsyncThunk(
  'users/fetchByIdStatus',
  async (userId, thunkAPI) => {
    const response = await userAPI.fetchById(userId)
    return response.data
  },
  {
    onCompleted: async (finalAction, thunkApi) => {
      const data = await unwrapResult(finalAction);
      dispatch(someOtherAction(data.something));
    }
  }
)

Glancing at the code, it looks like something along those lines could get squeezed in around here:

https://github.com/reduxjs/redux-toolkit/blob/a25356bd65a68e774e623c33ff3685cb59e644c2/src/createAsyncThunk.ts#L462-L463

Not saying this is all necessarily a _good_ idea, just trying to see what might be feasible given the constraints.

[phryneas]

The question is just if this is even shorter adding such a callback.

I mean, compare the solutions. From a "code amount" position, there isn't much to be gained by moving this into cAT:

-const fetchUserById = createAsyncThunk(
+const _fetchUserById = createAsyncThunk(
  'users/fetchByIdStatus',
  async (userId, thunkAPI) => {
    const response = await userAPI.fetchById(userId)
    return response.data
  },
-  {
-    onCompleted: async (finalAction, thunkApi) => {
-      const data = await unwrapResult(finalAction);
-      dispatch(someOtherAction(data.something));
-    }
-  }
)
+ const fetchUserById = arg => async dispatch => {
+   const result = await dispatch(_fetchUserById(arg));
+   dispatch(someOtherAction(unwrapResult(result).something));
+   return result
+ }

Moving this into cAT also opens up a number of new questions like how to handle errors in onCompleted or if the promise should resolve before or after onCompleted, which I'm not really sure we want to tackle.

On a conceptual side it seems to kinda attach additional stuff into a (currently) single-purpose tool: wrapping something asynchronous into lifecycle events.

[shinynet]

I could potentially see some sort of a onCompleted callback option

While I don't think that's all that bad, and could get behind it, if I had to play devil's advocate, I could say that mixing promises together with callbacks is a strange thing to do.

From a "code amount" position, there isn't much to be gained

I'm trying to look at it beyond just the total number of characters. To be honest, when I first played around with porting a regular thunk to a createAsyncThunk, I didn't even really notice that much improvement in the amount of code. It's really more just moving code around and making it more structured. Existing reducers moved to extraReducers. The createAsyncThunk did remove a bit of branching logic that existed in the regular thunk, but not much more.

I've always liked thunks because it made for an easy wrapper around a unit of code. I think of them a lot like transactions in the rdbms world (and I think a lot of people like them for the same reason). When you split that up into thunks calling thunks, while that would make your code a bit more flexible and re-usable (which only matters if sub-chunks need to be re-used), you do lose a bit of readability. It wouldn't take much to fall into the promise-hell version of callback-hell.

I'm not sure I even think this is even something that really needs solved. While it'd be nice to have an elegant solution that'd work with most use-cases, sometimes the solution might just be to not use createAsyncThunk.

[phryneas]

I'm trying to look at it beyond just the total number of characters. To be honest, when I first played around with porting a regular thunk to a createAsyncThunk, I didn't even really notice that much improvement in the amount of code. It's really more just moving code around and making it more structured. Existing reducers moved to extraReducers. The createAsyncThunk did remove a bit of branching logic that existed in the regular thunk, but not much more.

Well, looking purely at the "code size" aspect, the biggest difference of cAT vs plain is probably that you don't need to define the actions and don't need to dispatch them.

But this goes a bit further. Did you notice the bugs in your fetchEmployees implementation? Most people wouldn't. But they are there.

You are essentially wrapping a try...catch around an asynchronically called dispatch. And this could have weird consequences.
This asynchronically called dispatch will result in all errors thrown in your reducers, selectors and even your components (those will be invoked synchronically in this case) being caught by your .catch - and in another action to be dispatched.

So, worst case for your code:
dispatch(fetchEmployeesSuccess()) -> render with thrown error -> dispatch(fetchEmployeesFailure())

(yes, there is the more obvious case with an error thrown somewhere during dispatch(showSnackbar(result.data.message)), which I skipped over here)

=> both the success and error action are dispatched.

This could be avoided by rewriting your code:

export const fetchEmployees = () => dispatch => {
    // update pending flag
    dispatch(fetchEmployeesRequest())
    http.get('employees')
        .then(
          result => {
            // update pending flag and data
            dispatch(fetchEmployeesSuccess(result.data))
            return result
          },
          error => {
            // update pending flag and error
            dispatch(fetchEmployeesFailure(error.message))
            return error
          }
        )
        .then(result => {
            // display snackbar
            dispatch(showSnackbar(result.data.message))
            return result
        })
}

But who really does that? Nobody. Because the other logic seems more natural and nobody uses the .then(successHandler, errorHandler) notation nowadays.

createAsyncThunk handles stuff like this for you without you having to care about this. Also, it handles serialization of error messages and whatnot. This is just all edge-case stuff, many people don't take into consideration. But nonetheless, it's very important. And the abstraction helps here.

[shinynet]

Well, looking purely at the "code size" aspect, the biggest difference of cAT vs plain is probably that you don't need to define the actions and don't need to dispatch them.

That was kind of my point. It eliminates the branching logic of when to do these these three things
dispatch(fetchEmployeesRequest())
dispatch(fetchEmployeesSuccess(data.data))
dispatch(fetchEmployeesFailure(message))

While I'll take what I can get, that's not really a huge improvement in my mind (again just talking in the context of total amount of code). Which is why I see the real benefit of createAsyncThunk as giving more "structure" to the code (which helps solve the other points in your post).

And yes, I'm aware that my example code isn't protecting from potential errors thrown outside the async chain. Had I posted real code, rather than pseudo code, the usage of async functional algebraic types would have distracted from the point I was trying to make :)

[markerikson]

I think the conclusion here is that we aren't going to make any changes for now, so I'll close this. Open to further discussion on the topic, though.