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

Missing statusCode in ApolloError causes inconsistent error handling in Nuxt #11634

Open
ptylek opened this issue Mar 1, 2024 · 3 comments
Open
Labels
🔍 investigate Investigate further

Comments

@ptylek
Copy link

ptylek commented Mar 1, 2024

Hi! 👋
I am working on Nuxt project with Apollo Client. Nuxt is using H3 and network ApolloErrors might cause inconsistent error handling.

This happens because ApolloError object doesn't contain statusCode needed by H3 to set the correct HTTP response status. This inconsistency leads to situations where the error message indicates a different status code than what is actually set by H3, for example: error message is Response not successful: Received status code 429 with statusCode 500.

To address this issue the easiest would be to include statusCode in ApolloError for network errors. I understand that this is already added on networkError but it causes issues when ApolloClient is used with H3.

Snippets below are from h3/src/error.ts:

H3Error is created using ApolloError object and then used to set response status and end response.

export function sendError(
  event: H3Event,
  error: Error | H3Error,
  debug?: boolean,
) {
  ...
  const h3Error = isError(error) ? error : createError(error);

  const responseBody = {
    statusCode: h3Error.statusCode,
    statusMessage: h3Error.statusMessage,
    stack: [] as string[],
    data: h3Error.data,
  };

  ...
  const _code = Number.parseInt(h3Error.statusCode as unknown as string);
  setResponseStatus(event, _code, h3Error.statusMessage);
  event.node.res.setHeader("content-type", MIMES.json);
  event.node.res.end(JSON.stringify(responseBody, undefined, 2));
}

createError sets statusCode only if it is set on error itself. For ApolloError it would be on input.networkError.statusCode instead. input.statusCode is missing.

export function createError<DataT = unknown>(
  input:
    | string
    | (Partial<H3Error<DataT>> & { status?: number; statusText?: string }),
) {
  ...
  const err = new H3Error<DataT>(input.message ?? input.statusMessage ?? "", {
    cause: input.cause || input,
  });

  ...
  
  if (input.statusCode) {
    err.statusCode = sanitizeStatusCode(input.statusCode, err.statusCode);
  }

If no statusCode is set for ApolloError, H3Error default statusCode is 500.

export class H3Error<DataT = unknown> extends Error {
  static __h3_error__ = true;
  statusCode = 500;
  fatal = false;
  unhandled = false;
  statusMessage?: string;
  data?: DataT;
  cause?: unknown;

I use @apollo/client@3.8.10 and nuxt@3.9.3.

@jerelmiller
Copy link
Member

Hey @ptylek 👋

I'd like to take this to the team to discuss a bit further. We have a team meeting next week and can figure out what to do there. Thanks!

@jerelmiller
Copy link
Member

Thanks for your patience!

After talking with the team, we'd like to avoid adding code in a core part of the library that is specific to one framework or library. Not all ApolloError instances are caused by network errors and we'd like to avoid cluttering the the ApolloError class with http-specific properties.

Preferably this should be handled in Nuxt/H3 itself in some way that would allow you to transform errors such as ours into something usable with that framework. An improvement I think we could make regardless is setting a cause from ApolloError back to the original error that caused the issue. While this information is currently available via ApolloError.networkError, cause is a more standard way for accessing the original error and could perhaps be utilized by h3 to parse the status code.

I've opened unjs/h3#691 to see if we can collaborate on a solution that works well for all parties involved.

@jerelmiller
Copy link
Member

Hey @ptylek 👋

I've opened unjs/h3#795 and along with #11902, we are hoping these two will integrate better together. Hopefully we can get both of these changes merged and released soon 🙂

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

No branches or pull requests

2 participants