facebook/relay

Thoughts and feedback on the @catch directive

Opened this issue · 1 comments

Note: I'm certain that the @catch directive has had a ton of design work and has been implemented internally at Meta, so it's entirely possible I'm missing something here. I'd love to be wrong and find out that there's a simple solution to this problem.

Our team has recently upgraded to Relay 18 and we've been attempting to consume the new @catch directive as a better way to handle errors. Prior to this we had implemented our own custom functions to attempt to expose the errors information in a GraphQL response to components. The hope was that we'd be able to completely remove those functions in favor of a Relay-native solution.

And in some cases, that's been true. We really enjoy the Result type on fields that are annotated with @catch as it makes error handling easy and functional. There are other places, however, where it feels like more iteration is needed on these new directives for us to be able to fully adopt the feature.

The hope with this issue is to bring up some of these issues and hopefully influence how the @catch directive (and potentially also the @throwOnFieldError directive) behave once they’re in a non-experimental state.

Example schema

Here's a simple schema that we'll refer to throughout.

type Book implements Node {
  id: ID!
  title: String!
  author: String!
  coAuthor: String
}


type Query {
  bookById(id: ID!): Book
}

Using @catch in a schema with non-null fields

I have a component that wants to query the bookById field and display the book information or an error if the book couldn't be retrieved or didn't exist. And I want to make sure I differentiate between those two error states so that I can either show a "Book failed to load" vs as "Book not found" message. My first inclination would be to do something like this:

const data = query BookQuery($id: ID!) {
  bookById(id: $id) @catch {
    id
    title
    author
    coAuthor
  }
}

Now when I render my component I'm able to differentiate my error states correctly.

if(!data.bookById.ok){
  return <div>Book failed to load, please try again.</div>
}

if(data.bookById.value === null){
  return <div>Book not found!</div>
}
... render Book

This is greatly improved on prior error handling in Relay because I'm able to easily differentiate between a field being null because of an error vs being null because of a resolver error. However, this has subtly created a different problem. If any of the sub-fields on a Book fail to resolve, I'll tell the user that we couldn't load the book, when in reality it was only a single sub-field of the Book that we couldn't load. This might make sense for the fields that are marked as non-null (id, title, author), but it doesn't feel like it makes sense for the coAuthor field which is nullable.

If I'm trying to render my Book object and the coAuthor field resolver fails, it will propagate that failure up and take out all other fields on the Book object. This is similar to the null bubbling behavior that GraphQL servers implement. However, with @catch it's different in that the coAuthor field is non-null so this null-bubbling is unexpected.

Now one way to solve this would be to add an additional @catch directive.

const data = query BookQuery($id: ID!) {
  bookById(id: $id) @catch {
    id
    title
    author
    coAuthor @catch(to: NULL)
  }
}

Note: My assumption here is that this component is ok with coercing expected-null and error-null on the coAuthor field. We don't feel the need to tell the user that we couldn't resolve the coAuthor field, we'll just hide that portion of the UI in both cases.

With this query, any failures within the coAuthor field will cause the field to be coerced to null, which our component handles correctly. It will also prevent us from taking out the id, title, and author fields from the Book object if that field fails to resolve.

However, this solution doesn't feel scalable. I'd first have to muddy up all of my queries with a number of @catch directives, let alone have to remember to add it to any new non-null fields I add to this query.

const data = query BookQuery($id: ID!) {
  bookById(id: $id) @catch {
    id
    title
    author
    coAuthor @catch(to: NULL)
    publisher @catch(to: NULL)
    language @catch(to: NULL)
    averageRating @catch(to: NULL)
    ...
  }
}

Using @catch in a fully nullable schema

The situation doesn't change all that much in a fully nullable schema, which our team uses. In a fully nullable schema the query from above might change to something like this

const data = query BookQuery($id: ID!) {
  bookById(id: $id) @catch {
    id 
    title @required(action: THROW)
    author @required(action: THROW)
    coAuthor
  }
}

But the same issues from above exists here where an error on the coAuthor field will cause the entire Book object to return a ResultError.

What could this do instead?

Only bubble errors on non-null fields

Initially, let’s assume we’re using our non-null schema from above where id, title, and author are non-null in the schema.

One option would be for Relay to change its behavior of @catch to only bubble up errors that happen on non-null child fields. This would more accurately represent the error bubbling that happens on the server.

const data = query BookQuery($id: ID!) {
  bookById(id: $id) @catch {
    id       # Errors here cause an ErrorResult above
    title    # Errors here cause an ErrorResult above
    author   # Errors here cause an ErrorResult above
    coAuthor # Any error that happens here would cause this field to be null
  }
}

If my component needed to be able to differentiate between legitimate null and error null for the coAuthor field, I could always put a localized @catch directive on that field to handle that field specifically.

const data = query BookQuery($id: ID!) {
  bookById(id: $id) @catch {
    id       
    title    
    author
    coAuthor @catch # I want to know specifically if this field had an error
  }
}

If we shift to a fully nullable schema (for sake of simplicity let’s ignore the id field for a second), we see some interesting behavior. In that case, the bookById field would only ever be an ErrorResult if that specific field contained an error. That may feel odd for an @catch directive on a parent field, but if it’s combined with the @required directive it starts to visually make more sense.

const data = query BookQuery($id: ID!) {
  bookById(id: $id) @catch {
    id 
    title @required(action: THROW)
    author @required(action: THROW)
    coAuthor
  }
}

However another alternative would be to make this behavior opt-in.

const data = query BookQuery($id: ID!) {
  bookById(id: $id) @catch(nonNullOnly: true) {
    id
    title
    author
    coAuthor
  }
}

I haven't thought through all of the implications of changing this behavior and I'm certain that there are use cases and edge cases where this would be undesirable. But I also feel like the use case from above is fairly common and something that should have a supported solution. I also suspect that there is behavior overlap here with the @throwOnFieldError directive. We’re also hesitant to use that directive as well because of the same granularity problems; we don’t want errors on certain nullable fields to cause an entire query/fragment to throw an error.

Also, I’ve specifically avoided talking about the @semanticNonNull directive as part of this, mostly because I’m trying to keep the discussion simple for now, but also because I believe the @catch directive should work for the above use case without relying on a solution that requires the @semanticNonNull directive.

Hi @ernieturner - thanks! This is rally great feedback. I think some of the expectations here differ a bit from our assumptions when building @catch. You're right that we did a ton of ergonomics and use-case-enumerating work, but of course there's always feedback like this that surfaces what we didn't expect. I'll reread this in a bit and give my thoughts.