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 helper function #3757

Merged
merged 3 commits into from Dec 23, 2021
Merged

toFormData helper function #3757

merged 3 commits into from Dec 23, 2021

Conversation

carpben
Copy link
Contributor

@carpben carpben commented Apr 17, 2021

With this functionality we can provide the user an option to pass a data object, instead of building FormData by himself. When the data is nested, building form data isn't trivial, and can result in an inappropriate structure and bugs. Hence usage of this functionality seems more reliable.

The function can handle nested data of the following types: object, array, string, number, boolean, null, undefined, Fiile.

Related feature request thread: #3721

This doesn't deal with the API in which this functionality will be offered to the user.

@jasonsaayman
Copy link
Member

jasonsaayman commented May 4, 2021

Hi @carpben

Can you maybe ad some documentation to the readme for how this is intended to work and give an example?

Thanks

@carpben
Copy link
Contributor Author

carpben commented May 7, 2021

Hi @jasonsaayman,
I'm not sure yet how this functionality should be used.
The most straight forward way would be to export the toFormData function, and allow the user to use it by himself.

import axios, {toFormData} from "axios"

axios({
  method: "post",
  url: "www.example.com/endpoint",
  data: toFormData(jsObject), 
  headers: { "Content-Type": "multipart/form-data" },
})

@DigitalBrainJS
Copy link
Contributor

DigitalBrainJS commented May 7, 2021

Wouldn't it make more sense to do this conversion automatically if the content type is multipart/form-data and data is a plain object? We can add an option to control this behavior, such as requestType or dataType, or just make it default behavior, temporarily disabled by a transitional option.

@carpben
Copy link
Contributor Author

carpben commented May 7, 2021

@DigitalBrainJS , it does make sense. If we can convert an object to FormData by default if content type is multipart/form-data it would be much more user friendly.

@carpben
Copy link
Contributor Author

carpben commented May 7, 2021

@DigitalBrainJS ,

Perhaps add a request method alias:

axios.postFormData(url, jsDataObject)

So instead of

axios({
  method: "post",
  url: "www.example.com/endpoint",
  data: toFormData(jsObject), 
  headers: { "Content-Type": "multipart/form-data" },
})

The user can use the equivelent:

axios.postFormData("www.example.com/endpoint", jsObject)

@carpben
Copy link
Contributor Author

carpben commented May 29, 2021

@jasonsaayman , @DigitalBrainJS ,
Any thoughts?

@carpben
Copy link
Contributor Author

carpben commented Jul 19, 2021

@jasonsaayman, Which direction do you think we should go in?

  1. Export toFormData directly (see code above)
  2. Add request method alias axios.postFormData (See code above)
  3. Folow @DigitalBrainJS suggestion - conver object to formData if content type is multipart/form-data

@jasonsaayman jasonsaayman merged commit 9964815 into axios:master Dec 23, 2021
3 checks passed
@caugner
Copy link
Contributor

caugner commented Jan 18, 2022

@carpben @jasonsaayman The release notes mention this toFormData helper function, which is very promising, but it doesn't seem to be exported to be used outside of axios?

@jasonsaayman
Copy link
Member

jasonsaayman commented Jan 18, 2022

Oh darn, I must have forgotten to do this, I wanted to ask @carpben to do it but clean forgot and then forgot myself too, I will update a pull request for that.

@carpben
Copy link
Contributor Author

carpben commented Jan 19, 2022

Thanks @caugner.
@jasonsaayman there is one thing we should address. Current solution won't work for node, as FormData is not available there natively.
Can we import 'form-data' if running in node?
Would be happy to prepare a PR.

@DigitalBrainJS
Copy link
Contributor

DigitalBrainJS commented Jan 19, 2022

It seems that the mix of 1 + 3 options is the best solution we can find here.
Axios already processes payload as JSON if the content type is application/json, and it would be logical to add a formData resolver for the multipart/form-data case too.
plainObject && multipart/form-data => stringify as FormData
plainObject || application/json => stringify as json

