4
\$\begingroup\$

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 < 300) {
    return response
  } else {
    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
      });
  }
}

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..

\$\endgroup\$
10
\$\begingroup\$

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:

  1. 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
  2. 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:

  1. Your semi colons aren't consistent. Either use them or don't. Don't mix and match, it just confuses everyone.
  2. 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.

\$\endgroup\$
  • \$\begingroup\$ Wow, this is way more then I hoped to get out of this code review. Thanks a lot for taking so much time for my question. To give you some background info. The inconsistencies slipped in because I first wanted to add this as a PR to their repo but hesitated because I was doubting the code too much and did not want to more of a question then a PR. I then created a question on StackOverflow, but realized it might be better to add it here. The snippet I used in my project was using semicolons and let/const. \$\endgroup\$ – Tieme Jul 5 '16 at 13:50
  • \$\begingroup\$ Thanks for your thoughts on mixed returns. I hadn't realized that yet, good point. About catching the error, if you don't catch it if parsing the json fails, then you won't be able to do something with the response.statusText right? So the displayed message to the user would be something like: 'Unexpected token x' instead of 'Internal server error'. You're right, unwinding the stack trace is indeed no fun, I wonder why the fetch repo is throwing the error directly, maybe they don't want to include any Promises in the docs.. \$\endgroup\$ – Tieme Jul 5 '16 at 13:50
  • \$\begingroup\$ About the json.message || response.statusText. That was done to either get the user friendly message from the json payload, or, if it was not provided, fall back to the statusText, that is the most user friendly we can do right? Well you could hardcode a 'Sorry data could not be loaded' message in your javascript of course.. maybe that would be better indeed. \$\endgroup\$ – Tieme Jul 5 '16 at 13:52
  • \$\begingroup\$ Don't had code if, I just wouldn't use that in an Error object, personally \$\endgroup\$ – Dan Pantry Jul 5 '16 at 14:49
  • \$\begingroup\$ Sorry, I'm not sure what you mean? You mean that you wouldn't send the error message along with the api response? \$\endgroup\$ – Tieme Jul 5 '16 at 14:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.