-
Notifications
You must be signed in to change notification settings - Fork 2
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
v0.1.0 #1
v0.1.0 #1
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1 +/- ##
=======================================
Coverage ? 98.75%
=======================================
Files ? 3
Lines ? 80
Branches ? 14
=======================================
Hits ? 79
Misses ? 1
Partials ? 0 Continue to review full report at Codecov.
|
* object with the `siren()` method for parsing JSON | ||
* [Siren](https://github.com/kevinswiber/siren) (`application/vnd.siren+json`). | ||
*/ | ||
export default class ClientResponse implements Response { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isSiren()
helper method would be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems odd to have that without isJson()
, isText()
, etc. helpers, which aren't part of the Response
interface. What would isSiren()
do exactly? Keep in mind, that the body is a stream, so once it's read, it cannot be read again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would check that the Content-Type
header is Siren (maybe isContentTypeSiren()
). That will probably be a common check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe checking the Content-Type
header is sufficient, per Section 3.1.1.5 of RFC 7231:
A sender that generates a message containing a payload body SHOULD
generate a Content-Type header field in that message unless the
intended media type of the enclosed representation is unknown to the
sender. If a Content-Type header field is not present, the recipient
MAY either assume a media type of "application/octet-stream"
([RFC2046], Section 4.5.1) or examine the data to determine its type.
The last piece ("examine the data to determine its type") is where it gets tricky with the body stream. Since there are no other isMediaType()
helpers, I'm not sure there's much value here. If it's not Siren, users will have to check the Content-Type
using the typical headers.get('Content-Type')
anyway. OTOH, it might be useful to have a constant for the Siren media type, so the check would be less error-prone. Something like this, maybe?
if (response.headers.get('Content-Type') === MediaType.Siren) {
const entity = await response.siren();
}
See CHANGELOG.md