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

Error response is not captured properly #454

Open
kenng opened this issue Oct 10, 2024 · 3 comments
Open

Error response is not captured properly #454

kenng opened this issue Oct 10, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@kenng
Copy link

kenng commented Oct 10, 2024

Environment

any version, as it is related to behaviour of try...catch

Reproduction

Sample snippet

async function test1() {
  const u = { res1: undefined, res2: undefined }
  try {
    u.res1 = await fetch(url)
  } catch (e) {
    console.error(u.res1)     // will be undefined
  }
}

Describe the bug

The response is not set when there is an error.

context.response = await fetch(

While the retry function relies on the response, this causes the retry function to execute unintentionally.

const responseCode = (context.response && context.response.status) || 500;

Additional context

No response

Logs

No response

@kenng kenng added the bug Something isn't working label Oct 10, 2024
@feelixe
Copy link

feelixe commented Dec 16, 2024

I don't believe this is a bug.
In you're code example when u.res1 = await fetch(url) throws an exception it will not assign a value to u.res1, this is expected and just how Javascript is meant to work.

However, if you want to read the response of a fetch error you can do the following:

import { FetchError, ofetch } from "ofetch";

try {
    const res = await ofetch("https://example.com/123")
} catch (error) {
    if(error instanceof FetchError) {
     console.log(error.response)    
    }
}

@kenng
Copy link
Author

kenng commented Dec 23, 2024

@feelixe I think my message misled you. You are absolutely right about the behaviour of the JavaScript. The issue is that this repo's code is written in such a way that it expects the response to have a value.

See the code below, annotated with arrows

    try {
      context.response = await fetch(      <-- context.response will be null on error
        context.request,
        context.options as RequestInit
      );
    } catch (error) {
      context.error = error as Error;
      if (context.options.onRequestError) {
        await callHooks(
          context as FetchContext & { error: Error },
          context.options.onRequestError
        );
      }
      return await onError(context);
    } finally {
      if (abortTimeout) {
        clearTimeout(abortTimeout);
      }
    }
  async function onError(context: FetchContext): Promise<FetchResponse<any>> {
    // Is Abort
    // If it is an active abort, it will not retry automatically.
    // https://developer.mozilla.org/en-US/docs/Web/API/DOMException#error_names
    const isAbort =
      (context.error &&
        context.error.name === "AbortError" &&
        !context.options.timeout) ||
      false;
    // Retry
    if (context.options.retry !== false && !isAbort) {
      let retries;
      if (typeof context.options.retry === "number") {
        retries = context.options.retry;
      } else {
        retries = isPayloadMethod(context.options.method) ? 0 : 1;
      }

      const responseCode = (context.response && context.response.status) || 500;  <--- responseCode=500, which causes the retry to keep executing when retries > 0 (default of retries value is more than zero if I not remember it wrong)
      if (
        retries > 0 &&
        (Array.isArray(context.options.retryStatusCodes)
          ? context.options.retryStatusCodes.includes(responseCode)
          : retryStatusCodes.has(responseCode))
      ) {
        const retryDelay =
          typeof context.options.retryDelay === "function"
            ? context.options.retryDelay(context)
            : context.options.retryDelay || 0;
        if (retryDelay > 0) {
          await new Promise((resolve) => setTimeout(resolve, retryDelay));
        }
        // Timeout
        return $fetchRaw(context.request, {
          ...context.options,
          retry: retries - 1,
        });
      }
    }
    ```

@pi0
Copy link
Member

pi0 commented Dec 23, 2024

Thanks @kenng for the further explanation. Can you please make an updated reproduction then? (ie, it might be expected that you might want context.response in error hook or error.response in catch).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants