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

Support more content types #1575

Merged
merged 15 commits into from
Sep 8, 2020
Merged

Conversation

letmaik
Copy link
Member

@letmaik letmaik commented Sep 4, 2020

Fixes #1554.

Inside a JS endpoint:

  • Request body is a 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().
  • Response body can be a string, a JSON-serializable object, a typed array object, or an ArrayBuffer object. The type decides the response content type. In the future, an API should be added so that the endpoint can override the default content type. Note that previously strings were serialized as JSON -- this doesn't happen anymore as it's generally not what you want.

Using the Python client (and hence in unit tests):

  • Request body can now be 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 a str 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.
  • Response body is str (for text/plain), a parsed JSON object (for application/json), or else bytes.

@letmaik letmaik requested a review from a team as a code owner September 4, 2020 16:07
@ghost
Copy link

ghost commented Sep 4, 2020

letmaik/bytes-payload@12388 aka 20200908.8 vs master ewma over 50 builds from 11852 to 12368
images

@@ -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.
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@letmaik
Copy link
Member Author

letmaik commented Sep 7, 2020

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.

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

Successfully merging this pull request may close these issues.

Unit test helpers should allow sending raw body
2 participants