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

Open
jimmywarting opened this Issue Aug 6, 2018 · 11 comments

Comments

Projects
None yet
2 participants
@jimmywarting

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

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

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

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

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Aug 6, 2018

Member

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.

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 Aug 6, 2018

@jimmywarting

This comment has been minimized.

Show comment
Hide comment
@jimmywarting

jimmywarting Aug 6, 2018

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.

jimmywarting commented Aug 6, 2018

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

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Aug 6, 2018

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@jimmywarting

jimmywarting 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

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

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Aug 6, 2018

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Aug 6, 2018

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@jimmywarting

jimmywarting 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 quest?" 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

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 quest?" 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

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Aug 6, 2018

Member

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 );
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

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Aug 6, 2018

Member

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.

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.

@timmywil timmywil reopened this Aug 6, 2018

@jimmywarting

This comment has been minimized.

Show comment
Hide comment
@jimmywarting

jimmywarting commented Aug 6, 2018

Thanks

@timmywil timmywil added Ajax and removed Needs review labels Aug 13, 2018

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Aug 13, 2018

Member

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.

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.

@timmywil timmywil added this to the 4.0.0 milestone Sep 3, 2018

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