commitMutation should return errors in onCompleted

[Eightyplus]

Hi, I'm trying to propagate errors to the users that occurred in a commitMutation
The following documentation does not really provide any info other that an array is returned in the onCompleted callback #commitMutation.

Giving the code example, errors are undefind if an error occur on the server. I'm using a wrong id on purpose (for testing) and the graphql server throws an error.

  const mutation = ... 

  const deleteStuff = (environment, { stuffId }) => {
    const variables = {
      input: { stuffId }
    };
  
    commitMutation(environment, {
      mutation,
      variables,
      onCompleted: (response, errors) => {
        console.log("response", response)
        console.log("errors", errors)
      },
      onError: err => console.error("ERR", err)
    });
  };

Ok, so onError shows on errors, thats fine. But errors are undefind in onCompled. Is this expected behaviour?

[jgcmarins]

errors returned at onCompleted are on developers land.
For example if you have any validation logic on Mutation, at server side, and any of those validations fails, you can return an object like this:

return {
  errors: ['Error message']
}

and then on outputFields:

errors: {
  type: GraphQLList(GraphQLString),
  resolve: ({ errors }) => errors,
},

On client side, those errors returned by GraphQL Mutation outputFields will be available as parameter at onCompleted callback.

[Eightyplus]

Thanks for the fast reply!
To make sure I understand, outputFields goes into my schema, or what is it?

[Eightyplus]

This would make it part of the response and not errors.

[josephsavona]

What version of Relay are you using? Can you provide a sample GraphQL network response?

errors returned at onCompleted are on developers land.

Kind of - the second (errors) argument to the onCompleted callback comes from the errors field of the GraphQL payload (not data.errors). This argument is only populated if the GraphQL response actually has a top-level error value.

[taion]

Also, if you're using react-relay-network-modern (without noThrow) or following the recommendation to throw in the network layer per #1913, then you'll also see the same behavior where the errors won't make their way to onCompleted, as the network layer will rethrow the payload error as a request error, for the reasons outlined in the linked issue.

[Eightyplus]

We are already dealing with Relays lack of implementing the GraphQL standard by mapping errors into data like so, but this is inside the fetchQuery.

Also, if you're using react-relay-network-modern (without noThrow) or following the recommendation to throw in the network layer per #1913, then you'll also see the same behavior where the errors won't make their way to onCompleted, as the network layer will rethrow the payload error as a request error, for the reasons outlined in the linked issue.

So how do I avoid avoid this, use react-relay-network-modern or do not throw errors from the server? But how will errors propagate to onCompleted?

[Eightyplus]

    "react-relay": "^1.6.2",
    "relay-compiler": "^1.6.2",
    "relay-mock-network-layer": "^1.2.0",
    "relay-runtime": "^1.6.2",

[josephsavona]

If your server returns responses per the GraphQL specification ({data, errors} object), those errors will be available in the onCompleted callback of commitMutation. The main reason that would not happen is if you are doing something to intercept that top-level errors field:

• If you're throwing if errors is present, then onError will be called instead of onCompleted.
• If you're mapping the errors data into the query payload (so that it becomes part of data), then obviously Relay won't see any top-level errors and the onCompleted errors argument will be undefined. You can just access the errors from the payload in that case (presumably you've defined an errors field in your schema that you can select from?).

[josephsavona]

We are already dealing with Relays lack of implementing the GraphQL standard

Just to be clear, Relay does conform to the GraphQL spec and best practices - but I can understand confusion here, as the best practices for error handling in GraphQL have emerged over time. The GraphQL errors field is intended for truly exceptional errors such as an invalid query or variable value: things that are not reasonably expected to occur in an application. Invalid user input or a backend service being unavailable are reasonable errors that are expected to occur (hopefully infrequently for the service down case!). If information about errors is required by the application to function, I suggest following @leebyron's advice:

GraphQL errors encode exceptional scenarios - like a service being down or some other internal failure. Errors which are part of the API domain should be captured within that domain.

link

a common best practice has been to include user errors as part of the schema itself so they can contain domain specific information.

link

There's a great writeup by @alloy on this approach as the final suggestion in http://artsy.github.io/blog/2018/10/19/where-art-thou-my-error/#Make.exceptions.first-class.citizens.of.your.schema.

[Eightyplus]

Ok, I cloned the ToDo MVC example

https://github.com/Eightyplus/relay-examples/commit/1d591f5cdec206c327b1078e312ee95446b251fb

So I guess I now have to find you where the errors are consumed 😭

[Eightyplus]

We are already dealing with Relays lack of implementing the GraphQL standard

Just to be clear, Relay does conform to the GraphQL spec and best practices - but I can understand confusion here, as the best practices for error handling in GraphQL have emerged over time. The GraphQL errors field is intended for truly exceptional errors such as an invalid query or variable value: things that are not reasonably expected to occur in an application. Invalid user input or a backend service being unavailable are reasonable errors that are expected to occur (hopefully infrequently for the service down case!). If information about errors is required by the application to function, I suggest following @leebyron's advice:

First of all, why are Relay not just passing the data AND errors forward, huh? Could Relay Modern live up to its name? The noThrow flag does not do this, it result in data=null.

We have a quite complex apps where we have a large three structure and multiple error scenarios, all errors are perfectly coded like:
https://www.apollographql.com/docs/apollo-server/features/errors.html

But we had to do this in fetchQuery, just waiting for it to be fixed.
#1913 (comment)

GraphQL errors encode exceptional scenarios - like a service being down or some other internal failure. Errors which are part of the API domain should be captured within that domain.
Yeah, but I want my coded errors separated from data. Besides, our domain is rapidly expanding, it is way easier to separate the two.

@leebyron left the building, now it is up to you to give this
https://facebook.github.io/graphql/June2018/#sec-Errors

a common best practice has been to include user errors as part of the schema itself so they can contain domain specific information.
Done that, don't like it.

There's a great writeup by @alloy on this approach as the final suggestion in http://artsy.github.io/blog/2018/10/19/where-art-thou-my-error/#Make.exceptions.first-class.citizens.of.your.schema.

This is a cute little app, they could have used REST 😂

[Eightyplus]

Ok, I should stick to the topic/issue, and comment on other issues elsewhere.
I'll investigate commitMutation further in our scenario and let you know if I find an actual issue.

[Eightyplus]

Ok, so I finally managed to track down the issue. Thanks for hinting me in the right direction!
Mapping errors into data (somewhat like so: #1913 (comment) ) made errors undefined in our case in onCompleted callback.

I have fixed the mapping and errors appear in onCompleted

[Eightyplus]

However, it would be really nice if this was fixed: #1913

[alloy]

Seems like this can be closed then, as the original issue is not caused by Relay?

Ok, I should stick to the topic/issue, and comment on other issues elsewhere.

I’m not in any way speaking for the Relay team–this is just my personal OSS communication experience–, but yeah that’s probably helpful to your own cause. E.g. I’m not even sure what you mean by: “Could Relay Modern live up to its name?”, but it comes across as spiteful when in reality you’re asking somebody to help you with your issue when they have no obligation to do so.

[alloy]

Ah, looks like it was closed while I was typing up my comment 👍

[Eightyplus]

I sorry, no one has obligations to do so. New technology seems to give head scratching (with early adoption?), and Relay is no exception.

I think my personal frustrations are with the documentation. Something appears supported when they are not, and basically I have to implement stuff with trail and error mindset. Mapping errors into data was not our first choice and we're still hoping for this to be solved.

And, it goes both ways, the community feedback is just as resourceful as your help is to us.
Thanks @josephsavona @alloy @taion

[alloy]

I sorry, no one has obligations to do so.

👍

New technology seems to give head scratching (with early adoption?), and Relay is no exception.
…
I think my personal frustrations are with the documentation.

Agreed, you're not alone on that one!

Luckily after a lot of work by the Relay team and people in the community we're seeing a lot more movement in addressing non-FB needs; so welcome to the club and please do file PRs with doc improvements as you come across them! 🙏