Skip to content
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

Add documentation on how to centralize error handling #88

Open
nirvdrum opened this issue Jan 28, 2020 · 1 comment
Open

Add documentation on how to centralize error handling #88

nirvdrum opened this issue Jan 28, 2020 · 1 comment

Comments

@nirvdrum
Copy link
Contributor

I'm trying to centralize my exception reporting with react-relay-network-modern and haven't found a very good way to do so yet within the confines of either the existing middleware or providing my own custom middleware. In my case, I'm looking to report GraphQL errors to Sentry, but any exception reporting or log aggregation service would run into the same issue. I'm deliberately not looking to add logging code to every QueryRenderer because it's too easy to miss a case and any change in logging requires updating it in N places.

There are two basic error classes I'd like to capture: fetch errors and GraphQL responses that come back with an errors field. As far as I can tell, the only way to centrally handle the fetch errors is with custom middleware:

next => async req => {
  Sentry.setTag('graphql_query', req.getID())

  try {
    return await next(req)
  } catch (e) {
    Sentry.captureException(e)
    throw e
  }
}

That part works fine, although I also get errors about requests being canceled if a user goes to a different page while the query is still running. So a real solution will require a bit more filtering.

Logging the errors field, however, is proving to be a lot more complicated. It exists as a field on the response object that's later turned into a RRNLRequestError. The conversion to an error appears to happen outside the middleware stack, so the catch statement from my custom middleware doesn't catch it.

The errors field will populate on the response object, so I could do something like:

next => async req => {
  Sentry.setTag('graphql_query', req.getID())

  try {
    const res = await next(req)

    if (res.errors) {
      Sentry.captureException(res.errors)
    }

    return res
  } catch (e) {
    Sentry.captureException(e)
    throw e
  }
}

But, res.errors isn't really an exception, so that makes the logging in Sentry a bit weird. Since errors is typed as any and has no formatting functions that I could find, trying to treat it as a string and use Sentry.captureMessage(res.errors.toString()) doesn't produce useful output.

Another option is using the errorMiddleware, like so:

errorMiddleware({
  logger: (message: string) => { Sentry.captureMessage(message) }
})

I didn't really want to split the error handling up, but I could live with that. The real problem with this option is the message argument really expects to be used with console. The docs mention the logger value is "log function (default: console.error.bind(console))", but if you use anything other than console you end up with a bunch of noisy formatting codes (%c) with no way to disable them. However, at least you have an otherwise nicely formatted message.

It'd be nice if I could invoke that formatter code from my custom middleware and handle logging of res.errors, but that formatting code is private to errorMiddleware.

The final option is to just handle RRNLRequestError in every QueryRenderer. I don't really like this option because it's easy to miss a logging statement and it litters the exception handling code around. But, with this approach you get a nicely formatted error message and a stack trace. Perhaps exposing formatGraphQLErrors and createRequestError is the best option?

I'm happy to provide a PR with documentation enhancement. I just don't know what the best way to centralize exception handling currently is. Any guidance would be much appreciated.

@olso
Copy link

olso commented Apr 23, 2020

For anyone bumping into this

horrible snippet ahead

export const apolloServerErrorsMiddleware = (): Middleware => {
  return (next) => async (req) => {
    const res = await next(req);

    // @ts-ignore
    if (res && res.errors && Array.isArray(res.errors) && res.errors.length) {
      // @ts-ignore
      res.errors.forEach(({ extensions }) => {
        if (extensions && extensions.code) {
          if (extensions.code === "UNAUTHENTICATED") {
            // @ts-ignore
            res.status = 401;
          }

          if (extensions.code === "FORBIDDEN") {
            // @ts-ignore
            res.status = 403;
          }
        }
      });

      // @ts-ignore
      if (res.status === 401 || res.status === 403) {
        console.error(res);
        Sentry.captureException(new Error());

        // This will force new token down the line, the error might be fucked tho
        return Promise.reject({ res }); // eslint-disable-line prefer-promise-reject-errors
      }

      console.error(res.errors[0]);
      Sentry.captureException(res.errors[0]);

      return Promise.reject(res.errors[0]); // eslint-disable-line prefer-promise-reject-errors
    }

    return res;
  };
};

on mutation error, error.message will actually contain the apollo server code, without the formatting fluff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants