Rethrow apollo errors and recommend error boundaries

[trojanowski]

Right now errors are returned as a part of the useQuery result. Maybe a better idea would be to throw them and recommend to use Error Boundaries instead (at least in the suspense mode).

So instead of this code:

const Hello = () => {
  const { data, error } = useQuery(GET_HELLO);
  if (error) return `Error! ${error.message}`;

  return <p>{data.hello.world}</p>;
};

we could write:

const Hello = () => {
  // we can be sure here that the error hasn't occurred
  return <p>{data.hello.world}</p>;
};

and use an error boundary at the root of the react tree (or anywhere else above the component) - similar to how we use the <Suspense /> component.

[fbartho]

Forcing an ErrorBoundary around every component seems painful at first glance. When useQuery I want to catch error-states. If you changed this, I think I'd have to do:

let result
try {
  const {data} = useQuery(MY_QUERY);
  result = data;
} catch(e) {
  if (isPromise(e)) { throw e; }
  result = makeErrorData(e)
}
return (<Component {…result} />);

[trojanowski]

I'd make it optional (but enabled by default) so if you wanted the old behavior you could use it like e. g. const { data, error } = useQuery(MY_QUERY, { throw: false });.

But the general idea is to make error management similar to what suspense does for data loading. So in most cases, it should be enough to put a single error boundary at the top of the React tree, but if you need it, you can also put it at the bottom, just above a component which requires a special behavior. Please compare:

render(
  <Suspense fallback={<GlobalLoadingFallback />}>
    <MyPageComponent>
      <PageDataComponent />
      <Suspense fallback={<ImageLoadingFallback />}>
        <MyImageComponent />
      </Suspense>
    </MyPageComponent>
  </Suspense>
);

and:

// a helper component but you can also easily implement it by yourself
import { ErrorBoundary } from 'react-error-boundary';

render(
  <ErrorBoundary FallbackComponent={GlobalError}>
    <MyPageComponent>
      <PageDataComponent />
      <ErrorBoundary FallbackComponent={ImageError}>
        <MyImageComponent />
      </ErrorBoundary>
    </MyPageComponent>
  </ErrorBoundary>
);

Of course, you can mix suspense and error boundaries. Please also note that you have access to the error instance inside of the error boundaries.

[avocadowastaken]

#34 also related to this, because we have to merge array of GraphQL errors in to single ApolloError.

[bradleyayers]

I came across this issue because I'm trying to use useSubscribe in my component, where the variables for the subscription come from the data returned by useQuery. I'm not sure how to adhere to the rule "hooks must be called unconditionally" (i.e. not in an if (!error) { … } branch) if it's possible that data from useQuery is not available.

The idea of using exceptions for this, inspired by suspense, seems like it might be workable. Though I'm not thrilled with the magic and implicit requirement to wrap components in a <Error fallback={…}>, though I guess it's no worse than needing <Suspense> (I haven't started using suspense, so I'm not sure how severe my concerns are in practice). Perhaps it can be enforced adequately by having the <Error> component contribute something to the context that's then checked at runtime by the useQuery hook.

In fact I don't think even this would be enough for my use-case, in my case the subscription really is optional, depending on whether the data from the query includes some specific fields. The subscription simply isn't needed in some scenarios. Perhaps I actually need a no-op subscription query that I can pass into useSubscription when I don't need it.

@trojanowski have you had any more thoughts about this issue since December?

[danielkcz]

Hah, I just wanted to open a new issue to propose exactly this. I definitely support this. Even though it's possible to do it with custom hooks, it sounds like a good idea to have it by default.

Btw, it's not really necessary to wrap every component in the error boundary. That's the beauty of it. You can have a single boundary on top of the app. Then there might be some left panel which can render errors differently, but another part of the app is still working. There can be "Retry" button which would essentially re-mount those components and re-run queries to try it again. 😍

I believe this is what React is trying to teach us with Context and Suspense. Instead of thinking about atomic components, we should be thinking about features and how to present failures to a user the best way.

Frankly, I haven't tried this in real world yet, but I definitely want to!

[trojanowski]

I think it would make sense to have two separate hooks long term (when Suspense for data fetching will be considered stable):

// This hook uses both suspense and error boundaries, so we can be sure here
// that `data` in the returned object is not null and has a correct type.
useHighLevelQuery<TData, TVariables>(query, options): {
  data: TData,
  // ...
}

// Like in the current version of `useQuery` - we can't be sure if `data`
// is already loaded (or if a network error happened) so it can be undefined.
// Also, it's possible that there were errors in GraphQL resolvers, so a correct
// type for data, in that case, should be something like RecursivePartial:
// https://stackoverflow.com/a/51365037/2750114
useLowLevelQuery<TData, TVariables>(query, options): {
  data?: RecursivePartial<TData>;
  loading: boolean;
  // a network error (or also a GraphQL error since v0.4.0)
  error?: ApolloError;
  // GraphQL errors (thrown by server resolvers)
  errors?: GraphQLError[];
  // ...
}

Of course, the actual hook names would be different.

In the short term, I'll add a throw option to useQuery. It will be disabled by default - I think it makes the most sense if used together with suspense - which right now is marked as experimental in docs.

[danielkcz]

In the short term, I'll add a throw option to useQuery. It will be disabled by default - I think it makes the most sense if used together with suspense - which right now is marked as experimental in docs.

Hm, but throwing errors are not related to Suspense, it's about error boundaries which are stable. So I would stick to the original plan and make it default :) And btw, I tried to use throw with a new useMutation and had a problem related it is reserved word. Something like shouldThrow would be more appropriate.

Otherwise, I agree with high/low hooks, I already got something like that implemented because I was annoyed by checking for data and if it contains actual data. It works quite nicely.

  const { data, loading, error, ...rest } = Hooks.useQuery<TData, TVariables>(query, options)
  if (error) {
    throw error
  }
  return {
    loading: loading || !data || isEmpty(data),
    data: data as TData,
    ...rest,
  }

[trojanowski]

Hm, but throwing errors are not related to Suspense, it's about error boundaries which are stable.

They are related to me that way that they work best combined. I don't see too much benefit in that code:

const Hello = () => {
  const { data, loading } = useQuery(GET_HELLO);
  if (error) return <Loading />;

  return <p>{data.hello.world}</p>;
};

render(
  <ErrorBoundary>
    <Hello />
  </ErrorBoundary>
);

vs

const Hello = () => {
  const { data, error, loading } = useQuery(GET_HELLO);
  if(!dataNotAvailable({ data, error, loading }) {
    return <FallbackContent error={error} loading={loading} />
  }

  return <p>{data.hello.world}</p>;
};

render(<Hello />);

So I would stick to the original plan and make it default :)

When I wrote the original plan, I hadn't thought I'd change the default value of the suspend option to false. :)

And btw, I tried to use throw with a new useMutation and had a problem related it is reserved word. Something like shouldThrow would be more appropriate.

Good catch. I remember discussions about class vs className for React Fire. shouldThrow sounds OK, but what do you think about rethrow?

So I plan to introduce a new option (throw/shouldThrow/rethrow) which will have false as the default value. That way useQuery with default options will behave similarly to the <Query /> component from react-apollo. When Suspense for data fetching is considered stable, I'll rename useQuery to something like useLowLevelQuery and mark it as a hook for more advanced use cases and provide a new version of useQuery - something like useHighLevelQuery from #28 (comment).

[danielkcz]

@trojanowski I have a feeling you haven't used error boundaries too much yet so you are kinda biased against them :)

I don't agree that handling errors is the same as handling loading state. Usually, you want some way to recover from the error. Be it reloading the whole app to avoid cascading errors or even re-rendering only part of the tree which has failed.

And that's exactly where error boundaries shine. Really think of them as "feature recovery container". Under the "feature" term you can imagine a whole app, some kind of product listing or even a button that does some kind of validation. In case of the error, you can show the fallback UI which can be about user filling some kind of feedback or really a "retry" button which would re-execute all queries.

Think about that last statement: The error boundary can re-execute all contained queries for you. If you would be handling errors manually in every component, you also have to manually call refetch in case of error. You could also end up with a bunch of "retry" buttons there for each query. Not even thinking about multiple queries within a component.

What's also important to mention, you can have a super simple error boundary which would only log the error, but keep UI untouched and then you can really do a granular work of handling errors on query level if you like.

class ErrorBoundary extends React.Component {
  componentDidCatch(error) {
    console.error(error)
  }

  render() {
    return this.props.children; 
  }
}

We could expose such boundary directly in this package just for convenience.

---

Besides, in my rewrite of the useMutation (#93) I am solving a similar problem and I think that error strategy should be really similar. So mutation would be throwing errors by default, but queries won't, it would be super confusing :)

[trojanowski]

I have a feeling you haven't used error boundaries too much yet so you are kinda biased against them :)

I use them and like them. Please notice that I opened this issue :)

And that's exactly where error boundaries shine. Really think of them as "feature recovery container". Under the "feature" term you can imagine a whole app, some kind of product listing or even a button that does some kind of validation. In case of the error, you can show the fallback UI which can be about user filling some kind of feedback or really a "retry" button which would re-execute all queries.

Think about that last statement: The error boundary can re-execute all contained queries for you. If you would be handling errors manually in every component, you also have to manually call refetch in case of error. You could also end up with a bunch of "retry" buttons there for each query. Not even thinking about multiple queries within a component.

It's the opposite of my experience. I usually try to have only one query per page with the help of fragment collocation. However, there are cases when I have more than one query. But there is always a "main" one - e.g. which is used to server-render the page, and there are "less important" components with own queries, which are invoked after client JS bundle is loaded. I definitely wouldn't like to show a fallback container on the whole page or re-fetch the main query if the less important one failed.

Besides, in my rewrite of the useMutation (#93) I am solving a similar problem and I think that error strategy should be really similar. So mutation would be throwing errors by default, but queries won't, it would be super confusing :)

I suggested something else. Please look at #93 (comment).

[trojanowski]

So I'd like support for error boundaries enabled by default, but not yet. As I said in #28 (comment) I'd like to have two hooks - the high-level one recommended for most cases (which will use suspense and error boundaries) and the second one - when you prefer to manage everything by yourself.

[danielkcz]

It's the opposite of my experience. I usually try to have only one query per page with the help of fragment collocation

Funny, we are doing quite an opposite to support modularization. Each part of the app can stand on its own without relying on something. Besides, fragments have a big flaw as they don't support arguments. We need that very often and I cannot imagine having a huge query that accepts tens of unrelated variables.

Perhaps, I am missing something there, but it doesn't matter. Point being for matters of this issue that there are vastly different approaches and every default will probably annoy someone :)

[nareshbhatia]

@trojanowski, I am not able to understand your original proposal.

As far as I can tell, you are proposing that the following code could handle errors properly by introducing an Error Boundary at a higher level in the component tree.

const Hello = () => {
  const { data } = useQuery(GET_HELLO);
  return <p>{data.hello.world}</p>;
};

My understanding is that Error boundaries do not catch errors from asynchronous code, which is what the useQuery hook is. So an error thrown by that hook wouldn't be caught by the Error Boundary anyway. What am I missing here? Can you please explain.

[danielkcz]

@nareshbhatia You can throw error synchronously even from useQuery, basically like following. I am doing that in my fork of this lib and works great.

const useQuery = () => {
  const [error, setError] = React.useState(null)
  if (error) {
    throw error
  }
}

[nareshbhatia]

Thanks @FredyC. I tried this approach in my code and it seems to work well. I am seeing my ErrorBoundary catch the error and display the error message. The only glitch is that after all this happens the component is being re-rendered, I don't know why. This causes the useQuery to be called again and another error being thrown. This time the error is not being caught by the error boundary and the app crashes.

I will take a look at this tomorrow, but if you have any insights please let me know.

[danielkcz]

The only glitch is that after all this happens the component is being re-rendered

That's the behavior of ErrorBoundary unfortunately. There is no way to prevent re-render, it's by design to avoid "broken UI". Basically, you should render some replacement UI which informs about the error. It can have "retry" button to try again or possibly a "restart" button to reboot the whole app.

[nareshbhatia]

Thanks @FredyC, I got it to work! Here's the hook and here's the ErrorBoundary.

Note that my use case is REST, but the idea for handling errors is the same.

[nareshbhatia]

FYI, I am having mixed luck with throwing an error synchronously and having the ErrorBoundary handle it.

1. In this custom hook, throwing a synchronous error works perfectly. It is caught by the ErrorBoundary and I am seeing the fallback UI.
2. When using a MobX store, if I check for an error in the store and throw it synchronously, I get “Reaction already disposed” from MobX followed by “Rendered fewer hooks than expected” from React. (see this commit where I reverted the code). I spent half a day on this - I am sure all my hooks are at the top level and not hiding in any conditionals. I suspect that in this case MobX is rerendering the component while ErrorBoundary is also trying to render the fallback UI.
3. In another case (a private project), the component is being rerendered as soon as I throw a synchronous error - I have no idea why - its state has not changed! This causes useEffect() to be run again and throw a second error. The first error shows the error boundary fallback UI for a second and then the app crashes - not being able to handle the second error.

It seems that the interactions between thrown errors and error boundaries are very finicky. So far my workaround is not to throw synchronous errors, but expose them as part of the API. The client component can check for these errors explicitly and show error messages. This seems to be much more stable.

[danielkcz]

Well, generally you are better without actually throwing errors :) It's a quick & dirty way, but also kinda error-prone (pun intended). I would say the ErrorBoundary should be considered more like a safety net, not the general way of handling errors.

