agraboso / redux-api-middleware

Redux middleware for calling an API.
MIT License
1.49k stars 195 forks source link

Custom payload function is overwritten in RequestError & InternalError instances #257

Open essensx opened 3 years ago

essensx commented 3 years ago

Not sure if this is intended by design or not, but currently, it's not possible to handle these types of errors in the FETCH_ERROR, payload function.

It gets overwritten, here for example https://github.com/agraboso/redux-api-middleware/blob/2263853010708a75ac94af4878847068ea122b53/src/middleware.js#L185

Don't think this should be the case

iamandrewluca commented 3 years ago

I think this is intended by design. This happens when your fetch is something custom and throws an explicit error. Server error is handled here

https://github.com/agraboso/redux-api-middleware/blob/2263853010708a75ac94af4878847068ea122b53/src/middleware.js#L209-L222

essensx commented 3 years ago

Well, it does make little sense in network failure (lost internet connection or any other exception fetch throws) to dispatch the same action type of failure, but do not respect payload function in these cases. Feels like a mistake, as the API basically is inconsistent and is prone to unhandled cases.

This is exactly how i discovered this 😄

iamandrewluca commented 3 years ago

Hmm, maybe I don't understand exactly your use case.

Any network failure or lost connection, you don't have any payload. For handling server response error, is checked by options.ok In case when your server responds with a 200 for every response, you can use a custom fetch (options.fetch) like this one bellow to change request

async function doFetch(input: RequestInfo, init?: RequestInit): Promise<Response> {
  const _res = await fetch(input, init)
  const contentType = _res.headers.get('Content-Type')
  if (_res.ok && contentType && contentType.indexOf('json') !== -1) {
    const res = _res.clone()
    const json = await _res.json()
    if (json.status === 'ERROR') {
      // all `Response` properties are readonly,
      // so we need `Object.defineProperty` to override them
      Object.defineProperty(res, 'statusText', { value: 'Internal Server Error' })
      Object.defineProperty(res, 'status', { value: 500 })
      Object.defineProperty(res, 'ok', { value: false })
    }
    return res
  }
  return _res
}
essensx commented 3 years ago

Yes, you are correct, but how could i handle what is inserted in the store in such cases? I could, of course do some hacking in the fetch implementation, as you given in the example above, but i don't find this the best solution in such a situation. Even in a network error case, the payload error is inserted in the store.

I create a custom shape for the data, so this basically breaks, as i have no possibility to edit the data that is stored, unless i wrote a middleware (overkill) or something.

davesag commented 3 years ago

I have a slightly different use case for this.

I need my bailout function to throw an error and I need errors to trigger a *_FAIL action.

export const login = ({ login: username, password }) =>
  createAction({
    endpoint: `${apiBase}/v2/oauth/token`,
    method,
    headers,
    bailout: () => {
      if (FORBIDDEN_USERNAMES.includes(username.toLowerCase())) {
        console.debug('username forbidden', username)
        throw new Error('Disallowed Username')
      }
      return true
    },
    body: `username=${username}&password=${password}&grant_type=password`,
    types: ['LOGIN', 'LOGIN_SUCCESS', 'LOGIN_FAIL']
  })

It would be ideal to be able to report this error rather than simply logging it. Alas when the bailout throws it simply gives me the RequestError '[RSAA].bailout function failed' which is not very helpful. Maybe RequestError could add an optional cause prop that lets e get passed into the RequestError along with the message.

FYI I worked around the issue of errors not triggering *_FAIL responses by writing the following middleware function which runs after the api middleware

failOnError.js

export const failOnError = ({ dispatch }) => next => async action => {
  const { type, error } = action

  if (!error || !type.endsWith('_SUCCESS')) return next(action)

  const failType = type.replace('_SUCCESS', '_FAIL')

  dispatch({
    ...action,
    type: failType
  })
}