[4.0] Question about new error types

[wassim-k]

Hi,

I've been testing the new Apollo Client v4 in an enterprise Angular app, and I want to start by saying great work so far on the new changes, to name a few:

• The migration of Observables to RxJS, espcially for Angular, as it provides a much more seamless intergration.
• watchQuery observable no longer terminating on errors.
• More consistent typing across the library.

On the other hand, one area that I found a bit more challenging to migrate was error handling, and I had some feedback if you don't mind me sharing:

Default error message

Currently, it's common to use apollo.watchQuery in a component and simply display result.error.message. I noticed the error message has changed, for example:
v3:

Email address is already in use

v4:

The GraphQL server returned with errors:
- Email address is already in use

My guess, and I might be wrong, most developers would probably choose to hide that part of the message, for example in our case, almost all of our end users don't know what GraphQL is and that to me feels like an implementation detail.

I know, the solution is simple, to call a formatError function, or implement onError link and wrap it in a new error type with a formatted message, but that introduces overhead and complexity in the app, for what I believe should be the default behavior. I'm keen to hear your thoughts on this?

Network errors

I noticed ApolloError has been replaced by multiple new error types, for the most part it's a welcome change.

The only challenge I found with the new types is that it's hard to distinguish a network error from other errors thrown in the app, this is especially challenging when an ApolloClient operation is only one part of a more complex async flow, it used to be easy to do an ApolloError.networkError check then, for example, decide to retry the operation. But now, and I might be missing something here, there's no simple reliable way of doing that anymore? I'd appreciate some suggestions.

Thanks.

[jerelmiller]

Hey @wassim-k 👋

This is exactly the type of feedback we are looking for, so thank you so much for providing it! Great to hear you like the changes we've made so far! That helps a lot 🙂.

Default error message

You've got a valid point here. The biggest reason we made this change to something less UI friendly was to nudge users to handling GraphQL errors more completely rather than just relying on our own formatting rules to do so.

I know, the solution is simple, to call a formatError function, or implement onError link and wrap it in a new error type with a formatted message, but that introduces overhead and complexity in the app, for what I believe should be the default behavior. I'm keen to hear your thoughts on this?

I would actually recommend against this, purely because the CombinedGraphQLErrors class that wraps GraphQL errors has additional properties for accessing the raw errors along with any partial data in the response. Unless you have your own wrapped error class that does these things, you would lose access to this data. Not only that, but you'd need to emit the error in a different way for this to work (i.e. your link needs to call observer.error(new CustomWrappedError())) which breaks functionality like errorPolicy: 'all'.

The only challenge I found with the new types is that it's hard to distinguish a network error from other errors thrown in the app, this is especially challenging when an ApolloClient operation is only one part of a more complex async flow, it used to be easy to do an ApolloError.networkError check then, for example, decide to retry the operation. But now, and I might be missing something here, there's no simple reliable way of doing that anymore? I'd appreciate some suggestions.

I think you'll like #12546 which should be out in the next alpha. This adds a static is method to our new error classes which should make determining whether an error is a specific type much easier. is also adds type-narrowing so you can easily access additional properties on those error types.

Once this lands, rather than checking if (error.networkError), you'd do something like:

