restore proper display of form field errors from GraphQL requests #162
Conversation
|
I dig it, i'll merge tonight when i get a chance for a full review. In the mean time, since you're knee deep in this stuff right now, what's your take on the best GraphQL error handling pattern? Since writing this, my thinking has kinda to where the original GraphQL errors make it to the client, but then client-code would replace the ugly stuff with an opaque error. I still like sending down a stringified error object & duck typing it to see if it's from my code (expected) or GraphQL (unexpected), but that's just because I haven't seen a better pattern emerge from the community yet. |
| const context = {authToken, ...newContext}; | ||
| const result = await graphql(Schema, query, null, context, variables) | ||
| const {error} = prepareClientError(result) | ||
| res.send({data: result.data, error}) |
There was a problem hiding this comment.
sending an error field in every payload is no good since we want it as short as possible. Rather it should rest on the client to know how to interpret the payload.
There was a problem hiding this comment.
so you're saying only assign error if it's not null? makes sense
|
I'm not experienced enough to have an opinion on GraphQL error handling, this is the first project I've used GraphQL on :) Debugging this duck typing was definitely confusing but I can see the motivation for distinguishing between GraphQL and other errors. I don't understand yet if there are any GraphQL errors that would be due to user input and not programmer error, and if so, if there's any systematic way for the client to present those errors to the user in a clear way. |
|
@mattkrick what do you want me to change on this one, just only send |
|
Yep, same as I do here (https://github.com/ParabolInc/action/blob/master/src/server/graphql/httpGraphQLHandler.js#L10-L17). Just ignore the whole mapping junk, meatier is on an older version of GraphQL that doesn't require it. In another month or 2 I'll sunset meatier in favor of Action. Since I'm being paid to keep Action maintained it'll be a lot fresher, whereas a lot of the things I learn from it I don't bother back propagating to meatier. For example, react-look, webpack-assets in compile time, cashay, rethink-migrate, etc. The only part of the stack that differs is that Action uses auth0 instead of my homemade auth. Maybe later we'll switch back, but for now it's super convenient outsourcing the part of the app that's attacker-prone & it's free for small stuff, so I have no worries promoting it in my code. |
Seems something must have caused a regression of displaying form errors from GraphQL (I'll open an issue in a moment). This PR restores proper error display by ensuring errors get delivered and processed on the client in the way that seems originally intended.
Fix #163