So we can just replace

axios/lib/defaults.js

Lines 74 to 77 in 1163588

if (utils.isObject(data) || (headers && headers['Content-Type'] === 'application/json')) {
setContentTypeIfUnset(headers, 'application/json');
return stringifySafely(data);
}

with

var isObjectPayload = utils.isObject(data);
var contentType = headers && headers['Content-Type'];

if ( isObjectPayload && contentType === 'multipart/form-data' ) {
  return toFormData(data);
} else if ( isObjectPayload || contentType === 'application/json' ){
  setContentTypeIfUnset(headers, 'application/json');
  return stringifySafely(data);
}

@xiaoyu-tamu
Copy link
Contributor

xiaoyu-tamu commented Jan 21, 2022

is this only work on browser?

Does nodejs native support global.FormData? I don't see any imports from the form-data package.

@carpben
Copy link
Contributor Author

carpben commented Jan 21, 2022

@xiaoyu-tamu as far as I know global.FormData is undefined. Currently toFormData is only supported in the browser. Adding support should be pretty straight forward by using form-data as a dependency.

@carpben
Copy link
Contributor Author

carpben commented Jan 21, 2022

@DigitalBrainJS I agree this is the right direction. But I'd rather wait with it for a bit.

  1. Solve Node case first
  2. Promote usage of toFormData in docs and elsewhere
  3. See how it is adopted by the community. Are there any bug reports or feature related requests?

Consider if you suggested change is a breaking change.
Could it be that some users inappropriately send a request with:

data instanceof FormData // false 
contentType // 'multipart/form-data'

And for their requests to somehow be successful?

@ankurk91
Copy link

ankurk91 commented Jan 22, 2022

I have been using this package in past

https://www.npmjs.com/package/object-to-formdata

@carpben
Copy link
Contributor Author

carpben commented Jan 23, 2022

I was not aware of object-to-formdata. There are 2 aspects the package supports that we currently don't:

  • bracket notation for nested values.
  • Date type.
    Perhaps we can consider it in the future. Thanks for sharing this @ankurk91 !

@jasonsaayman
Copy link
Member

jasonsaayman commented Jan 24, 2022

@DigitalBrainJS can you look at the package mentioned and see if we would rather look into that? We can add that but there is a cost that might be a bit high, I am in favour at looking at native support for it's feature set

mbargiel pushed a commit to mbargiel/axios that referenced this issue Jan 27, 2022
* adding toFormData test

* adding toFormData

Co-authored-by: Jay <jasonsaayman@gmail.com>
@carpben carpben mentioned this pull request Feb 4, 2022
@saschagehlich
Copy link

saschagehlich commented Mar 29, 2022

I believe this change causes axios to not recognize FormData objects in react-native. I stumbled over this issue after updating axios to the latest version. Suddenly, axios started stringifying FormData objects.

For everyone finding this, here's a workaround to making it work. Add this to your request's options, formData being your FormData object:

{
    transformRequest: (data, headers) => {
        return formData;
    },
}

This causes axios to not transform your data.

@radko93
Copy link

radko93 commented Mar 29, 2022

Thanks a lot @saschagehlich, this is completely breaking React Native form data requests.

@myrs
Copy link

myrs commented Mar 31, 2022

Thanks @saschagehlich, this broke my requests in react-native as well! Actually, as mentioned here, the workaround can be even more concise:

{
  //..
  transformRequest: data => data
  //..
}

I've tested on react-native=0.67.4 and axios=0.26.1 and it works.

@carpben
Copy link
Contributor Author

carpben commented Apr 2, 2022

@saschagehlich The issue was probably caused by a different change (change to isFormData detection). It was fixed by #4448 (not published yet, should be soon).

@jasonsaayman
Copy link
Member

jasonsaayman commented Apr 4, 2022

Working on the release will be done asap, just trying to get a couple things done and try not introduce any breaking changes on 0.26.2

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

Successfully merging this pull request may close these issues.

None yet

9 participants