Fetch: how to deal with a json payload in an error response?

Trying to get https://github.com/github/fetch working with an API that almost always returns a json payload, even when the response is an error:

{
  "message": "User not logged in",
  "code": 401
}

So I came up with this, adopting their status check example

function checkStatus(response) {
  if (response.status >= 200 && response.status  {
        var error = new Error(response.statusText)
        error.response = response
        throw error
      }).then((json) => {
        var error = new Error(json.message || response.statusText)
        error.response = response
        throw error
      });
  }
}

This would handle both json and an invalid json reponse in case the api isn't able to produce valid json.

Would this be okay or am I missing something? I don't really like the .catch before the .then, but otherwise the .catch will catch the thrown error.. which is not what we want..

Note this reads more like a hypothetical question but I'm giving you the benefit of the doubt.

Your example is alright, but it has a few issues. I'm going to mostly touch on semantics here, so bear with me. Note I'll be going with the WHATWG spec that's implemented in browsers now, which should be the same as the repo you've linked, but just so you are aware :)

if (response.status >= 200 && response.status < 300) {
    return response

There's no need to check response.status like this; it also doesn't cover all "success" responses (assuming that is indeed what you're trying to do. WHATWG fetch exposes an ok property on the response object which will do this for you..

if (response.ok) {
    return response

Let's look at your function as a whole.

function checkStatus(response) {
  if (response.ok) {
    return response
  } else {
    ...
  }
}

As this is just a straight up if-else, you can reduce cyclomatic complexity by dropping the else here.

return response.json()
    .catch(() => {
      var error = new Error(response.statusText)
      error.response = response
      throw error
    }).then((json) => {
      var error = new Error(json.message || response.statusText)
      error.response = response
      throw error
    });

I think this is a bit redundant. I'm guessing what you're trying to do is either:

• Catch, and re-throw (thus rejecting the promise), the error with the response property if you hit this branch if there was already an exception

• Reject the promise by throwing an error if there was not an exception by taking the response json and assigning it as an error message.

I'd argue that you should not be catching the error in the first place. The only place an error will occur here is if there is a JSON syntax error. There's nothing you can do to handle this, so don't attach bits and pieces to it; the only thing it would be useful for is debugging, and you shouldn't be adding debugging stuff like this to the user's browser - this should be caught in e2e tests. Instead, just let the default error propagate up. This leaves you with this so far:

function checkStatus(response) {
  if (response.ok) {
    return response
  }
  return response.json().then((json) => {
    var error = new Error(json.message || response.statusText)
    error.response = response
    throw error
  });
}

The next thing I have to add is this function could either be synchronous or asynchronous depending on what is passed to it. This is a bad thing and is explained here by more clever people than I. The upshot is that you should design your API so that if it can return asynchronously, it always should. Solution to this is just to wrap response in a Promise.resolve().

function checkStatus(response) {
  if (response.ok) {
    return Promise.resolve(response)
  }

  return response.json().then((json) => {
    var error = new Error(json.message || response.statusText)
    error.response = response
    throw error
  });
}

Next:

• Your semi colons aren't consistent. Either use them or don't. Don't mix and match, it just confuses everyone.

• If you're using ES6 arrows, you can use const/let as well. Use them.

You could simplify your promise handler code by using something like Object.assign if you support it or _.assign otherwise, which makes things nice and sugary.

function checkStatus(response) {
  if (response.ok) {
    return Promise.resolve(response)
  }

  return response.json().then(json => {
    const error = new Error(json.message || response.statusText)
    throw Object.assign(error, { response })
  })
}

Consider using Promise.reject instead of throw. It's a standard API for promises and clearly communicates your intent. Unwinding the stack trace isn't fun.

function checkStatus(response) {
  if (response.ok) {
    return Promise.resolve(response)
  }

  return response.json().then(json => {
    const error = new Error(json.message || response.statusText)
    return Promise.reject(Object.assign(error, { response }))
  })
}

I think that's about it for now. I think it's weird you're doing either json.message || response.statusText as an error message, but whatever floats your boat. Error reporting messages are largely what is helpful to the developer. I'd recommend you put user-friendly error message handling somewhere, though.

StackExchange Code Review Q#133911, answer score: 11