Skip to content

Conversation

@dmitrizagidulin
Copy link

Updates the request() method to also support binary Blob objects in request body (in addition to just JSON object).

* @param {object} options.json - The JSON object, if any, to send with the
* @param {object} [options.json] - The JSON object, if any, to send with the
* request.
* @param {Blob|Buffer|Uint8Array} [options.blob] - The binary blob to send
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this should perhaps be named body. Thoughts? I would also say that {Blob|Uint8Array)} would suffice, since a Buffer is a Uint8Array.

Copy link
Author

Choose a reason for hiding this comment

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

Understood, yeah. So, part of the challenge here is that -- the fetch parameter for BOTH JSON and any other payload is named body. But ky made a syntactic shortcut field json that was short for "body: & content-type: json", right.

So, my thought with naming this param blob is to to use the same pattern -- have that stand for a shortcut of "body: & content-type: octet-stream".

I'm happy to drop that and just pass body through, though.

Copy link
Member

Choose a reason for hiding this comment

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

I think passing in body is what we should do -- and if you pass both json and body it's an error (which hopefully ky handles on its own already, I don't recall).

Notably, a Blob can have a totally different content-type than just octet-stream (as a Blob can carry its own media type as an option). If we accept either json (for backwards compatibility/compatibility with ky) or body, where it can be a Blob or Uint8Array -- and we don't allow both, I think we'll have our bases covered without introducing yet another option that isn't in sync with either fetch or ky.

Then, when body is a Uint8Array, the content-type will have to be manually set (or default to octet-stream) and if it's a Blob instance, the content-type can be pulled from the Blob or default to octet-stream.

Unless I'm missing some other problem, it seems body would work fine and would be more future proofed in the event there are other body "types", with only json having been special cased.

Copy link
Member

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

Starting making suggestions, but there are lot -- probably easier to do find and replace for blob => body.

Also, I didn't check the headers here, presumably, ky / httpClient handles them by using application/octet-stream if nothing else is provided.

* @param {object} options.json - The JSON object, if any, to send with the
* @param {object} [options.json] - The JSON object, if any, to send with the
* request.
* @param {Blob|Buffer|Uint8Array} [options.blob] - The binary blob to send
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {Blob|Buffer|Uint8Array} [options.blob] - The binary blob to send
* @param {Blob|Uint8Array} [options.body] - The binary blob to send

headers = {},
json
json,
blob
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
blob
body

Comment on lines +385 to +387
// handle blob vs json body
if(blob !== undefined) {
options.body = blob;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// handle blob vs json body
if(blob !== undefined) {
options.body = blob;
// handle binary body vs json body
if(body !== undefined) {
options.body = body;

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.

3 participants