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

ToFormData - Which way should we proceed? #4471

Closed
carpben opened this issue Feb 13, 2022 · 6 comments
Closed

ToFormData - Which way should we proceed? #4471

carpben opened this issue Feb 13, 2022 · 6 comments

Comments

@carpben
Copy link
Contributor

carpben commented Feb 13, 2022

#3757 added a function converts a JS object to a FormData format.
My suggestion was, to export it independently for now, and proceed in a risk free approach.
@DigitalBrainJS thought we should add it by default if data is not in FormData format, and content-type is multipart/form-data. @DigitalBrainJS merged #4413 accordingly.

Recent FormData issues (such as #4406) show that tests related to FormData are lacking. . If we do decide to publish #4413 we need to make sure that there are proper tests for using toFormData by default. More specifically, that current usage, passing data in FormData format, is not affected at all by the change.

@jasonsaayman

Is your feature request related to a problem? Please describe.

Describe the solution you'd like

Describe alternatives you've considered

Additional context

@jasonsaayman
Copy link
Member

Can you please add this to the discussion :)

@jasonsaayman
Copy link
Member

See #4474

@DigitalBrainJS
Copy link
Collaborator

we should add it by default if data is not in FormData format, and content-type is multipart/form-data

You miss the point. The transformation to the FormData instance is applied only if the payload is a plain object and content-type is multipart/form-data, not for any payload with this content header. It's a very important difference.
Any object passed as a payload to Axios must be serialized in some way, and by default it uses JSON.stringify() no matter what content type is set.

So the question is:

Do Axios users pass a plain object as a payload with multipart/form-data and actually expect the payload to be passed as JSON?

Most Likely Answer:

No. Passing JSON with multipart/form-data will break all the standards. But even if the user wants to do this, they can still manually use JSON.stringify before passing the payload to axios.

Therefore, I believe this change will not break existing user projects in any way.

And yes, the serialization quality of that helper is not good enough. I'm working on better implementation.

@carpben
Copy link
Contributor Author

carpben commented Feb 13, 2022

@DigitalBrainJS I did not miss that point (see quote at the beginning of your comment 🙂).
Your claim that currently JS object are serialized to JSON is a strong one, and it makes sense to be consistent.

I thought we need to have a test if a user passes FormData current change doesn't affect it in any way. But I noticed a test you added in #4448 that solves this issue.

I guess my only reservation currently is that at first we should not recommend it as the preferred method of choice in the documentation. Be more gradual. First see how it is accepted (No major issues/bugs) and only then recommend it as the first/default method.

@carpben
Copy link
Contributor Author

carpben commented Feb 13, 2022

Can you please add this to the discussion :)

Sorry @jasonsaayman . @DigitalBrainJS replied so perhaps we can keep this discussion here and have the next one in the discussion tab.

@carpben
Copy link
Contributor Author

carpben commented Feb 13, 2022

Discussion moved to #4474 (comment).

@carpben carpben closed this as completed Feb 13, 2022
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