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

Composition - Error Handling? #81

Closed
Benjamin-Dobell opened this Issue Jul 5, 2016 · 7 comments

Comments

7 participants
@Benjamin-Dobell

Benjamin-Dobell commented Jul 5, 2016

I've started writing my asynchronous actions like this:

export function fetchLocations(userId: string, locationIds: Array<string>) {
  return async function(dispatch: Function) : Promise<Array<Location>> {
    try {
      dispatch(startFetchingLocations(userId, locationIds))
      const locations = await SomeAPI.fetchLocations(userId, locationIds)
      dispatch(successfullyFetchedLocations(userId, locations))
      return locations
    } catch (e) {
      dispatch(failureFetchingLocations(userId))
      throw e
    }
  }
}

The idea is that these actions are composable/chainable (i.e. you can then() or await on dispatch, get the Promise result (locations above) and then do something else. However, these actions do not need to be chained, I've also got synchronous success and failure actions that my reducers listen for.

To make these asynchronous actions properly chainable, in addition to dispatching the failure message, they rethrow any errors. This means the chain can catch the error.

Chained use (Composition)

async function doStuffChained()
  try {
    const locations = await dispatch(fetchLocations(userId, locationIds))
   // Do some stuff with the returned locations
  } catch (e) {
    // Do something
  }
}

Regular use

function doStuff() {
  dispatch(fetchLocations(userId, locationIds))
}

The problem

The issue is when I want to use this action in a non-chainable fashion (e.g. doStuff) I get React Native complaining at me that I've got unhandled promise errors - because we rethrow our errors so they're chainable, but in doStuff we don't care about errors and are discarding dispatch's return value (in this case a Promise). Instead the errors are handled in the reducer thanks to my failureFetchingLocations() action.

I realise that I could change doStuff to:

function doStuff() {
  dispatch(fetchLocations(userId, locationIds)).catch(e => console.log(e))
}

However, this seems awfully peculiar and will break if I were to change my action to be synchronous. The asynchrony of the action is really an implementation detail, callers shouldn't need to know whether or not the action is asynchronous.

Instead of simply extending dispatch's behaviour, redux-thunk has broken its contract. We're now requiring dispatch to be used in an entirely different way, and it's unclear from looking at the code whether or not you need to handle dispatch errors or not. It's effectively a violation of Liskov's principle of substitution i.e. You cannot use redux-thunk's altered dispatch everywhere you would use redux's (default) dispatch.

EDIT: I realise Liskov's principle of substitution typically refers to OOP inheritance, but you get the gist.

Is there an existing solution for this problem?

A default error handler could perhaps be installed to gobble up the errors. That would prevent the violation of dispatch's contract i.e. you don't need to occasionally capture promise errors. I'm not sure how other people would find that solution though - also, it would need to be done in a way that doesn't break chaining.

@Benjamin-Dobell Benjamin-Dobell changed the title from Chaining - Error Handling? to Composition - Error Handling? Jul 5, 2016

@vmasto

This comment has been minimized.

vmasto commented Mar 2, 2017

This is an interesting problem and question of mine as well @Benjamin-Dobell.

I wonder what you ended up doing?

@Benjamin-Dobell

This comment has been minimized.

Benjamin-Dobell commented Mar 2, 2017

@vmasto No solution at present. Admittedly I haven't tried to come up with a solution.

I'm just ignoring/dismissing the YellowBox warnings at the moment - they're infrequent because this is inherently only an issue when errors are thrown (luckily a rarity in my app).

However, it's my opinion that this is a fairly serious design flaw with redux-thunk, so I would like to see it addressed. Or at least start a dialogue regarding the issue.

@mzedeler

This comment has been minimized.

mzedeler commented May 23, 2017

I have another related issue: it seems that React rerenders inside the call to dispatch, so if there are any rendering errors, they'll bubble up through the dispatch call. Using redux-thunk with promises makes it complicated to tell if an exception should be handled or not, because if you handle rendering errors, they won't show up in the console.

@jstejada

This comment has been minimized.

jstejada commented Jul 26, 2017

I also have this problem, one thing I ended up doing is passing a flag to indicate that the error should be re-throws, which is false by default. Not ideal :/ but it was a quick workaround

@yussinsharp

This comment has been minimized.

yussinsharp commented Jan 21, 2018

I also have this problem. When using thunks to wrap axios calls like:

function fetchData() {
  return (dispatch, setState) => {
    return axios.get('/some/url')
      .then(({ data }) => {
        // handle successful request
        dispatch(receivedData(data));
        dispatch(setIsFetching(true));
      })
      .catch((err) => {
        // handle axios error from server
        // HOWEVER, if one of the dispatch calls causes
        // a react error I receive it here, makes it very hard to debug
        dispatch(handleError(err));
      });
  };
}
@noinkling

This comment has been minimized.

noinkling commented Jan 21, 2018

I ran into this issue and ended up doing the same thing as @jstejada: making my async action creators accept a rethrow flag as an (optional) argument.

@timdorr timdorr closed this May 25, 2018

@mzedeler

This comment has been minimized.

mzedeler commented May 25, 2018

Just for the record, I wound up explicitly catching and re-throwing errors that bubble up through dispatch calls while handling the rest. I found out that this pattern works for many of those cases:

function fetchData() {
  (dispatch, setState) =>
    axios
      .get('/some/url')
      .then(handleOk, handleError)
}

By using the two parameter version of then errors in handleOk will not be caught in handleError as it would if you just add a catch to the chain, so any errors in dispatch calls in handleOk will bubble out and result in an unhandled failed promise exception (which covers my requirements).

This pattern can be reused inside handleOk and handleError, allowing unlimited nesting of the pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment