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

Make ajax understand other acceptable data #4150

Closed
jimmywarting opened this issue Aug 6, 2018 · 14 comments
Closed

Make ajax understand other acceptable data #4150

jimmywarting opened this issue Aug 6, 2018 · 14 comments

Comments

@jimmywarting
Copy link

jimmywarting commented Aug 6, 2018

How to send a file or formdata with jQuery is brought up a lot at StackOverflow

I think you should do things better by trying to understand what kind of data they are sending without having to do processData = false and contentType = false

that is why i propose this should be possible

jQuery.ajax({
  url: url, 
  method: 'POST',
  data: FormData || typed arrays || Blob || File || UrlSearchParams

  // without the need of this:
  // processData: false,
  // contentType: false
});

// and also 
jQuery.post(url, FormData || typed arrays || Blob || File || UrlSearchParams)

What i usually do is propose using the fetch api instead of jquery's ajax, this is something that you don't make any easier for the developers, it's quite the opposite. It makes it harder and more confusing to why things don't work

@timmywil
Copy link
Member

timmywil commented Aug 6, 2018

Thank you for the issue. However, jQuery automatically processes data to query strings that can be sent in GET requests. This is likely still the most common use case. Besides, changing the defaults of these options would break a lot of code.

@timmywil timmywil closed this as completed Aug 6, 2018
@jimmywarting
Copy link
Author

Sorry, i fail to see how something like that could break when it don't even work in the first place.

I didn't mean you should change the default settings of processData & contentType to false. I only meant that you should check whatever if data is a instanceof FormData and if it's do the correct thing.

@timmywil
Copy link
Member

timmywil commented Aug 6, 2018