if (CombinedGraphQLErrors.is(error) {
  // error is type-narrowed to CombinedGraphQLErrors
} else if (CombinedProtocolErrors.is(error)) {
  // error is type-narrowed to CombinedProtocolErrors
} else {
  // you can treat the rest of these as "network" errors
}

That is property will be added to all of our error classes:

• CombinedGraphQLErrors
• CombinedProtocolErrors
• ServerError
• ServerParseError
• UnconventionalError

Keep an eye out for this change to land! (hopefully today 🙂)

---

On the default error message, I'm not opposed to reverting back to the default in v3, especially given the fact that we don't recommend rewriting the error in the link chain. Let me bring this back to my colleagues and I will get back to you on that!

---

Once again, thanks so much for the feedback! Please feel free to open issues with as much feedback as you're willing to provide. It's difficult to gauge whats working and what isn't otherwise and this helps us a ton!

[jerelmiller]

@wassim-k I just opened #12557 which updates the error message for CombinedGraphQLErrors and CombinedProtocolErrors back to the v3 format. This PR also makes it possible to override the default formatter in case you want to change this app-wide. That should provide enough flexibility for those that may want to change the format without having to create their own wrapped classes. We will try and get this out in the next alpha or two.

Thanks again for the feedback!

[wassim-k]

@jerelmiller thanks for taking the time to provide a detailed response to my questions. I also appreciate you already making the change to CombinedGraphQLErrors default error message, I believe the solution you provided has the least "friction" while giving users the control to change it as needed.

And the new is type narrowing static methods will definitely be useful.

I don't know if this is a concern in real world scenarios, but I thought I'd flag it for your consideration anyway:
I can envisage a potential use case, where the following function is called:

function inviteUser() {
  if (currentUserAccount.isIncomplete) throw new Error('You must complete your account before inviting other users.');

  return apollo.mutate({ ... });
}

In this case, the error elimination method you described above might not be sufficient in distinguishing the network error from the app thrown error, where the caller might decide to retry the operation a couple of times in case of a network error...

Again, This might be a very edge case that other more verbose solutions in this case will suffice, for example:

function inviteUser() {
  if (currentUserAccount.isIncomplete) throw new Error('You must complete your account before inviting other users.');

  return apollo.mutate({ ... })
    .catch(error => {
      if (isNetworkErrorByElimination(error)) {
        throw new CustomNetworkError(error);
      }
      throw error;
    });
}

If you don't find this to be of concern, happy for you to resolve this issue and thanks again for taking the time.

[jerelmiller]

@wassim-k would you mind going into a bit more detail about how inviteUser is invoked? I think I might be missing the context on how that is used since the thrown error in that first example exists outside the scope of mutate.

I think I might also be missing the reason you need CustomNetworkError in your case. Are you perhaps envisioning a case where you'd call client.mutate again as a result of an error to try the mutation again?

inviteUser().catch((error) => {
  // try again
  return inviteUser()
});

And if that is the case, I think you're asking how best to narrow down to the "network error" case so that if you only want to retry on network errors you can do so correct?

Assuming yes, that would look something like this:

// rather than this from v3:
inviteUser().catch((error) => {
  if (error.networkError) {
    return inviteUser()
  }
});

// Check if the error is GraphQL errors
inviteUser().catch((error) => {
  if (!CombinedGraphQLErrors.is(error)) {
    return inviteUser()
  }
});

Mutations can only return CombinedGraphQLErrors or the raw error returned from the server, so if its not CombinedGraphQLErrors, then you know you're working with a network error.

CombinedProtocolErrors is specific to multipart HTTP subscriptions, so the only case where you would ever see those is with client.subscribe() to kick off a subscription.

Again, I might be missing the context on what CustomNetworkError is good for here, so please let me know the use case!

[wassim-k]

Apologies if I didn't explain it very well, but I wanted to try and provide a simple example of a use case we already had in our app.

I think I might have made a mistake in my function, I only work with observables in Angular 😅, this what it should look like:

function inviteUser() {
  if (currentUserAccount.isIncomplete) {
    return Promise.reject(new Error('You must complete your account before inviting other users.'));
  } else {
    return apollo.mutate({ ... });
  }
}

Basically, yes, the caller of inviteUser() would check the error thrown and retry it based on the type of error, in the example you provided:

inviteUser().catch((error) => {
  if (!CombinedGraphQLErrors.is(error)) {
    return inviteUser()
  }
});

This wouldn't work, because !CombinedGraphQLErrors.is(error) will still return true for the error thrown in code, hence CustomNetworkError:

inviteUser().catch((error) => {
  if (error instanceof CustomNetworkError) {
    return inviteUser()
  }
});

[jerelmiller]

Ok this definitely helps but I think I'm still missing what you mean here:

This wouldn't work, because !CombinedGraphQLErrors.is(error) will still return true for the error thrown in code, hence CustomNetworkError:

We brand CombinedGraphQLErrors with a special symbol that we use in .is to ensure the error is indeed one thrown by us, so that error returned from Promise.reject(new Error(...)) should never be true in this case.

function inviteUser() {
  // pretend this is all that is here for now
  return Promise.reject(new Error("You must..."))
}

inviteUser().catch(error => {
  // this returns `false` since `Error` is not `CombinedGraphQLErrors`
  if (CombinedGraphQLErrors.is(error)) {
    // ...
  }

  // error is a network error from this point forward:
})

Or am I misunderstanding which error you're referring to when you say "error thrown in code"?

[jerelmiller]

Oh you know I think the miscommunication might be coming down to how you and I define the term "network error" here. We will likely retire that term in Apollo Client v4, but I've been using that to mean "any non GraphQL error". I think you're talking about it here as an error emitted from the link chain (i.e the "network") and you're looking to differentiate between your validation error vs an error from the link chain. Does that sound correct?

[wassim-k]

You're absolutely right, I'm referring to network errors as errors with connectivity, like Fetch error or server not reachable errors.
And yes, I would like a simple way of saying for example NetworkError.is(error)

[jerelmiller]

Ok thanks for confirming!

One of the reasons we stopped wrapping errors in v4 was because these error types obscured the source of the error. Worse, "ApolloError" as a name ended up in a lot of error reporting tools, so at a glance it looked like the client was doing something wrong and it was difficult to tell that there might have been user error involved. We felt it better to surface the raw error so that its easier to see the cause of an issue in those error reporting tools rather than obscuring it through a wrapped instance.

I won't lie, I think we'd prefer to keep it this way for that reason. I believe it will be easier to understand the cause of an error by leaving them unwrapped. That said, it should be easy to create your own custom link that wraps these errors with your own class which you can use to differentiate:

// with your own `NetworkError` class defined:
class NetworkError extends Error {
  // ...
}

const wrapNetworkErrorLink = new ApolloLink((operation, forward) => {
  return forward(operation).pipe(
    catchError((error) => {
      throw new NetworkError(error);
    })
  );
});

How would you feel if we provided this guidance in the docs vs adding something to Apollo Client itself?

[wassim-k]

I think that's a totally acceptable solution 👍 Thanks!

[jerelmiller]

Awesome! I'll add this to the v4 milestone so we can track it as a docs task. Thanks!