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

Stop throwing errors and wrap JSON.parse with try/catch block #235

Closed
niftylettuce opened this issue Nov 26, 2015 · 13 comments
Closed

Stop throwing errors and wrap JSON.parse with try/catch block #235

niftylettuce opened this issue Nov 26, 2015 · 13 comments

Comments

@niftylettuce
Copy link

https://github.com/github/fetch/blob/master/fetch.js#L193

See facebook/react-native#4376 for more insight.

@matthew-andrews
Copy link
Contributor

I believe it's correct as per the spec for fetch to throw an error if you try to get json output from a non json response… ?

@niftylettuce
Copy link
Author

so if that were the case, how do you gracefully show the user an error
message when the error doesn't get bubbled up to the callback/promise? now
im having to write try/catch blocks everywhere.
On Nov 26, 2015 4:51 PM, "Matt Andrews" [email protected] wrote:

I believe it's correct as per the spec for fetch to throw an error if you
try to get json output from a non json response… ?


Reply to this email directly or view it on GitHub
#235 (comment).

@niftylettuce
Copy link
Author

e.g. an endpoint suddenly returns 404 Not Found instead of normal 200 JSON
On Nov 26, 2015 4:52 PM, "Nick Baugh" [email protected] wrote:

so if that were the case, how do you gracefully show the user an error
message when the error doesn't get bubbled up to the callback/promise? now
im having to write try/catch blocks everywhere.
On Nov 26, 2015 4:51 PM, "Matt Andrews" [email protected] wrote:

I believe it's correct as per the spec for fetch to throw an error if you
try to get json output from a non json response… ?


Reply to this email directly or view it on GitHub
#235 (comment).

@niftylettuce
Copy link
Author

Because of this mess, here's the fix I wrote for this issue we have now... I basically have to do my own JSON.parse:

        fetch(apiUri + path, {
          method: 'GET',
          headers: that.headers
        })
        .then((res) => {
          try {
            let response = JSON.parse(res);
            if (response && response.error)
              throw new Error(response.error);
            return res.json();
          } catch (e) {
            return res.text();
          }
        })
        .then((res) => {
          fn(null, res);
        })
        .catch(function(err) {
          fn(err);
        })

@matthew-andrews
Copy link
Contributor

We normally check the status (res.ok is handy for that) of the response before requesting json…

.then(res => {
  if (res.ok) {
    return res.json();
  } else {
    // handle error 
  }
});

Also you could always implement the try catch yourself and do:-

.then(res => res.text())
.then(res => {
  try {
    return JSON.parse(res);
  } catch(err) {
    // handle error
  }
});

@matthew-andrews
Copy link
Contributor

I would highly recommend checking the status of the response either via .ok or .status.

@niftylettuce
Copy link
Author

good point

@matthew-andrews
Copy link
Contributor

I will close this issue then :)

@sethfatz
Copy link

@matthew-andrews Attempting to return response.json() after reading response.ok (or response.status) throws 'Already read' log and 'undefined is not an object' error

@mislav
Copy link
Contributor

mislav commented Nov 17, 2016

@sethfatz Make sure to return response from a handler that checks response.ok.

@acomito
Copy link

acomito commented Apr 12, 2018

Does anyone have any "real world" examples of how they're handling errors here? Particularly in react?

.then(res => res.text())
.then(res => {
  try {
    return JSON.parse(res);
  } catch(err) {
    // handle error
    // return error and use this.state to display it?
    // show an alert?
   // alert sentry
  }
});

I'm sure it's context specific, but I'm having trouble finding good examples beyond error handling 101.

@jymbob
Copy link

jymbob commented Sep 11, 2018

Pretty sure I can share this without giving away too many trade secrets 😉

Premise: a successfuly query to our API endpoint returns a valid JSON packet
Sometimes it returns an error packet with a suitable code e.g. 409 - conflict
Sometimes it returns a 404 (e.g. record not found)
Sometimes it returns an empty file, or text, or html (it's complicated)

I definitely lifted some of it from a helpful tutorial - will cite if I can find it, but worked on the error handler a lot since then

const fetchJSON = ((url, apiKey) => {
  return fetch(url, {
    mode: 'cors',
    credentials: 'include',
    cache: 'no-store',
    headers: {
      'Bearer': apiKey,
    }
  }).then(validateResponse)
    .then(readResponseAsJSON)
    .catch(handleResponseError)
})

const validateResponse = (response => {
  if (!response.ok) {
    return Promise.reject(response)
  }
  return response
})

const readResponseAsJSON = (response => {
  return response.json()
})

const handleResponseError = (response => {
  // explicitly returning a promise, because I can't figure out the right syntax otherwise
  return new Promise((resolve, reject) => {
    const statusText = response.status + ': ' + response.statusText
    response.text()  //response.json() doesn't appear to support sensible error handling of non-JSON
      .then(text => {
        let jsonData = {}
        try {
          jsonData = JSON.parse(text)  // try to do our own parsing of the json here
        } catch(err) {
          //no-op
        }
        return jsonData
      })
      .then(packet => {
        if (packet.error) {
          return reject(packet.error)
        } else {
          if (response.status == 404) {
            return reject('404: not found')  //we treat this as a special code in a couple of places
          } else {
            return reject(statusText)
          }
        }
      })
  })
})

We use that everywhere, in the format

fetchJSON('http://example.com/json', 'mySuperSecretApiKey123')
  .then(packet => {
    //success
    console.log(packet)
  }).catch(err => {
    //failure
    console.error(err)
  })

@jaychsu
Copy link

jaychsu commented Feb 20, 2019

I think there is a good real-world solution@MDN to distinct JSON-formatted response and plain text.

fetch(myRequest).then(function(response) {
    var contentType = response.headers.get("content-type");
    if(contentType && contentType.includes("application/json")) {
      return response.json();
    }
    throw new TypeError("Oops, we haven't got JSON!");
  })
  .then(function(json) { /* process your JSON further */ })
  .catch(function(error) { console.log(error); });

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants