melange-fetch icon indicating copy to clipboard operation
melange-fetch copied to clipboard

FormData binding problem

Open FilipKittnar opened this issue 3 years ago • 6 comments

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.

FilipKittnar avatar Sep 25 '20 15:09 FilipKittnar

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?

glennsl avatar Oct 13 '20 05:10 glennsl

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 avatar Oct 13 '20 15:10 yawaramin

In Chrome at least there are no issues with:

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

fd.append('bl', bl, undefined)

yawaramin avatar Oct 13 '20 15:10 yawaramin

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.

FilipKittnar avatar Oct 14 '20 05:10 FilipKittnar

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.

yawaramin avatar Oct 14 '20 14:10 yawaramin

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?

glennsl avatar Oct 18 '20 10:10 glennsl