Errors should not be shared between queries

[tobias-tengler]

We're currently experimenting with Relay's new (experimental) approach to handling queries that contain errors. We've enabled both the ENABLE_FIELD_ERROR_HANDLING and ENABLE_FIELD_ERROR_HANDLING_THROW_BY_DEFAULT feature flag.

Overall it works really nicely and error-handling-clients are definitely the future. There is one major caveat though that still needs to be addressed in our opinion: Errors being shared between queries / fragments.

Note: I'm well aware that this is all experimental at the moment, but I still wanted to provide some early feedback from outside of Meta.

What's the issue?

Let's imagine we have a page-level query like this:

usePreloadedQuery(graphql`
  query PageQuery($id: ID!) {
    productById(id: $id) {
      ...BuyBox
    }
  }`,
  queryRef
);

and we also have another query that is only invoked, once you scroll down the page:

useLazyLoadQuery(graphql`
  query LazyPageQuery($id: ID!) {
    productById(id: $id) {
      ...Reviews
    }
  }`,
 { id }
);

Currently, if the PageQuery is able to fetch the productById without an error, but the LazyPageQuery contains an error for the productById field, both queries will throw an error, since they both read out the errornous productById field.

This is pretty bad, because a temporary failure in the LazyPageQuery could take down other, previously successful, queries like the PageQuery.

Reproduction

https://github.com/tobias-tengler/relay-errorhandling-error

Expected behavior

I would expect that errors aren't shared between queries and fragments. In the above example I would expect only the useLazyLoadQuery hook to throw, since the error is associated with the productById field, which is read out by that hook. The usePreloadedQuery also reads out the productById field, but its query was able to resolve the productById, so it doesn't make sense to also propagate the error there and potentially kill an entire page, if the query itself completed without errors before.
In the same vain, fields that are associated with errors shouldn't update the store. Errornous fields are nulled and just how you would not want to share the error between queries, you also wouldn't want the productById field to suddenly become null in other places that were able to read it out without an error.

[captbaritone]

Thanks for providing the feedback! We've been thinking about this challenge internally as well. Today, every fragment or query rendered by relay render directly from the normalized store. That makes it difficult to show data in different states in different places. In fact, data consistency is a key priority of Relay, so generally this operates as a feature.

That said, we agree that in many cases, it would be preferable to see the stale value than to blow up.

One idea we've considered is to avoid publishing errored fields to the store at all if a non-error field already exists. However this might be misleading in the primary query if you were expecting a change.

Cc @itamark

[itamark]

@tobias-tengler thanks so much for the feedback here. Even though this is experimental, it's really helpful to get this early feedback.

@captbaritone would storing stale value on errored fields (and providing them explicitly alongside the error state as, say, staleValue so engineers can fall back if they want to) - be anathema to Relay's the data consistency tenet?

Edit: I was referring to @catch scenario, but just realized this doesn't solve for the throwing scenario.

[tobias-tengler]

I don't think errors fit Relay's data consistency tenet. I want actual data to be synchronized across surfaces, but I find it very hard to find use cases where one would like an error in a different query to affect successful queries that came before it. If you want to have this behavior, you can implement it in user-space. It should definitely not be the default though. It's not only jarring for the developer, but also for the user. Why did this thing in the sidebar suddenly have an error, if I clicked on this seemingly unrelated thing over here?

Currently you use Relay under the assumption that something will only error if you re-execute it and an error occurs. Other queries being able to trigger an error for previously successful queries, breaks this assumption and is not really intuitive.

@captbaritone My expectation would be that non-error fields in a response update the store, but errornous fields don't.

@itamark The staleValue would also be strange for me. I don't want to have to handle actual values and stale values at every call-site, just because a query somewhere could force my query into an error state.

[itamark]

This makes sense. Just checking - because error handling is meant to introduce correctness as well. I agree that showing prev successful data where a different query had an unexpected error - is the desired experience. What about in a case where the error is legitimate and an expected error occurs, is the expectation that the lazily loaded query will get that data, but the previously successful query will still show the non-error-handled state? For example if a bestfriend deletes their profile or a product becomes unavailable.

As @captbaritone said we're actively discussing this behavior and this feedback is really helpful.

[tobias-tengler]

How do you differentiate between a expected and unexpected error?
We model expected errors in our schema. Though only on the mutation-side atm, because we haven't yet had a case for expected errors during querying.
The top-level errors field is only for unexpected, developer-facing errors in our case. So if there's an error associated with a field it would be unexpexted and should therefore not propagate through the store.

[itamark]

I was referring to my assumption that in a world with more explicit error handling, engineers would throw errors purposefully in order to handle them UI-side. That's what I meant by expected error. Anyway - these are really good points. We're discussing this internally and I'll update - but in the meantime if you had any input on an alternative behavior, we'd love to hear it.

My expectation would be that non-error fields in a response update the store, but errornous fields don't.

Just to confirm - this suggests that we still throw if an error happens in a particular query, but we wouldn't update the store with an error. Rather, we'd keep the previous value intact.

[tobias-tengler]

Yes. If a field is associated with an error, the new value (null) should not replace a previously successfully retrieved value in another query. Hooks attempting to read out a field associated with an error, that are "connected" to the query that produced the error, should throw.