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

FormData binding problem #37

Open
FilipKittnar opened this issue Sep 25, 2020 · 6 comments
Open

FormData binding problem #37

FilipKittnar opened this issue Sep 25, 2020 · 6 comments

Comments

@FilipKittnar
Copy link

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
Copy link
Contributor

glennsl commented Oct 13, 2020

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
Copy link
Contributor

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
Copy link
Contributor

In Chrome at least there are no issues with:

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

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

@FilipKittnar
Copy link
Author

FilipKittnar commented Oct 14, 2020

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
Copy link
Contributor

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
Copy link
Contributor

glennsl commented Oct 18, 2020

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?

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

No branches or pull requests

3 participants