-
Notifications
You must be signed in to change notification settings - Fork 217
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
Support more content types #1575
Conversation
letmaik/bytes-payload@12388 aka 20200908.8 vs master ewma over 50 builds from 11852 to 12368 |
@@ -452,7 +474,14 @@ def request( | |||
global_commit = unpack_seqno_or_view(out[18:26]) | |||
payload = out[26:] | |||
if status_code == 200: | |||
body = json.loads(payload) if payload else None | |||
if payload: | |||
# Ideally, the content type should be sent like for HTTP. |
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.
The view here is that websockets is the efficient option, and it is the efficient option because it does away with the large variable header. Endpoints are expected to expose their schema out of band rather than send (or expect) self-describing traffic.
An application that wishes to replicate the content-type concept across their websockets endpoints would do so in the outer frame of their schema.
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.
So, is it fine to keep the try-except? If not, then maybe for WebSocket it should always return bytes
?
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.
We probably should, but then we have to adjust the logging test accordingly. Let's keep it as is and do that clean up in a separate PR?
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.
While changing the npm app to use the new Body class I stumbled upon an issue when returning ArrayBuffer: protobufjs returns an Uint8Array with byteLength 14 and the underlying ArrayBuffer has a byteLength of 8192, which means we should probably also accept typed arrays as return values to avoid potential issues where a slice was taken and the lengths would mismatch. I'll see if quickjs has the necessary APIs to deal with typed arrays. |
Fixes #1554.
Inside a JS endpoint:
Body
object which is similar to https://developer.mozilla.org/en-US/docs/Web/API/Body, except that it has a synchronous API and only offers.text()
,.json()
, and.arrayBuffer()
.Using the Python client (and hence in unit tests):
str
,bytes
, and all other types are assumed to be JSON. The type determines the default content type (text/plain
,application/octet-stream
,application/json
). If the body is astr
starting with@
then it is only assumed to be JSON if the filename ends with.json
-- no JSON parsing is done here, this is just for determining the default content type.str
(fortext/plain
), a parsed JSON object (forapplication/json
), or elsebytes
.