A much better way, in my opinion, is to utilize the Context. You can simply expose yourself a onError callback to which you will pass any error you have. The Provider can similarly decide to render fallback UI, with the help of React.cloneElement it can even simulate the behavior of ErrorBoundary to remount if it's desired. It definitely gives much better control. I made myself an abstraction that in essence looks like this.

    <ApolloDefender
      onNetworkError={onNetworkError}
      onUserErrors={onUserErrors}
      onOperationError={onOperationError}
    >
      <SillyErrorBoundary onError={onUnhandledError}>
        {render()}
      </SillyErrorBoundary>
    </ApolloDefender>

Note: ApolloDefender is proprietary for now

This is also a reason why I wanted to wrap the errors with additional information in #93, but @trojanowski rejected without giving it much of the thought seemingly. I have a custom fork that has it and it's amazing. In the ApolloDefender I can see all information about a failed operation without any awkwardness.

[nareshbhatia]

@FredyC, thanks for your insights.

I am failing to understand how Context would help in this case. While an onError can pass the error to a context provider that will render the fallback UI, how is this approach different from an error boundary aside from the transport mechanics? I can't see how this will prevent the rendering conflicts that I have described above. I must be missing something.

[danielkcz]

Well, I think that throwing known errors can be simply unpredictable. You would need to take care of edge cases and ensure nothing is left hanging. It's like you would take a stab into the component and if it's not ready for it, it will crumble. It's extremely hard to ensure everything is solid.

The Context is a completely different mechanism that won't do anything bad to the component. Instead, you will decide if current UI should stay there or be properly cleaned up.

[nareshbhatia]

Thanks, @FredyC, that does give me a better "context" (pun intended) :-)

[amcdnl]

@FredyC - Would you mind sharing the ApolloDefender component?

[danielkcz]

@amcdnl Well, there is nothing magical about it, it's just bunch of conditions about recognizing of what type of error it is and calling the appropriate callback with it. It builds on top of ApolloOperationError to supply more details about the error. And there is a Context which provides onError callback which can be called from the app code (instead of throwing that error).

I don't want to share it because it also contains a bunch of specific code to our projects and backend solution that would be only confusing if presented. Sorry.