Comments: Where art thou, my error?

[alloy]

[taion]

We're working to implement something like this, and I have a couple of thoughts here.

The former is that, on mutations, it might be more interesting to put the union on the root. For example, instead of having a submitOrder mutation that has a top-level orderStatusOrError field, it may make more sense to have a submitOrderOrError mutation where the payload is of type SubmitOrderPayload | OrderError. This is because, for the most part, with a mutation, we're concerned about the failure of the root mutation operation itself, rather than errors with fetching any particular payload field in the mutation. For example, we may not care about any of the payload fields in a setPassword mutation, but this mutation may still fail!

For the latter, there's an interesting open topic in just what the errors should be. The example in the blog post has two kinds – HTTPError and OrderError, which are interesting and different. The former corresponds to something in the back end (presumably), while the latter corresponds to something specific to the semantic operation. I would argue that, for mutations, there's a third kind of error that makes more sense. Something like a generic UserInputError that's specific to the category of operation but does not make assumptions about the back end or about the specific mutation would enable patterns like automatically wiring up server-side error handling to forms.

One last thought – in general, operations can fail with multiple errors, e.g. multiple fields on a mutation input can be invalid. There's a question, then, of how to handle this sort of generic error handling as well.

[mrtnzlml]

Hi @alloy! We are playing with this idea as well however, I noticed one issue and not sure how to deal with it. Let say we have a query returning connection (with edges and nodes). So you can easily do this:

allLocations( ... ) {
  ... on LocationConnection {
    edges { node { id } }
  }
  # some errors here
}

However, you cannot do it with @connection which is quite common when using, well, connections:

allLocations( ... ) @connection(key: " ... ") {
  ... on LocationConnection {
    edges { node { id } }
  }
  # some errors here
}

It results in this Relay Compiler error:

