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

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

Open
essensx opened this issue Jan 6, 2021 · 5 comments

Comments

@essensx
Copy link

essensx commented Jan 6, 2021

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

payload: new RequestError(e.message),

Don't think this should be the case

@iamandrewluca
Copy link
Collaborator

iamandrewluca commented Jan 6, 2021

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

// Process the server response
if (isOk) {
return next(await actionWith(successType, [action, getState(), res]));
} else {
return next(
await actionWith(
{
...failureType,
error: true
},
[action, getState(), res]
)
);
}

@essensx
Copy link
Author

essensx commented Jan 6, 2021

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
Copy link
Collaborator

iamandrewluca commented Jan 6, 2021

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
Copy link
Author

essensx commented Jan 6, 2021

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
Copy link

davesag commented Apr 29, 2021

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
  })
}

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

3 participants