1

I am writing a simple wrapper around fetch.

async function apiCall(
  endpoint: string,
  {
    data,
    headers: customHeaders,
    ...customConfig
  }: { data?: Object; headers?: Object } = {}
) {
  const config = {
    method: data ? 'POST' : 'GET',
    body: data ? JSON.stringify(data) : undefined,
    headers: {
      'content-type': data ? 'application/json' : undefined,
      ...customHeaders,
    },
    ...customConfig,
  }

  return fetch(endpoint, config as any).then(async (response) => {
    if (response.ok) {
      const json = await response.json() // 
      return json
    } else {
    //   what if `response` contains error messages in json format?
      return Promise.reject(new Error('Unknown Error'))
    }
  })
}

it works fine. The problem is with this snippet

 return fetch(endpoint, config as any).then(async (response) => {
    if (response.ok) {
      const json = await response.json()
      return json
    } else {
    //   what if `response` contains error messages in json format?
      return Promise.reject(new Error('Unknown Error'))
    }
  })

if response is not ok, it rejects with a generic Error. This is because by default, window.fetch will only reject a promise if the actual network request failed. But the issue is that, even if response is not ok, it might still be able to have error messages in json format. This depends on the backend implementation details but sometimes you are able to get the error messages in the response body by response.json(). Now this use case is not covered in the wrapper I built.

So I wonder how I am going to be able to account for that? I guess you can do something like

fetch(endpoint, config as any).then(async (response) => {
      if (response.ok) {
        const json = await response.json()
        return json
      } else {
          try {
            const json = await response.json()
            return Promise.reject(json)
          } catch {
            return Promise.reject(new Error('Unknown Error'))
          }
      }
    })

but I wonder if there is some more elegant way to do that?

Lastly, I am very aware of libraries like Axios. I built this partly to satisfy my intellectual curiosity.

Btw, a lightly unrelated question but I wonder if these two are equivalent

 if (response.ok) {
        const json = await response.json()
        return json
      }
 if (response.ok) {
        return response.json()
      }

Someone flagged my question as a duplicate of this question. In fact they are not the same. I did not make the same assumption as that question did about the API call returning JSON data both on success and on failure. My question is about exactly how we should do in cases where we cannot make such an assumption.

Joji
  • 4,703
  • 7
  • 41
  • 86
  • Does this answer your question? [fetch: Reject promise with JSON error object](https://stackoverflow.com/questions/29473426/fetch-reject-promise-with-json-error-object) – coagmano Mar 28 '21 at 22:47
  • @FredStark no it doesn't. The question the link points to makes the assumptions that I that requests always returns JSON data both on success and on failure. My question didn't have that assumption and in fact it is asking how we should do in situations where we cannot make that assumptions. Please revert your vote to close my question. – Joji Mar 29 '21 at 00:26
  • Oh I may have missed the case, but you're not in control at all of the backend? JSON is not the only way to send error messages, are you willing to also handle raw text messages or HTML error pages? – Kaiido Mar 29 '21 at 01:03
  • @Joji ahh okay, I see how it's different. Vote retracted. Looks like you're getting half-decent answers now too :) – coagmano Mar 29 '21 at 02:27

2 Answers2

5

That the response is not ok doesn't prevent you from consuming its body as JSON so your last snippet is indeed how it should be handled.

Now you ask for something "more elegant", well it may not be more elegant but a less redundant way to write the same would be:

fetch(endpoint, config as any).then(async (response) => {
  if (response.ok) {
    return response.json(); // there is no need to await the return value
  }
  else {
    const err_message = await response.json() // either we have a message
      .catch( () => new Error( "Unknown Error" ) ); // or we make one
    return Promise.reject( err_message );
  }
})

And regarding the last question, yes both are equivalent, the await version doing one more round-trip through the microtask-queue and making your code a bit longer, but for all that matters we could say they are the same.

Kaiido
  • 123,334
  • 13
  • 219
  • 285
  • Hi thanks for the answer. Can I ask why is that even if response's body doesn't have a JSON, calling `response.json()` is not going to throw? Or as long as the request 'content-type' is 'application/json', the response body is always going to be a valid json even if it is not ok?? – Joji Mar 29 '21 at 01:37
  • It is going to throw, well, the returned Promise will reject, that's why I chained a `.catch()` to set your own error value. The other answer is wrong. – Kaiido Mar 29 '21 at 01:55
  • Alternatively you could also use `return response.json().catch(() => new Error("Unknown Error")).then(error => { throw error })` in the else-statement, then remove the `async`/`await`. – 3limin4t0r Apr 09 '21 at 11:14
2

I'd simplify Kaiido even further, fix some bugs, and add a custom error handler.

class ApiCallError extends Error {

  response: Response;
  body: any;
  httpStatus: number;

  constructor(response, body) {

    super('HTTP error: ' + response.status);
    this.httpStatus = response.status;
    this.response = response;
    this.body = body;

  }  

}
async function apiCall(endpoint: string, options?: any) {

  const config = {
    // do your magic here
  }

  const response = await fetch(endpoint, config);

  if (!response.ok) {
    throw new ApiCallError(response, await response.json());
  }
  return response.json();
}

Changes:

  1. You don't need to catch errors if you're just going to throw again.
  2. You don't need .then() if you support await, and this will make your code a lot simpler.
  3. There's really no point in Promise.resolve and Promise.reject, you can just return or throw.
  4. You should not return plain errors, you should throw them (or make sure they are wrapped in a rejecting promise)
  5. Even though in javascript you can 'throw anything' including strings and arbitrary objects, it's better to throw something that is or extends Error, because this will give you a stack trace.
  6. Making all the error information available on the custom error class provides everything a user of apiCall could possibly need.
Evert
  • 93,428
  • 18
  • 118
  • 189
  • If `response.json()` is not a valid JSON then this will reject with a SyntaxError JSON.parse error instead of the expected ApiCallError one. – Kaiido Mar 29 '21 at 03:45
  • @Kaiido yes, you are right. So it really depends on what you expect. Perhaps OP always intends to emit JSON errors and *wants* that error if the server didn't emit JSON. – Evert Mar 29 '21 at 04:09
  • @Kaiido in your example, that case turns into an `Unknown Error`, I would argue that I rather get an JSON-parsing related error than an unknown one. – Evert Mar 29 '21 at 04:10
  • I only rewrote OP's logic, I didn't make it. – Kaiido Mar 29 '21 at 04:42
  • @Kaiido sorry, forgot that was in the initial question. Regardless, I stand by it. – Evert Mar 29 '21 at 04:48