- Expected field `allLocations: DangerZone_Locations` to have a edges selection in document `LocationsPaginated_data`.
  
  Source: locations/LocationsPaginated.js (7:9)
  6:         ) {
  7:         incrementalPagination: allLocations(first: $count, after: $after)
             ^
  8:           @connection(key: "allLocations_incrementalPagination") {

It is because "currently @connection requires that the edges field be a direct child" (see). How did you solve this issue? It seems logical to return connection or an error since we are fetching data in the allLocations query (resolver). But then you cannot paginate it using @connection.

[alloy]

@mrtnzlml Hmm, good question, perhaps this wasn’t an issue with the old Relay version we’re stuck on 🤔 I theoretically don’t see a reason it couldn’t work, though, so perhaps this requires an update to Relay’s connection handler?

[alloy]

@taion Sorry, only just now notice your comment.

The former is that, on mutations, it might be more interesting to put the union on the root. […] This is because, for the most part, with a mutation, we're concerned about the failure of the root mutation operation itself, rather than errors with fetching any particular payload field in the mutation

Good point. The thought was to have a consistent pattern and I guess that because most (if not all) of our mutations have a single root field I didn't consider that further.

So a field selection failing would still yield an error in the fieldOrError, but a mutation failure (such as input) would be at the root. Makes sense 👍

I would argue that, for mutations, there's a third kind of error that makes more sense. Something like a generic UserInputError that's specific to the category of operation but does not make assumptions about the back end or about the specific mutation would enable patterns like automatically wiring up server-side error handling to forms.

I agree, but I see that more as a best-practice spec, akin to Global Object Identification and Connections, in that it’s something that’s possible on top of this more generic abstraction.

One last thought – in general, operations can fail with multiple errors, e.g. multiple fields on a mutation input can be invalid. There's a question, then, of how to handle this sort of generic error handling as well.

I wonder if this hypothetical input error spec could have a single error object which would follow the input type’s shape?

[mrtnzlml]

Hmm, good question, perhaps this wasn’t an issue with the old Relay version we’re stuck on 🤔

@alloy How old version do you use? 😊 This comment is from Apr 2017 and it's still the case. I didn't know about this limitation before I tried it after your article but seems like this is how @connection works for quite a while. So this pattern (with @connection) works for you and it just needs some more work in the current Relay version to return the support?

[taion]

@alloy

Yup! We have a lot of

union CreateWidgetOrErrorPayload = CreateWidgetPayload | InvalidInputError | _ClientError__INTERNAL

(the last one is a fallback for an unhandled error.)

We ended up not dealing with multiple errors in the general case. For us they can only come up for invalid input fields, so we just have

type InvalidInputError implements ErrorInterface {
  message: String
  fields: [InvalidInputErrorField!]!
  _originalError__DEBUG: JSON!
}

[cowboy]

In case it helps anyone, we've created a helper function for the client like so:

const dataByTypename = data => data && data.__typename ? { [data.__typename]: data } : {}

That allows us to effectively "de-unify" the union type into its component types, allowing us to write code like this:

const OrderStatus: React.SFC<Props> = ({ order: orderStatusOrError }) => {
  const { OrderError, OrderStatus } = dataByTypename(orderStatusOrError)

  if (OrderError) {
    return (
      <div className="error">
        {OrderError.code === "unpublished"
          ? "Please contact gallery services."
          : `An unexpected error occurred: ${OrderError.message}`}
      </div>
    )
  }

  return OrderStatus.deliveryDispatched ? 
    ? "Your order has been dispatched."
    : "Your order has not been dispatched yet."
  }

[petergaultney]

I really appreciate this post, as it covers a lot of the ground I've been thinking about recently.

The conclusion goes somewhere I can't follow, though - I think that maintaining unused keys (e.g. error alongside myFoo) is a much cleaner practice than using Unions, because most front end languages simply don't have clean/healthy syntax for pattern matching/union disambiguation.

In your examples, there's now a reliance on an implementation detail (if (fooOrError.__typename === 'FooError') ...) rather than something explicit in the return type (if (foo.error) ...).

In any case, I do really appreciate this thoughtful exploration of the problem space, and it's helped me clarify my thoughts on things even if I partly disagree with your conclusions. :)

[alloy]

@petergaultney Glad it could be of assistance in any way 👌

I do want to push-back on the notion that relying on __typename is relying on an implementation detail, though. That field is a first-class way to identify a GraphQL type and thus is a reserved field name–which is what the dunderscore implies, not that it is private. It also maps cleanly to the member types of the union you select in your query.

[petergaultney]

yes, that's a good point, thank you for the correction. I've been assuming it was an Apollo implementation detail, but I stumbled across that section in the GraphQL docs maybe a couple of hours after writing that.

I've not been thrilled with the Apollo usage of __typename, which probably causes me to be biased against it. A lot of boilerplate to get rid of or inject __typename gets written when you've got a CRUD app that needs to use optimistic client-side responses. But being in the spec certainly makes the tradeoff more compelling.

Incidentally, this all reminds me that I wish GraphQL supported the concept of 'absent keys' rather than simply having 'nullable fields'. Not only would that address your concerns about the unneeded keys, but it would (in my view) make for a cleaner static typing approach in Flow/Typescript. I suppose catering to the client language would be a true implementation detail, though. :)

[alloy]

I’m not familiar with creating optimistic responses with Apollo Client, this is what typical usage looked like of our Relay/TS setup where type-narrowing made things pretty seamless: https://github.com/artsy/emission/blob/9ebf2fcf7543ae07d4eaadc7d2b208da4dfce8ec/src/lib/Scenes/Artwork/Components/ContextCard.tsx#L138-L157

[rramaa]

@alloy I think this approach is making the client coupled to the resolvers. If the resolving logic of the types change, then the client would have to change the query to use those errors. I am more interested in the createFragmentContainer which I was in the example. What if the the component gets the data/error as props(which would be automatically injected into the component through the container). The path which errored is already in the response(as per Apollo Server 2 implementation). Now even if the resolving logic changes, the key which has errored will always be provided to the component.

[alloy]

True, to some degree. You don’t need to over-expose details and you can use an abstraction for error codes (imo always using HTTP status codes for that is fine) which you can keep using regardless of backing.

More importantly, my experience thus far is that it is unlikely in reality that these will change, so I’d prefer modelled/typed errors over stitching together data on the client. If you know you will have a different scenario, then of course do what works best for you.