politics-rewired / Spoke

Politics Rewired's fork of Spoke
GNU General Public License v3.0
36 stars 17 forks source link

Standardize GraphQL error handling #1270

Open bchrobot opened 2 years ago

bchrobot commented 2 years ago

Is your feature request related to a problem? Please describe.

We don't have a documented approach to GraphQL error throwing and handling.

Describe the solution you'd like

We should have a standard for how we use and handle errors on both the server and in the client. Do we use the GraphQL spec for errors, including partial results (e.g. data and errors)? Do we provide our own first class error return union type? Do we use React error boundaries?

Describe alternatives you've considered

Continuing with a situation-by-situation and developer-by-developer approach.

Additional context

I have had a difficult time finding best practices for GraphQL error handling in React. It seems like it should be easier to work with GraphQL errors and React error boundaries. Or at least better ways of interacting with the many error types that could be returned.

It seems like the prevailing sentiment is to reject the GraphQL spec for returning errors and instead to use result unions to return data OR errors ([1], [2]).

Apollo, however, recommends "[using] nullable types for fields on which partial failure is acceptable" ([3], [4]), such as anticipated permission errors.

A breakdown of different approaches to draw from ([5]).

[1] https://itnext.io/the-definitive-guide-to-handling-graphql-errors-e0c58b52b5e1 [2] https://www.the-guild.dev/blog/graphql-error-handling-with-fp [3] https://www.apollographql.com/blog/graphql/error-handling/full-stack-error-handling-with-graphql-apollo/ [4] https://www.apollographql.com/blog/graphql/basics/using-nullability-in-graphql/ [5] https://productionreadygraphql.com/2020-08-01-guide-to-graphql-errors

ajohn25 commented 1 year ago

One note here is that React handling for these errors can sometimes be a red herring for debugging. A permissions error from GetOrganizationSettingsQuery led me to think that permissions had changed after a recent release but it was just an existing error we're hiding from the user 😅