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

Don't parse JSON if response is a server error #61

Open
jdforsythe opened this issue Jan 21, 2021 · 3 comments · May be fixed by #68
Open

Don't parse JSON if response is a server error #61

jdforsythe opened this issue Jan 21, 2021 · 3 comments · May be fixed by #68
Labels

Comments

@jdforsythe
Copy link
Contributor

Related to #48

When a server responds with a status code of 5xx, the body should not be parsed as JSON.

import * as p from 'phin';

const makeRequest() {
  const url = 'https://google.com';

  const opts: p.IJSONResponseOptions = {
    url,
    method: 'GET',
    parse: 'json',
    headers: {
      accept: 'application/json',
    },
  };

  const result = await p(opts);

  return result.body;

If the server returns a 500 Internal Server Error, the JSON parse still happens and the error is Unexpected token I in JSON at position 0. It appears the only way around this is to not have phin parse the JSON and to do it manually after checking the statusCode.

I think it makes more sense for phin to not parse the JSON unless the statusCode is a 2xx.

jdforsythe added a commit to jdforsythe/phin that referenced this issue Mar 2, 2021
Fixes ethanent#61

Prevents parsing body as JSON unless the status code is in the 2xx range to allow 4xx and 5xx errors to propagate without JSON parsing errors
@jdforsythe jdforsythe linked a pull request Mar 2, 2021 that will close this issue
@XuluWarrior
Copy link

I recall another request library (axios?) throwing a (deliberate) exception on non-success statuses.
I'm not saying that's the best way to deal with them but it is an option.

@XuluWarrior
Copy link

I created #75 as an alternative fix

@ethanent
Copy link
Owner

Comments on #75 as an alternative would be appreciated. You can see my comment on that PR. I think it's a practical solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants