Add support for errorsCallback so we got errors detail on server …

[ohardy]

I added a test for this but sinon must be used for this test to really work. Didn't know if you are OK to include sinon and sinon-chai ?

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[leebyron]

Two primary points of feedback:

First, please ensure this change is well scoped. This makes some seemingly unrelated changes to the package.json file.

Second, could you help me understand the motivation for this change? This seems directionally good, but I'd like to better understand what you're trying to accomplish by proposing this, since there may be other solutions to consider.

[leebyron]

Also please see and mirror existing test formatting rather than including new test mechanisms

[cdonohue]

This could be extremely helpful.

If graphql is hooked up to a database to make a query, wouldn't you want to intercept any errors and handle them accordingly? Currently, you are only able to format the errors, which passes them in a 200 response with the errors inside an errors property.

And, when promises are used, every request will go through as a success unless there is a graphql syntax error, wrong verb used, etc. These errors are specific to graphql, but there is no way of handling anything outside of graphql.

So, in the database example above, How would one go about sending back anything other than a 200?

The only way I can see to accomplish this would be to have something check the errors property and condition accordingly.

Thoughts?

[cdonohue]

@leebyron @ohardy ?

[ohardy]

@cdonohue For me it's OK now with formatError, not you ?

[cdonohue]

I guess I'm just looking for a solution where errors can be caught and the response be set accordingly.

Take the example above. If the graphql backend is connected to a database and for some reason, the database is down (problem outside of graphql). When the graphql query/mutation returns, it will still send a 200 status back to the client. You can format the errors all you want, but you are still dealing with a SUCCESS.

This also means that you would have to handle your client requests a bit differently for express-graphql:

axios.post("/path-to-graphql", { query, variables })
  .then(response => {
    if (response.data.errors && response.data.errors.length) {
      throw response.data.errors[0];
    }
    // --> process data
  })
  .catch(error => {
    // handle client errors
  })
;

To me, the catch should do it's job without having to check `response.data.errors' and throw accordingly.

Maybe this should go in its own issue? But I already saw that #36 has been closed out.

How are others handling this case when errors happen on the server and 200 status codes are being sent?

[leebyron]

Returning 200 is intentional in the case that a nullable portion of a query failed, but other parts of the request succeeded. Express-graphql should return an error code if the whole query fails

[cdonohue]

Thanks for clarifying, @leebyron. I'll revisit my example and double-check everything.

[leebyron]

Closing this as I think we have better solutions for the two motivating use cases:

1. You wish to format error output before they get to the client. Options can include a formatError function which does exactly this.
2. You wish to log errors as they occur for metrics purposes. express-graphql is probably the wrong layer of abstraction to do such a thing, this isn't implemented yet, but there is a tracking task for this very purpose at Execution & Error Logging graphql-js#284.