The default would have to be changed if you don't want to set contentType at all since the default is application/x-www-form-urlencoded. However, rather than removing the Content-Type header completely, you could set contentType to the more appropriate "multipart/form-data". As for processData, the default behavior is to try to convert the data to a string. It's been like this a long time. We could start adding exceptions to this (it probably wouldn't stop with FormData), and that is where breaking existing code comes in. Plugins or custom methods that handle the correct $.ajax options for each of these types of requests would be simpler (no added magic needed in $.ajax) and prettier (e.g. sendFormData()). Besides, it seems to me if you're going to use FormData, you may as well use fetch.

@jimmywarting
Copy link
Author

jimmywarting commented Aug 6, 2018

you could set contentType to the more appropriate "multipart/form-data"

You shouldn't have to set the request contentType header yourself to multipart/form-data since the boundary has to be added (something of which xhr and fetch do for you automatically)

Content-Type: multipart/form-data; boundary=----WebKitFormBoundaryAtoyumhGsqZNfA8E

just setting the content to multipart/form-data would break the backend code to not know how to read the boundary. I think you should not set the request content type at all if the data is a FormData. just let the XHR handle it

the default behavior is to try to convert the data to a string. It's been like this a long time.

Well, the world is changing and i think you are laking behind. It's grate that you try to convert everything to string but not everything can not and should not be casted to a string such as FormData which just becomes [Object Formdata] (along with Blob/File/DataView and typed arrays)

We could start adding exceptions to this

I think you should. And please don't add something like sendFormData(). $.ajax() and $.post() is good enough. ppl, could stop having to write the long $.ajax() version and go back to the simpler more sort version of just doing $.post(url, formdata) but you are holding them back. And please don't add something else for .blob() .file() and typedArrays() also cuz that would be to much... you can handle them if you put your mind to it.

Besides, it seems to me if you're going to use FormData, you may as well use fetch.

I'm not talking for myself, I don't use jQuery any longer but I get that other people do. And I want to help them. The fetch API is not always an option since not even IE11 has the fetch api. Microsoft is still going to support it until 2025 something and other follows this rule and even down to IE9 still today.

and that is where breaking existing code comes in

Still don't see any breaking changes, it would just be an added functionality? if it is a FormData instance, avoid setting the contentType and don't try to process it to a string - simple, no?
When would it ever be good to do the default thing of trying to cast FormData, blob and typed arrays to string? never! you can do the default thing for other type of data
Might be missing something, haven't used jQuery for a couple of years or so. you might have to give me a code example
If not now, then do it in the next major release?


I see similar question like this pop up all the time at SO

  1. How to send FormData objects with Ajax-requests in jQuery?
  2. Sending multipart/formdata with jQuery.ajax
  3. How to use FormData for ajax file upload

@timmywil
Copy link
Member

timmywil commented Aug 6, 2018

I wasn't suggesting a core method when I suggested sendFormData. As I said, it would be a plugin or some function in user code. As the answers to those SO questions indicate, this is very easily addressed without additions to jQuery core. Nevertheless, since you feel strongly about it, feel free to submit a PR and we'd be happy to review it. Otherwise, this will likely stay a wontfix. It should probably be a an ajaxPrefilter, as Rick suggested in an old ticket.

@timmywil
Copy link
Member

timmywil commented Aug 6, 2018

I'll go ahead and answer this...

Still don't see any breaking changes, it would just be an added functionality?

No, it would not just be added functionality. You can't always predict how a change like this will affect millions of lines of code in the wild. Would it actually result in breakages? I don't know. We've seen over and over again how behavior changes affected users in ways we never expected. You just have to be extra careful. And this is definitely a change in behavior, which is synonymous with a breaking change when a library has a lot of users.

That said, sometimes we deem it necessary and introduce the change in a major version. To me, this doesn't seem critical enough to warrant the change, but the best way to persuade any of the core team is with code.

@jimmywarting
Copy link
Author

jimmywarting commented Aug 6, 2018

I wasn't suggesting a core method when I suggested sendFormData. As I said, it would be a plugin or some function in user code.

ah, My misstake.

As the answers to those SO questions indicate, this is very easily addressed without additions to jQuery core

IMO it's more like: "Why do they ask this question?" you don't make it easy for them.

just played around with prefilter, where not that easy when you cast an error the first thing that happens. Guess you need to make any core changes to let this slide

$.ajaxPrefilter(function(...args) {
  // write a potential fix that never logs anything
  console.log(args)
});

$.ajax({
  url: '/',
  method: 'post',
  data: new FormData()
})
Uncaught TypeError: Illegal invocation
    at add (jquery-1.11.3.js:9508)
    at buildParams (jquery-1.11.3.js:9497)
    at Function.jQuery.param (jquery-1.11.3.js:9528)
    at Function.ajax (jquery-1.11.3.js:9099)
    at <anonymous>:1:3

@timmywil
Copy link
Member

timmywil commented Aug 6, 2018

You're right, prefilters seem to be applied after the data is converted. The following blocks could perhaps be swapped in ajax.js on line 561. I can't think of a reason why this would be a problem, but I wonder if @gibson042 @jaubourg @dmethvin @mgol @markelog might.

                // Convert data if not already a string
		if ( s.data && s.processData && typeof s.data !== "string" ) {
			s.data = jQuery.param( s.data, s.traditional );
		}

		// Apply prefilters
		inspectPrefiltersOrTransports( prefilters, s, options, jqXHR );

@timmywil
Copy link
Member

timmywil commented Aug 6, 2018

Reopening to investigate the possibility of a prefilter, or at least investigate the possibility of swapping the above blocks of code to enable user prefilters to get at data before it goes to jQuery.param.

@jimmywarting
Copy link
Author

Thanks

@timmywil
Copy link
Member

timmywil commented Aug 13, 2018

Discussed this in the meeting and we're open to exploring this. First step is to the swap those blocks mentioned above and see if any tests break.

@mgol
Copy link
Member

mgol commented Aug 8, 2022

I tried swapping the blocks in ajax.js as @timmywil suggested in #4150 (comment) and all tests were still passing in the latest Chrome & Firefox.

mgol added a commit to mgol/jquery that referenced this issue Jan 16, 2023
Two changes have been applied:
* prefilters are now applied before data is converted to a string;
  this allows prefilters to disable such a conversion
* a prefilter for binary data is added; it disables data conversion
  for non-string non-plain-object `data`; for `FormData` bodies, it
  removes manually-set `Content-Type` header - this is required
  as browsers need to append their own boundary to the header

Ref jquerygh-4150
@mgol
Copy link
Member

mgol commented Jan 16, 2023

PR adding support (we still need to discuss it, no decision made yet): #5197.

Input on that appreciated.

mgol added a commit to mgol/jquery that referenced this issue Jan 16, 2023
Two changes have been applied:
* prefilters are now applied before data is converted to a string;
  this allows prefilters to disable such a conversion
* a prefilter for binary data is added; it disables data conversion
  for non-string non-plain-object `data`; for `FormData` bodies, it
  removes manually-set `Content-Type` header - this is required
  as browsers need to append their own boundary to the header

Ref jquerygh-4150
mgol added a commit to mgol/jquery that referenced this issue Jan 23, 2023
Two changes have been applied:
* prefilters are now applied before data is converted to a string;
  this allows prefilters to disable such a conversion
* a prefilter for binary data is added; it disables data conversion
  for non-string non-plain-object `data`; for `FormData` bodies, it
  removes manually-set `Content-Type` header - this is required
  as browsers need to append their own boundary to the header

Ref jquerygh-4150
mgol added a commit that referenced this issue Feb 1, 2023
Two changes have been applied:
* prefilters are now applied before data is converted to a string;
  this allows prefilters to disable such a conversion
* a prefilter for binary data is added; it disables data conversion
  for non-string non-plain-object `data`; for `FormData` bodies, it
  removes manually-set `Content-Type` header - this is required
  as browsers need to append their own boundary to the header

Ref gh-4150
Closes gh-5197
@mgol
Copy link
Member

mgol commented Feb 1, 2023

#5197 should be fixing it for 4.0; I should have marked it as Fixes, not Ref. Closing as done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants