-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GraphQL error handling in Relay Modern #1913
Comments
Looks like there is an error param for the onCompleted callback: https://github.com/facebook/relay/pull/1938/files Don't need to throw in the relay network layer anymore. |
Not quite the issue – the problem is actually in QueryRenderer. |
Yeah, I have found that |
That's not what I'm talking about. Of course errors in |
Agreed, it would be very helpful to have a way to view the GraphQL errors within the QueryRenderer render method. |
This definitely seems like a bug, but I'm not sure what the best resolution is here, API-wise. Should the I don't mind submitting a PR to fix it either way, but I'd like some advice on which direction to go in. |
So can anyone clarify this? Whats the Relay way of handling this? Is that a bug and Relay is supposed to listen on the 'errors' field of the mutation response? |
Still trying to figure out how to handle "network is unavailable" in Relay without it crashing in production - I have the error in fetchQuery but don't know what to do to get it back to the Component to display. Is everyone here stuck on the same thing or can someone point me to a resource? |
Seems like there's a lot of confusion on what @taion is talking about in this thread. I think I'm hitting the same issue @taion is and wanted to illustrate with some examples. Basically, I'm passing GraphQL errors back to Relay and they're getting swallowed by QueryRenderer. Something like:
Then in the render function of For the
from the server, then I get my error to come through but it gets formatted and aggregated into some Relay Modern error:
So it seems like I can access my errors if I force my data to be |
My current workaround until fixed for this is to return explicit data: null in network: if (result && result.errors) {
return {data: null, errors: result.errors};
} |
I´m having the same problem. I need to query for a item that was deleted from another user at the server side database. I receive
Although the client application behavior is OK (no error in client application), there must be a way to check for the GraphQL transaction response. Today How can I get this GraphQL transaction error detected ? Here is my environment code:
|
@renatonmendes How do you resolve? |
I haven´t solved. What I did is to throw an error on server side based on database layer return and on client side I do check for data and errors field on a normal QueryRenderer return. If there is no data, then it´s an error. If the error field contains messages, then its a error. If there is data and error field is empty, then I have a normal operation and I proceed. |
@renatonmendes Thank you for your answer.
But i have on error on TypeError: errors.map is not a function (like if errors would be an array and not object, but if i put that object in array is not a very clean solution) |
Simple workaround for Relay bug (see: facebook/relay#1913). I have no idea how to write tests for this part of application. I would prefer to merge this and think about later since we will need to refactor it a little bit because of other tests...
Simple workaround for Relay bug (see: facebook/relay#1913). I have no idea how to write tests for this part of application. I would prefer to merge this and think about later since we will need to refactor it a little bit because of other tests...
For me setting
This way the partial errors are accesible inside my components. To accomplish this I created a new error type in the GraphQL schema. I use
I added an
The
You can query the
If you want to handle server errors directly in your component you can add an
You can call it in the fetchers fetch function like following
|
Leaving a link to @josephsavona’s explanation here for posterity. |
I just ran into this and was very confused as a user. I understand the trickiness in regards to mapping the concept of partial data with Is there something I'm missing here? It seems this behavior can be rectified independent of the discussion of callbacks. |
any update for this? for now I add |
I have tried the workaround described above (passing null to data) but it didn't work for me with Relay versions 1.5.0 and 1.6.0. It turns out that error handling in QueryRender was completely broken since v1.5.0, see #2385. I rolled back to 1.4.1, and the workaround worked. |
@jamesone will be great if you take care of this problem. |
On it 👍 @nodkz I'll keep you in the loop. I was able to fix the issue, I'll let you know once I'm done and create a PR on the repo. |
@nodkz relay-tools/react-relay-network-layer#61 - Check it out and let me know what you think |
I decided to go other way - I dont think import { commitMutation } from 'react-relay'
// inspired by https://github.com/relay-tools/relay-commit-mutation-promise
// check https://github.com/facebook/relay/issues/1913 for more info
export default function commitMutationPromise(environment, config) {
return new Promise(function(resolve, _reject) {
commitMutation(
environment,
Object.assign(
{},
config,
{
// onCompleted is called when:
// - data is not null (it doesn't matter if errors field is empty or not)
onCompleted: function(data, errors) {
// restore original response
resolve({ data, errors })
},
// onError is called when:
// - data is null
// - errors array is not empty
onError: function (errors) {
// TODO: can errors be an js error object? reject then?
// never reject
// (reject is pointless because "partial success" feature is one of the founding principles of GraphQL),
// restore original response
resolve({ data: null, errors })
},
}
)
)
})
} |
Simple workaround for Relay bug (see: facebook/relay#1913). I have no idea how to write tests for this part of application. I would prefer to merge this and think about later since we will need to refactor it a little bit because of other tests...
Please fix this, this is annoying! |
I have somewhat older version of Relay Modern (v.1.6.0). This is how I fixed the issue. When graphql sends error message, it has a default format like following,
You can modify it to simple format by adding After doing this, Now graphql will send error message in following format. Now
Now go to your front-end side code, open
Now, It will kind of look like this:
Note:- Don't catch this error. This thrown error will get transmitted to QueryRenderer where it will be available as
|
I have been wondering if there would be a way to add support for additional patterns around error handling by adding a few new APIs. At a high level, the issue seems to be that relay does not have a way to represent non-critical errors, or errors which are not intended to bubble all the way to the root of the query without introducing/representing those errors at the schema level. While I understand the value and benefits of properly representing error states you want to handle in your schema, and treating them as data rather than exceptions, many backend frameworks and patterns have been built around the idea of partial responses, and allowing errors in nullable fields to automatically be captured in the root error array with a I think eventually there will be some new backend patterns that will make this easier, but the current state of the world seems to be that there are lots of real world examples of graphql schemas that use patterns with partial + errors in responses. Here is my ideal solution:
The end result of this would be that you could create error boundaries around components that consume a fragment and handle errors for more granular parts of a query without having to break down your query into smaller requests to get more graceful degradation. I am pretty new to relay, so my understanding of relays concepts are pretty rough, but I think adding something like this could be done in a way that adds minimal overhead to existing patterns and would create something that is completely opt-in, while still providing a solution to a large set of problems around error handling. |
Have you seen @alloy's blog post in https://artsy.github.io/blog/2018/10/19/where-art-thou-my-error/ ? Not sure if this is what you're thinking about w/r/t "very manual process". One way to think about this – the profusion of custom types reflects the lack of generics in GraphQL, but that lack of generics is reasonable if you think of generics as mostly to make life easier for programmers, and as something that can be implemented inside your (programmatic) GraphQL schema, and merely reified on the actual emitted schema itself. But with all that, then it might not be terribly bad to manually unwrap fields with errors that you intend to handle. |
Yup, I love that post. It has a great breakdown of these problems. "very manual process" definitely was meant to encompass the work involved around setting up all the types/interfaces/unions required to represent these errors, but even after you've implemented most of the pieces in that blog post, without a lot of additional work, in most backend frameworks I've seen, its still relatively easy to end up with errors in your root level errors object. If you follow the advice in that blog post and have confidence most errors will come through as typed data in your response, you can always use one of the strategies mentioned above to bubble any remaining errors up to the root and have it fail at the query level. Every framework I've worked with (but I haven't looked at too many yet) default to having errors in a resolver will automatically fall back to making the closest nullable parent field null and inserting the error into the root errors list. For most applications I've worked in, developers try to handle the obvious error cases like a failed request, but there are almost always cases that are not explicitly handled, and explicitly handling every possible error case is often not practical. I am not trying to suggest that using unstructured errors like this is a good pattern, just that is a pattern which is common, and even in backends that try to implement better error handling by baking it into the schema are still likely to have some errors fall through the cracks and fall back into the root errors array. In these cases, I would personally prefer to have a way to extract those errors when retrieving the data/fields which the errors correspond to, rather than being forced to handle them at the root level. |
Right, so there are a couple of things going on here. The first is that GraphQL's default null semantics are... "interesting". If you mark a field as non-nullable, but it errors out, it's game over for all its parents up until you get to the first non-nullable one. There's nothing you can do about that if your schema is set up that way, hence the suggested best practice of making everything nullable. In terms of implementing the "either"-based approach on a GraphQL API, if you're using the programmatic API, it's really not so bad, if you're so inclined. You could do something like: fields: () => wrapWithOrError({
myField: {
type: MyType,
resolve: (obj) => whatever,
},
}); and in Then on the client side, if you try to select on a fragment for just the non-error branch of any of the Otherwise, with Relay (but similarly with any other GraphQL client), for most of these error conditions, if you really want to handle them gracefully, then you can also effectively (mostly) ignore request-level errors, and just blow up on null dereference in the relevant components. |
I think the issue there is that it assumes that a null means there was an error, which is often not the case, and would effectively mean you your schema can either have valid nulls or errors, but not both. It seems like you are suggesting that there is are patterns you can use in your schema/server to represent any error condition in you schema. I don't disagree, but I also don't think it is a cleaner solution. Having every field wrapped in a union makes your queries and you schema a lot more complicated, many schemas already exist, and use the simpler pattern of nulls + top level errors, and migrating these schemas to properly represent all their error cases in the schema might be a lot more complicated than exposing a way to access these errors in relay. I was not trying to advocate for this pattern to be the preferred way to handle errors in relay, just hoping it would be possible to expose these errors for users who would like to be able to access them somewhere outside of the root level query or the network layer. But, after thinking about the problem a little more, I think all this is actually pretty manageable without anything in relay itself. It should be pretty straight forward to basically map errors to object ids + fields at the network layer, then write some hooks that wrap the relay hooks to check for errors when unmasking a fragment. |
On valid nulls or errors – without checking into the errors sub-object, you wouldn't be able to have them on the same field, yeah. As a rule of thumb, we generally try to limit our use of valid Oh, right, the unions would only help in a "neat" way when used at fragment boundaries. They wouldn't help for individual scalar fields that throw... at the very least, you'd have to box the value, and it wouldn't be transparent to the client developer. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Hey @hayes, were you able to make this solution work for you? Would you happen to have any more details or example code? |
@wyattanderson no, I ended up going in a different direction, and finding a better way to add errors into the schema itself. I didn't end up trying what I suggested above. I ended up just writing this https://giraphql.com/plugins/errors, but that isn't a good solution unless your API is written in typescript. |
There seems to me like Relay proposes a mechanism for reporting and handling field errors that is different than the reccomendation in the GraphQL specification. This mechanism is incorporated to Relay by design: Relay expects that a GraphQL server will expose field errors through wrapper types around field result objects, where the enclosed result objects are unions that can supply either the actual value of the field or the error object. This is stated in the Given that, I believe that there is not much that I can do about it than either leave it or take it unless Relay provides the functionality. I am initating myself into this framwework and I am experimenting with the GitHub GraphQL service, which seems to adhere to the GraphQL specification versus the error handling mechanism expected by Relay. In any event, I want to continue exploring this framework for the good or for the bad, so I have proposed myself a compromise solution to this question in this example . |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I think error handling is a little bit weird.
Contra the example in https://facebook.github.io/relay/docs/network-layer.html, it looks like I need to actually
throw
anError
in the event that there are any query errors. I can't just pass through thegraphql
object with theerrors
property.This is because in https://github.com/facebook/relay/blob/v1.1.0/packages/relay-runtime/network/RelayNetwork.js#L197-L208,
normalizePayload
only throws an error ifdata
is nully.But the forward
throw
branch there is the only code path that will triggeronError
for the request, and correspondingly the only path that will hitonError
in<QueryRenderer>
per https://github.com/facebook/relay/blob/v1.1.0/packages/react-relay/modern/ReactRelayQueryRenderer.js#L205-L212.Otherwise we hit
onNext
per https://github.com/facebook/relay/blob/v1.1.0/packages/react-relay/modern/ReactRelayQueryRenderer.js#L229, and will always seterror: null
on thereadyState
that gets passed to therender
callback.This is weird. Am I missing something, or are things supposed to work this way?
The text was updated successfully, but these errors were encountered: