Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accessing the original error in createAsyncThunk #390

Closed
msutkowski opened this issue Feb 24, 2020 · 18 comments
Closed

Accessing the original error in createAsyncThunk #390

msutkowski opened this issue Feb 24, 2020 · 18 comments

Comments

@msutkowski
Copy link
Member

With the current implementation of createAsyncThunk, there is no way to access any errors returned from a server. Here is a specific scenario which I imagine is extremely common:

You attempt a form submission with a set of data like POST /users with a body of { first_name: 'valid', last_name: 'valid', email: 'in@valid' } and the API responds with a 400:

{
    "details": [
        {
            "message": "\"email\" must be a valid email",
            "path": [
                "email"
            ],
            "type": "string.email",
            "context": {
                "value": "in@valid",
                "key": "email",
                "label": "email"
            }
        }
    ],
    "source": "payload",
    "meta": {
        "id": "1582509238404:1c50114e-ef8e-4d97-b3e4-dbc995e8fcff:20:k6zpbe70:10030",
        "api": "1.0.0",
        "env": "production"
    }
}

In this case, I need some way to access that error so that I could update my UI - for example, by unwrapping the details field and using Formik's setErrors to provide feedback. I imagine it'd also be valuable for detailed toasts as well, where people useEffect and pop an alert if the error message changes.

After looking through createAsyncThunk, the only real solution I can think of is

  • Passing a new parameter for rethrow and rethrowing the error instead of returning finalAction.

If we were able to rethrow, we could do this sort of thing:

<Form 
   onSubmit={async (values, helpers) => {
          try {
            await dispatch(myAsyncThunk(values));
          } catch (err) {
            const joiErrors = err.response.details;
            if (joiErrors) {
              helpers.setErrors(unwrapErrors(joiErrors));
            }
          }
        }}
      />
@markerikson
Copy link
Collaborator

Per DMs, my inclination at this point is that if you need to reformat anything from the server, it's your job to do it in the payload creator.

Axios requests where you need to grab something out of response.data, normalization of entity data, extracting important values from a server error... we don't know anything about that ourselves. Do it yourself in the payload creator, and whatever you return or throw will end up in the corresponding action.

@markerikson
Copy link
Collaborator

Hmm. After further conversation, I'm still not convinced that "rethrowing" is the right idea. However, if the payload creator extracts some kind of an object from the error and tries to rethrow that, miniSerializeError() will assume it's actually an Error due to typeof error === 'object' and tried to read out the standard fields, vs just passing through the thrown object to the rejected action. That seems insufficient.

Can we do an instanceof Error check here?

@phryneas
Copy link
Member

phryneas commented Feb 24, 2020

Hmm...

This makes me question two things:

  • Do you need Redux at all here?

If you go to the lengths to even do exception handling from your component - are you even using the global store here? Does Redux make your life more easy here, or is it just a burden? Do you maybe even throw away the submission result (beyond the errors), so nothing of meaning would ever be stored in your redux store?

  • Is this an error from the programmer's view?

Sure, for the user it's an error. But for you as a programmer, that seems more like a result that you are maybe even expecting on more than 50% the responses. Is this still an error? Or wouldn't it make more sense to reserve the error/rejected way for things like connection timeouts etc. and handling this as a part of the "fulfilled" route?
I mean, the fetch was a success, it's just not the result the user wants to see, but for your app it's a pretty normal state, not really an "exception".
So you could just return this from the payloadCreator.

@markerikson

Can we do an instanceof Error check here?

I'd rather not go down this road. In TypeScript, just extending Error will give you a class that does not really extend error.
You've got to do extra magic to get a class that extends Error from the engine viewpoint.
So this is a very unreliable check for anything.

We could allow the payloadCreator to return/throw own fulfilled/rejected actions (check isFSA and actionCreator.fulfilled.match/reject on the return value/error) and let those override our finalAction. For those, we could just assume that they are serializable.

We could even think about bailing from miniSerializeError from conditions like err.constructor === Object or !(/(error|exception)/i.test(err.constructor.name)) - as this will probably be developer-provided data. If that isn't serializable, the serializability check will warn the developer in dev mode about it.

@msutkowski
Copy link
Member Author

msutkowski commented Feb 24, 2020

@phryneas Thanks for the insight!

Do you need Redux at all here?
In this instance, let's pretend we're doing PUT /users on form submit that either returns a new user instance { user: { id: 1, name: test, ... } or an error with some validation issues that come back in a 400 with a detailed message as shown. Typically, this would be a thunk as you're going to have a reducer handle inserting the returned user upon success.

Do you need redux at this point?

No, not really - we could dispatch an addUser() action after the success, or a setErrorMessage on the failure in the event we - but IMO this is more tedious and eliminates the point of a createAsyncThunk abstraction.

Is this an error from the programmer's view?

I might not be understanding correctly, but if you returned the error in the payloadCreator after catching it, you'd then be doing things like this, which seems like a more confusing approach?

[updateUserThunk.fulfilled]: (state, { payload }) => {
 // we would need to force a check here, always, as we could be returning "an error"
if (payload.user) {
  state.users = upsert(payload.user)
}

My other thought matches up with yours and I'd be happy to open a PR with that as well:

We could even think about bailing from miniSerializeError from conditions like err.constructor === Object or !(/(error|exception)/i.test(err.constructor.name)) - as this will probably be developer-provided data. If that isn't serializable, the serializability check will warn the developer in dev mode about it.

I think this is probably the best approach. My only additional feedback there is we might want to consider forcing a standardized error format when throwing a custom error object - the current format looks good, but we'd want to add a key such as data. In my example, that would look like this:

export const axiosUpdateThunk = createAsyncThunk(
  "users/update",
  async ({ data }) => {
    try {
      const result = await axios.put(`api/users`, data);
      return result.data;
    } catch (err) {
      const customError = {
        name: "Custom axios error",
        message: err.response.statusText,
        data: err.response.data // serializable
      };
      throw customError;
    }
  }
);

The benefit of this being an established pattern that offers predictable handling inside of all reducers - [whatever.rejected]: (state, action) => { state.error = action.payload.error } would always have the given shape. I could also see the counterpoint where this library shouldn't care about if a developer wants to dump an error with no context into the error message and we just serialize it away - but I think allowing that to happen in the data field is a good compromise.

@phryneas
Copy link
Member

@phryneas Thanks for the insight!

Do you need Redux at all here?
In this instance, let's pretend we're doing PUT /users on form submit that either returns a new user instance { user: { id: 1, name: test, ... } or an error with some validation issues that come back in a 400 with a detailed message as shown. Typically, this would be a thunk as you're going to have a reducer handle inserting the returned user upon success.

Do you need redux at this point?

No, not really - we could dispatch an addUser() action after the success, or a setErrorMessage on the failure in the event we - but IMO this is more tedious and eliminates the point of a createAsyncThunk abstraction.

If you actually use those actions dispatched from createAsyncThunk, that's totally fine.
It just seemed to me like you were using redux here, to in the end just derive a local component state and possibly not even modify the store in response. In which case you had no use-case for redux at all, it would just complicate your life.
Personally, I'd try to keep the logic either completely in redux or in the component, doing half of the logic in one and the other one in the other defeats the purpose a little bit for me.

Is this an error from the programmer's view?

I might not be understanding correctly, but if you returned the error in the payloadCreator after catching it, you'd then be doing things like this, which seems like a more confusing approach?

[updateUserThunk.fulfilled]: (state, { payload }) => {
 // we would need to force a check here, always, as we could be returning "an error"
if (payload.user) {
  state.users = upsert(payload.user)
}

You're gonna have that distinction either in the fulfilled reducer, or in the rejected reducer, as besides "server returned code 4/5xx", there are other possible reasons for a rejection you'd have to test for somewhere.
Depending on what you do in the end, it might fit in better with the one or the other. I just wanted to bring up the question, as it seems that if you moved this into the resolve branch, it should already be working for you.
(For example, fetch sees it like this: any kind of response, independent of status code, is a "successful" response as in "planned communication between server and client" and it throws errors only on actual network or CORS errors. I actually like that mindset.)

@msutkowski
Copy link
Member Author

Gotcha. I'm a fan of throwing custom error messages as shown, as it plays nicely with both selecting well-defined errors from the store, as well as having the option of unwrapResulting the actual desired detailed error data for consumption directly in the component. I was operating under the assumption that rejected would be replacing the common use case of NETWORK_REQUEST_FAILURE type of actions, where most implementations I've seen seem to catch & throw fetch errors to match this pattern.

As a side note, if we were to go with that mindset of matching the fetch behavior, we'd need to catch axios (and i'm assuming quite a few other fetching libraries) errors, then return them to handle them in fulfilled... which seems like it might cause some friction.

@phryneas
Copy link
Member

You do what works best for you there, no question!
I just wanted to give an explanation for my reasoning above :)

And yes, there seems to be a disconnect between fetch and other libraries - I don't really think one is better than the other, as both require you to do very specific things.

Completly new train of thought:

What is a bit of a shame here is that we cannot really provide any type information for the error of the rejected event, as it can literally be everything thrown anywhere - and TS doesn't enforce types for throw. (And neither try ... catch nor promise.catch(...) have any way of guaranteeing a specific type.)
So I haven't really seen the rejected path as something I would want to pass a lot of data down where I'd probably rely on the structure of that data.

If we would think my suggestion from above further:

We could allow the payloadCreator to return/throw own fulfilled/rejected actions (check isFSA and actionCreator.fulfilled.match/reject on the return value/error) and let those override our finalAction. For those, we could just assume that they are serializable.

In that case, we could allow devs to specify a generic type for rejected.payload and that way, data could be passed out of error situations by returning a rejected action with a set payload.

This might be the most TS-compatible way of handling this. If you still want to unwrapResult to do a catch, you can still do that - but you'll use type information that way. (As, again, a catch never has a type.)

@msutkowski
Copy link
Member Author

I think that makes a lot of sense. The typing on that would be nice as we'd just be able to do something like RejectedPayload = AxiosError<MyKnownResponse>

I did some experimenting with all of the ideas that are outlined here, and from a usability standpoint, handling success/'errors' in fulfilled isn't that awkward at all in its current state, but your idea would make it a straight forward.

@markerikson
Copy link
Collaborator

So what's the consensus out of that discussion, if any?

@msutkowski
Copy link
Member Author

I was experimenting with a lot of different ways to implement the types on this earlier and didn't come up with anything notable. I'm going to keep at it and see if I can get something to pan out.

@msutkowski
Copy link
Member Author

This was addressed in #393

@infostreams
Copy link

infostreams commented Sep 1, 2020

Ok, just for future Googler's who can't seem to figure out how to access the original response body. I've created a helper api class (as follows)

const handleRequest = (request, thunkApi) =>
    request
            .then(response => response.data)
            .catch(error => thunkApi.rejectWithValue(error?.response?.data || error))

const buildUrl = (url, params) => {
    // Support URLs with named identifiers, such as '/posts/get/:id'.
    // This code replaces the ':id' part with the value of params.id
    Object.keys(params).forEach(k => {
        if (url.indexOf(':' + k) > -1) {
            url = url.replace(':' + k, params[k])
            delete params[k]
        }
    })

    // all the parameters that were not bound to a named identifier are appended to the URL
    const encoded = Object.entries(params).map(kv => kv.map(encodeURIComponent).join("=")).join("&")
    return url + (encoded.length > 0 ? '?' + encoded : '')
}

export default class api {
    static post = (url, axiosConfig = {}) => (obj = {}, thunkApi) =>
        handleRequest(axios.post(url, obj, axiosConfig), thunkApi)

    static get = (url, axiosConfig = {}) => (obj = {}, thunkApi) =>
        handleRequest(axios.get(buildUrl(url, obj), axiosConfig), thunkApi)

    static delete = (url, axiosConfig = {}) => (obj = {}, thunkApi) =>
        handleRequest(axios.delete(buildUrl(url, obj), axiosConfig), thunkApi)
}

just so that I can easily create async thunks like this

import {createAsyncThunk} from '@reduxjs/toolkit'
import api from "service/api"
export const requestPasswordReset = createAsyncThunk('login/requestReset', api.post('/password/email'))

and refer to the originally returned JSON in my extraReducers as follows

        [requestPasswordReset.rejected]: (state, action, more) => {
            state.loading = 'error'
            state.error = action?.payload
        },

Now, state.error will contain the JSON that was returned by my API. Hope this helps someone.

@lqqokim
Copy link

lqqokim commented Oct 15, 2020

how pass parameters ??

export const requestPasswordReset = createAsyncThunk('login/requestReset', api.post('/password/email'))

@infostreams
Copy link

infostreams commented Oct 15, 2020

Hi, I updated the code in my comment so that you can now also properly use parameters in GET and DELETE requests. There's even support for named parameters, so your URL can be something like "/post/get/:id" and it will replace the :id with the appropriate value.

You can pass these values when dispatching the action, for example

this.props.dispatch(requestPasswordReset({username:"someone@example.com"})

@lqqokim
Copy link

lqqokim commented Oct 19, 2020

Hi, I updated the code in my comment so that you can now also properly use parameters in GET and DELETE requests. There's even support for named parameters, so your URL can be something like "/post/get/:id" and it will replace the :id with the appropriate value.

You can pass these values when dispatching the action, for example

this.props.dispatch(requestPasswordReset({username:"someone@example.com"})

Thank you very much for fixing it in a better way.
But I took the following method to make the url change for the axios method and the choice for params, queryString more convenient.

users.js

import {createAsyncThunk} from '@reduxjs/toolkit'
import {http, thunkHandler} from '../utils/api';

export const signInUser = createAsyncThunk(
    'users/signInUser',
    (user, thunkAPI) => thunkHandler(http.post('/users/signIn', user), thunkAPI);
)

api.js

export const http = axios.create({
    baseURL: apiUrl,
    headers: {
        Accept: 'application/json',
        'Content-Type': 'application/json'
    },
});

export const thunkHandler = async (asyncFn, thunkAPI) => {
    try {
        const response = await asyncFn;
        return response.data.result;
    } catch (error) {
        return thunkAPI.rejectWithValue(error.response.data);
    }
};

@jmuela1015
Copy link

jmuela1015 commented Oct 12, 2021

Based on the example from @infostreams this is what I came up with ... just incase it helps someone else.

UPDATED per the suggestions from @markerikson. Thank you!

import {createAsyncThunk, createSlice} from '@reduxjs/toolkit'
import axios from "axios";

export const signInUser = createAsyncThunk(
  'users/signInUser',
  async (user, thunkApi) => {
    return axios.post('/users/signIn', user)
      .then(response => response.data.results)
      .catch(error => thunkApi.rejectWithValue(error?.response?.data || error))
  }
)

export const usersSlice = createSlice({
  name: 'users',
  initialState:  { user: {}, errors: [] },
  reducers: {},
  extraReducers: builder => {
    builder
      .addCase(signInUser.pending, state => {
        state.errors = []
      })
      .addCase(signInUser.fulfilled, (state, action) => {
        state.errors = [];
        state.user = action.payload;
      })
      .addCase(signInUser.rejected, (state, {payload}) => {
        switch (payload?.response?.status) {
            case 401:
                state.errors.push({ error: "Access denied." }); break;
            case 403:
                state.errors.push({ error: "Forbidden." }); break;
            default:
                state.errors.push(payload); break;
        }
      })
  }
});


export const { } = usersSlice.actions;
export default usersSlice.reducer;

@markerikson
Copy link
Collaborator

@jmuela1015 two quick suggestions syntactically:

@felixcatto
Copy link

felixcatto commented Mar 5, 2023

Based on @lqqokim solution

export const thunkHandler = asyncFn => async (arg, thunkAPI) => {
  try {
    const response = await asyncFn(arg, thunkAPI);
    return response.data;
  } catch (e) {
    return thunkAPI.rejectWithValue(e.response.data);
  }
};

export const signIn = createAsyncThunk(
  'signIn',
  thunkHandler(async userCreds => axios.post(getApiUrl('session'), userCreds))
),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants