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

204 and json #113

Closed
wheresrhys opened this issue Aug 24, 2015 · 17 comments
Closed

204 and json #113

wheresrhys opened this issue Aug 24, 2015 · 17 comments

Comments

@wheresrhys
Copy link

When a Response is 204 and response.json() is called an unexpected end of input error is thrown.
As the response is already stating that there is no JSON trying to parse it feels wrong. Would it make more sense in this case to either:
a) resolve with an empty value
b) throw a more informative error e.g. Cannot parse a response with no content as json

@annevk
Copy link
Member

annevk commented Aug 24, 2015

I don't think we want to intertwine stream consumption as a particular type with semantics of the response. E.g., we don't check Content-Type either.

@wheresrhys
Copy link
Author

I agree it's not something you'd want to do in general for all content types, but given that the decision has been made to have a .json() method - thereby elevating json above all other content types already - is it not reasonable to want it to behave a little more intelligently than a simple proxy to JSON.parse()?

@annevk
Copy link
Member

annevk commented Aug 24, 2015

We don't do anything special for text() or formData() either.

@wanderview
Copy link
Member

In theory a 204 should have a null body, right? In that case, we can probably detect this condition with if(response.body) once streams are integrated.

@annevk
Copy link
Member

annevk commented Aug 30, 2015

Yes. Closing this since I don't think we want these methods to have this kind of dependency.

@bitinn
Copy link

bitinn commented Nov 7, 2016

Hey, I want to follow up on @wanderview's comment:

In that case, we can probably detect this condition with if(response.body) once streams are integrated.

Does any browser currently does this? As in, do any of them handle json() for 204 empty response so that users don't have to catch manually?

@daxiangaikafei
Copy link

i think that ,do not interwine the content-type is a good job

@haywirez
Copy link

haywirez commented Aug 2, 2018

Any new thoughts on best practices to handle this?

@jakearchibald
Copy link
Collaborator

async function getJSON(response) {
  if (response.status === 204) return '';
  return response.json();
}

@fregante
Copy link

fregante commented Sep 13, 2018

I'm dealing with GitHub's followers API and it returns empty responses as 404 or 204.

This is my solution:

const content = await response.text();
const json = content.length > 0 ? JSON.parse(content) : {}; // or : response.ok

Aside: I think that fetch's behavior is correct and APIs are breaking the contract by not sending valid JSON when the user expects it (JSON.parse('') also fails)

@jakearchibald
Copy link
Collaborator

Or, just check the status as in #113 (comment). Or: const json = await response.json().catch(() => ({}))

@fregante
Copy link

I was responding exactly to that, empty requests might have statuses other than 204.

Also .catch() will lump together real errors and empty responses, which might or might not be desirable.

@caub
Copy link

caub commented Aug 23, 2019

If we consider:

const data = await (/^application\/json/.test(r.headers.get('content-type'))
    ? r.json()
    : r.text());

(from https://gist.github.com/caub/7494b4391c2d62c49b565d2cfc2c0c1f/668fd976a7b72626af4345c93377ea6fca0d41a7#file-fetch-wrapper-js-L17)

since we can't consume the body twice, we can't do a try { data = await r.json() } catch { data = await r.text() }

And since content-type is not fully reliable, is it better to do do?

let data = await r.text();
try {
  data = JSON.parse(data);
} catch {}

in terms of performance? of readability, etc.

I know an answer would be to ensure the server always return json responses (or empty with/without the right 204 statusCode), but it's not always the case

I see why the idea of Response.prototype.json emerged, as a shortcut for JSON.parse(await r.text()), but it's not really ideal in practice as you can see

@annevk
Copy link
Member

annevk commented Aug 23, 2019

Well, that really depends on the application and to what extent the signals from the server are significant. E.g., processing a response without Content-Type does not seem desirable to me. Browsers got in a lot of trouble doing that. Not checking the response code also seems less than optimal.

We could perhaps introduce something akin to jsonOrUndefined that would effectively never throw, but it would be good to see that established as a pattern in libraries with high adoption rates and even then I'd worry a bit about the brittle infrastructure it'd encourage.

@hoIIer
Copy link

hoIIer commented Apr 8, 2020

New here and just ran into this, what's the community answer to deal with a json POST request that returns 204 No Content? Just check response code?

@caub
Copy link

caub commented Apr 9, 2020

const r = await fetch('https:...', { method: 'POST', .. });
if (r.ok) {
  const data = await r.json().catch(() => null);
  // ...
}

@AMS777
Copy link

AMS777 commented Nov 3, 2020

async function getJSON(response) {
  if (response.status === 204) return '';
  return response.json();
}

I would go with this approach. But if it's something that needs to be done always, it could just be integrated in the call to json().

boton added a commit to 4lejandrito/liferay-portal that referenced this issue Feb 24, 2021
boton added a commit to 4lejandrito/liferay-portal that referenced this issue Feb 25, 2021
brianchandotcom pushed a commit to brianchandotcom/liferay-portal that referenced this issue Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests