melange-community / melange-fetch

Fetch bindings for Melange
MIT License
203 stars 53 forks source link

FormData binding problem #37

Open FilipKittnar opened 3 years ago

FilipKittnar commented 3 years ago

Hello, I tried the recently added FormData binding in a ReScript project exactly according to your example with appendObject and it doesn't work. The problem is that the JS snippet it generated looks like this:

              var formData = new FormData();
              formData.append("image", {
                    type: "image/jpeg",
                    uri: photoUri,
                    name: "image.jpg"
                  }, undefined);

The undefined as the third parameter is the problem. There must be no third parameter, not even undefined. If I remove it from the gerenated JS file, it works correctly. If I understand the FormData API correctly the third parameter (filename) is used only for Blobs and Files.

glennsl commented 3 years ago

Hmm, yeah the filename argument doesn't make much sense for objects, so I think it's fairly safe to just remove it. I do wonder what happens if it's omitted and generates undefined for files and blobs as well. Would it be better if it was mandatory for those?

Any thoughts, @yawaramin, @anmonteiro?

yawaramin commented 3 years ago

AFAIK appendObject was intended only for React Native, it doesn't work for browsers. @FilipKittnar where are you running the output JS? In React Native? Can you show the exact error message?

yawaramin commented 3 years ago

In Chrome at least there are no issues with:

var fd = new FormData()
var bl = new Blob()

fd.append('bl', bl, undefined)
FilipKittnar commented 3 years ago

AFAIK appendObject was intended only for React Native, it doesn't work for browsers. @FilipKittnar where are you running the output JS? In React Native? Can you show the exact error message?

Unfortunatelly I cannot provide anything anymore because in the end I had to completely refactor the code and used the image in base64 instead of form data. It just didn't work with form data. I think you might be true, because originally we had this in React Native and it was working fine. We were changing the app to PWA, which means normal web React and I started to have problems with this. I'm pretty sure the error was something like "this image is not a blob" or something like that. If I altered the generated JS file and removed the undefined, the error was gone. But later (when I did my own binding for the function) I found out that it didn't work even like that, because the form-data in the request was not correct and server was not accepting it. So you might be right, this is probably only for React Native. It was probably my misunderstanding of how form-data works.

yawaramin commented 3 years ago

No problem. As it turns out, we documented the issue with React Native in the interface file: https://github.com/reasonml-community/bs-fetch/blob/934389964e1005d4e37911c8324a6aeb7ce0a1b0/src/Fetch.mli#L260 . Maybe we should warn more explicitly there that it won't work in the browser.

glennsl commented 3 years ago

Thanks for the input @yawaramin. I forgot that this was only for React Native, and I'm not entirely sure why I accepted it. I don't think I would today. So I think there are a few different ways we could move forward on this:

  1. Do nothing and expect people to read the documentation.
  2. Remove it as out-of-scope and expect people to bind to it themselves or use a library that extends bs-fetch for React Native.
  3. Move it to a ReactNative namespace module to make it explicit in code that this is platform-specific.

I'm leaning towards 2. What do you guys think?