Relay: [Modern] commitUpdate onError never get fired if mutation payload "data" not null

[nosykretts]

Right now im using Graphcool, I dont know if this Graphcool API bug or relay modern bug, or Im missing something .

// PAYLOAD A copied from devtool network tab
//This payload commitUpdate onError not firing
{
  "data":{ //this data not null
    "deleteUser":null 
  },
  "errors":[
    {
      "locations":[
        {
          "line":4,
          "column":3
        }
      ],
      "path":[
        "deleteUser"
      ],
      "code":3008,
      "message":"Insufficient permissions for this mutation",
      "requestId":"cj37m07vvigq20143c19ogqq3"
    }
  ]
}

// PAYLOAD B
//but this payload commitUpdate onError work as expected
{
  "data":null, //this data is null
  "errors":[
    {
      "message":"Variable '$input' expected value of type 'DeleteUserInput!' but got: {\"id\":\"cj37ez4c1fxxu0143yfzynp1e\",\"clientMutationId\":\"abcd\",\"wohoo\":\"A\"}. Reason: Unknown field 'wohoo' is not defined in the input type 'DeleteUserInput'. (line 2, column 3):\n  $input: DeleteUserInput!\n  ^",
      "locations":[
        {
          "line":2,
          "column":3
        }
      ]
    }
  ]
}

const mutation = graphql`
  mutation DeleteUserMutation($input: DeleteUserInput!){
    deleteUser(input: $input){
      #deletedId
      viewer{
          allUsers(first: 1000){
            count
          }
      }
    }
  }
`
function commit(environment: any, userId: string, viewerID: string) {
  return commitMutation(environment, {
    mutation,
    variables: {
      input: {
        id: userId,
        clientMutationId: 'abcd',
        wohoo: 'A'
      }
    },
    updater: (store) => {
      const payload = store.getRootField('deleteUser');
     // This is my temporary fix
     // if(payload === null){
     //   return
     // }
      const deletedId = payload.getValue('deletedId')
      sharedDelete(store, 'UserList_allUsers', viewerID, deletedId);
    }
    onCompleted: (response) => {
      console.log('Success!', response)
    },
    onError: err => {
      //with Payload A. This not getting fired.
      console.log(err)
    }

  })
}

CC: @graphcool , @marktani

[brysgo]

I had to check for errors in the network layer and throw. Not sure if this is by design.

[JenniferWang]

This is a KP and we have an internal task tracking this problem. This is because Relay Modern enforces the GraphQL response spec strictly http://facebook.github.io/graphql/#sec-Response which allows partial data to be included in the response.

[nosykretts]

im sorry @JenniferWang but what is "KP"? I dont understand 😄
So this relay modern bug?

I have sent message to Graphcool team about this problem and linking this issue to them via slack. But still no response from them.

[JenniferWang]

@nosykretts Oops, KP = known problem
The GraphQL language spec allows partial data with an error field/message; thus common onError and onSuccess callbacks on client side are not the best APIs to express this -- you can only call either onSuccess OR onError but not both. Currently Relay choose to call onSuccess whenever there is partial data (might need to double check). Go back to your example,

"data":null, //this data is null

is not considered as partial data, but

 "data":{ //this data not null
    "deleteUser":null 
  },

is considered as partial data. This actually happens occasionally in mutation payload and we haven't decided if we want to treat it as a special case since Relay modern this is not part of the spec.
An alternative way is to have onResponse instead of onSuccess + onError, but we are not very sure about this currently.

[nosykretts]

what onSuccess callback? what you mean is onCompleted right?
My use case is I want to know what my error code and error message inside my mutation, because onCompleted callback only return the "data", not errors array.

unless as @brysgo suggested check for errors in the network layer. Im thinking, should I modify my fetch like this? @JenniferWang is this safe?

 return fetch(__GRAPHCOOL_URL__, {
   //...
   //...
  }).then(response => {
    return response.json()
  }).then(responseJson => {
    if (responseJson.errors) {
      return {
        data: null,
        errors: responseJson.errors
      }
    }
    return responseJson
  })

[josephsavona]

and pushed the spec forward with location/component aware errors

@unirey This is the key point. The spec as it stands today makes it challenging for frameworks to provide flexible error handling. In Relay Classic we took an opinionated stance on this that differed from the spec, and with Relay Modern we've tried to err on the side of being spec compliant in order to see where the spec could improve.

[josephsavona]

So I agree; rather than address this in Relay, we should address this in the spec.

[nosykretts]

Thanks @unirey and @josephsavona for the comment. So this is spec limitation I expect this problem will be resolved for bit longer, for the work around @josephsavona can you help me on my last comment? is it good or bad?

[josephsavona]

@nosykretts yeah that's a good workaround.

[josephsavona]

My general advice for applications is, for now:

• Establish and enforce a convention in your schema for the values returned in the errors property, and when an error is considered critical enough to return null for data (When nothing can be meaningfully be rendered anyway). For example, for critical errors you might choose to still return data but have errors contain an object with {critical: true, message: '...'}, or you might choose to make data be null. The important part is consistency.
• Take advantage of this in your network layer. Check the data/errors property for the presence of critical error based on whatever approach you follow from the previous step, and if a critical error is present return {data: null, errors} to Relay. Otherwise pass through the data as-is.

[nosykretts]

Thanks for the advice it realy helpfull. I dont know if this issue should be closed or keep it open, for me this solved for now. @josephsavona you can close this issue anytime.

[aizatto]

I used the fetchQuery example, and have left a reference for other people:

function fetchQuery(
  operation,
  variables,
) {
  return fetch('/graphql', {
    method: 'POST',
    headers: {
      'Content-Type': 'application/json',
    },
    body: JSON.stringify({
      query: operation.text,
      variables,
    }),
    credentials: 'same-origin',
  }).then(response => {
    return response.json();
  }).then((json) => {
    // https://github.com/facebook/relay/issues/1816
    if (operation.query.operation === 'mutation' && json.errors) {
      return Promise.reject(json.errors);
    }

    return Promise.resolve(json);
  });
}

export const environment = new Environment({
  network: Network.create(fetchQuery),
  store: new Store(new RecordSource()),
});

[hisapy]

@aizatto solution works great. Moreover, if you have some data even when you have errors, you can just reject the whole json

Promise.reject(json)

[alloy]

Going to close this in favour of #1913.