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

Problem sending blobs #75

Closed
onel opened this issue Apr 12, 2020 · 8 comments
Closed

Problem sending blobs #75

onel opened this issue Apr 12, 2020 · 8 comments

Comments

@onel
Copy link

onel commented Apr 12, 2020

This might be more of a question than a bug.
I'm trying to send a blob using wretch and the it looks like only {} is being sent.

Example:

let blob = new Blob()
wretch('https://reqres.in/api/users/2').options({
  mode: 'cors'
}).put(blob)

Am I doing something wrong here?

@elbywan
Copy link
Owner

elbywan commented Apr 12, 2020

Hi @onel,

Am I doing something wrong here?

The thing is, .put(body) has a check on the type of the body argument. When the type is an object, it assumes that the body is JSON data and attempts to serialize it with an application/json content type.

In your case the typeof (new Blob()) === 'object' so it applies the "wrong" logic.

I could add a check specifically for Blob, but it would only work in the context of a web browser because Blob is not part of the nodejs standard lib.

So I think the correct way to send a Blob would be to use the body() method instead:

const b = new Blob([1, 2, 3])
wretch('...').body(b).put()//.res()/json()…

@onel
Copy link
Author

onel commented Apr 13, 2020

Thanks for the details @elbywan
Well, a couple of things about that:

  1. in the readme is mentions that .put() is a equivalent to calling .body().put() so I assumed the behaviour is the same. Maybe the doc should mention the difference or, make the .put/post() behave the same. Regarding the latter, the problem might be the same with TypedArrays
  2. Should wretch assume that the body should be a JSON? Would it work better if it looks at the content type header, and stringify if needed? If the user hasn't called .content() yeah, sure it can do that (see if it's an object) but right now even when specifically setting the header it still makes it a JSON.

@elbywan
Copy link
Owner

elbywan commented Apr 13, 2020

in the readme is mentions that .put() is a equivalent to calling .body().put() so I assumed the behaviour is the same.

The Readme actually states that the body is set in a opinionated way.

Excerpt:

// This shorthand:
wretch().post({ json: 'body' }, { credentials: "same-origin" })
// Is equivalent to:
wretch().json({ json: 'body'}).options({ credentials: "same-origin" }).post()

Maybe the doc should mention the difference or, make the .put/post() behave the same.

Indeed it is not clear enough and I will add a note to make it more explicit. Thanks for pointing this out!

Should wretch assume that the body should be a JSON?

Assuming that the payload is a JSON (which is much more common than a Blob or TypedArray) is a very handy shortcut to use. (wretch().put() instead of wretch().json().put())

If the user hasn't called .content() yeah, sure it can do that (see if it's an object) but right now even when specifically setting the header it still makes it a JSON.

True 👍.

I will make the change and remove the JSON assumption if the Content-Type header has been set already.

@elbywan
Copy link
Owner

elbywan commented Apr 13, 2020

Just published the version 1.7.2 with the fix 📦.

@onel
Copy link
Author

onel commented Apr 13, 2020

Awesome. Thanks for the quick fix 👍 This is a great lib

@elbywan
Copy link
Owner

elbywan commented Apr 17, 2020

Awesome. Thanks for the quick fix 👍 This is a great lib

Thanks! 😄

Closing the issue since the fix was published in the latest version.

@elbywan elbywan closed this as completed Apr 17, 2020
@RBrNx
Copy link

RBrNx commented Jun 29, 2021

Hey @elbywan, apologies for re-opening this but I felt it was relevant to the conversation. The changes made here are great, especially when compared to other fetch wrapper libs that don't support Blobs at all.

Unfortunately this only checks for the existence of a content-type: application/json header in the headers that are passed to the initial wretch() constructor, it does not account for headers that are passed by the .put() method.

It's fairly easy to workaround this by just passing the headers to the wretch constructor, but I thought it was an issue worth raising.

** Works - Blob is sent without being stringified **

wretch(s3PutObjectUrl, {
    headers: {
        'Content-Type':'image/jpeg',
        'Cache-Control': 'max-age=31557600',
    },
}).put(fileBlob),

** Doesn't Work - Blob is stringified before being sent **

wretch(s3PutObjectUrl).put(fileBlob, {
    headers: {
        'Content-Type':'image/jpeg',
        'Cache-Control': 'max-age=31557600',
    },
}),

@elbywan
Copy link
Owner

elbywan commented Jun 30, 2021

Hi @RBrNx,

apologies for re-opening this

No worries. 😄

Unfortunately this only checks for the existence of a content-type: application/json header in the headers that are passed to the initial wretch() constructor, it does not account for headers that are passed by the .put() method.

Absolutely, good catch, thanks for reporting this! I've just released a new version 1.7.5 with a fix.

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

No branches or pull requests

3 participants