[breaking] Sane handling of 204 response (successful)#1
[breaking] Sane handling of 204 response (successful)#1aars wants to merge 1 commit intopixelhandler:masterfrom
Conversation
|
@andrewmp1 any thoughts on this change? |
| this.cacheResponse({ | ||
| payload: payload, headers: headers, options: options | ||
| }); | ||
| if (payload) { |
There was a problem hiding this comment.
@andrewmp1 originally I just used an empty string for 204. But not content means no content. So yeah we can skip the deserialize and cache hooks on 204
| @param {Function} resolve - Promise resolve handler | ||
| */ | ||
| fetchNoContentHandler(response, resolve) { | ||
| return response.text().then(function(resp) { |
There was a problem hiding this comment.
@andrewmp1 in our apps there was some breakage when the content was empty I had added a string to bypass errors but that was a hack.
| // Did we receive a body? | ||
| if (text) { | ||
| // Parse it as JSON, make it our payload. | ||
| let json = JSON.parse(text); |
There was a problem hiding this comment.
This may need a try/catch, around JSON.parse()
There was a problem hiding this comment.
It looks like this lib is built to only handle JSON, though it's not made clear.
If we do only handle JSON, then nah, no need to catch it, since all we can do it re-throw something that tells the user the same thing; Failed to parse what we expected to be a JSON document. (Or, we could, but only to improve on the error message, while taking a performance hit on every request).
If this library should be able to handle other document types, or simply pass through the payload, then other work needs to be done.
2 participants
This is a breaking change, since it modifies the return value of the success handler.
As far as I can tell only ember-jsonapi-resources uses this library, which is also why I believe these changes are necessary:
204 is a successful response. There is no reason to handle it differently, except for the integrated deserialisation and caching in this library.
Because the
NoContentHandlerresolves with nothing but an empty string, there is no way for a user of this library to know if this was a 204 response (a check for an empty string payload is no good).EJR in turn is unable to handle a perfectly valid 204 response, except for maybe like so:
Which is very fragile. Not having a payload does not constitute a 204 (even though a successful response without any content is most likely a 204)
I think it would be good to always resolve with most, if not all useful data about the request, and not hide anything from the user (or, EJR) in such a low-level library as this.