#12004 - AJAX - Alias options.method to options.type #1007

Closed
wants to merge 7 commits into
from

Projects

None yet

5 participants

@farmdawgnation

I was in the mood for a bite-sized feature this morning, and didn't see that anyone had grabbed this one yet.

This pull request adds support for an additional AJAX option, method, that is aliased to the AJAX option type, in the fashion described in ticket #12004. For backward compatibility this is a simple alias, and the type argument will continue to work. When both are specified, method will win out.

Size comparison with this new feature.

    267694      (+113)  dist/jquery.js                                         
     94023       (+24)  dist/jquery.min.js                                     
     33618       (+15)  dist/jquery.min.js.gz    

Additionally, two new tests were added that simply mimic the simple GET/POST tests with the difference that they use method instead of type.

I ran the unit tests with the following results.

  • Firefox - PASS
  • Chrome - No additional failures.
    • There's a failure for jQuery.each that seems to already exists in master.
  • Safari - PASS
  • Opera - PASS
  • IE 9 - No additional failures.
    • From what I can tell master had a handful of tests already failing.
  • IE 8 - No additional failures.
    • See notes for IE 9.
  • IE 7 - No additional failures.
    • See notes for IE 9.
  • IE 6 - ?
    • Sauce scout chokes and I can't get an IE6 VM working. :/ Going to try again shortly but if someone has a spare cycle and an IE 6 VM already spun up I would love you forever.

Will update the PR when the status of IE 6 is solidified when either a) Sauce stops being stupid, b) I can get a working VM, or c) someone donates their time for me, jQuery, and the good of mankind. Considering that everything else is looking solid, I wanted to go ahead and open this PR and start discussion on merging.

Let me know if I missed anything. Cheers!

farmdawgnation added some commits Oct 24, 2012
@farmdawgnation farmdawgnation For AJAX options, alias options.method to options.type.
This will allow developers to use the word "method" in their jQuery.ajax
call to specify the HTTP method they would like to use for the AJAX
request. As per ticket #12004, we alias this instead of completely
nuking type so as to preserve backward compatibility.
8fb352d
@farmdawgnation farmdawgnation Duplicate simple get/post tests using method instead of type. cb2bab8
@jaubourg jaubourg and 1 other commented on an outdated diff Oct 24, 2012
src/ajax.js
@@ -335,6 +335,9 @@ jQuery.extend({
// Force options to be an object
options = options || {};
+ // Alias options.method to options.type as per ticket #12004
+ options.type = options.method || options.type;
@jaubourg
jaubourg Oct 24, 2012 jQuery member

What about when method is defined in ajaxSettings? You'd probably better do some magic on s or maybe even in ajaxSetup.

@farmdawgnation
farmdawgnation Oct 25, 2012

That's a good thought. Hm.

@farmdawgnation
farmdawgnation Oct 25, 2012

So, I think I'm going to go with implementing it in jQuery.ajax. Determining the correct method from the settings is the job of the ajax handling function, not of the settings handling itself.

@jaubourg
jaubourg Oct 25, 2012 jQuery member

I dunno, I'd expect aliasing a "setting" to be somehow related to "settings handling", wouldn't you say?

@farmdawgnation
farmdawgnation Oct 25, 2012

No, I'd disagree. Perhaps my choice of terminology was poor, but saying two settings are equivalent involves some insight into the meaning of the settings. The ajaxSetup method doesn't know anything about the meaning of the settings it's operating on, and I would argue that it shouldn't grow that knowledge now. On the other hand, jQuery.ajax does know the meaning of the settings its working with because it must to do its job. (It needs to know what kind of request to send, for example.)

In all honesty, if we were aliasing more than this one parameter, I'd be more in favor of extracting the aliasing into a different function before plugging it into ajaxSetup, but given that we're only hitting this one parameter I'm not sure the readability justifies the added bytes.

Interested in hearing other thoughts around these ideas, however.

@farmdawgnation farmdawgnation Do our aliasing on the s variable instead of options.
We want to make sure that the combined default settings for AJAX
requests observe this behavior as well.
fe04f2e
@dmethvin
Member

Amazing how complicated it can be to do something "simple" like create an alias for an existing property! Conceptually it does seem like this belongs in $.ajax if we're treating ajaxSetup as a know-nothing property bag. TBH I'd love to mark ajaxSetup as deprecated since global settings are such a bad idea. I'm not in favor of adding some type of alias layer, it's already a pretty big byte size for something trivial.

@farmdawgnation

Yeah, I was actually a bit surprised at the byte size change. Although, it gets a bit smaller in a commit I haven't pushed yet. heh. That said, I guess the question is whether or not jQuery eventually wants to using method altogether instead of type in some future version. That's a question for you guys to answer. If so, this would be the first step of sorts. If not, then we can all go home and call it a day. 💥! :D

@dmethvin
Member

I have a feeling we'll be supporting them both fer-ever, but having the method name seems good.

@rwaldron rwaldron commented on an outdated diff Oct 25, 2012
test/unit/ajax.js
@@ -2217,6 +2217,33 @@ if ( jQuery.ajax && ( !isLocal || hasPHP ) ) {
});
});
+ test( "jQuery.ajax - simple get with options.method", function() {
@rwaldron
rwaldron Oct 25, 2012 jQuery member

use asyncTest

@rwaldron rwaldron commented on an outdated diff Oct 25, 2012
test/unit/ajax.js
@@ -2217,6 +2217,33 @@ if ( jQuery.ajax && ( !isLocal || hasPHP ) ) {
});
});
+ test( "jQuery.ajax - simple get with options.method", function() {
+ expect( 1 );
+ stop();
+ jQuery.ajax({
+ method: "GET",
+ url: url("data/name.php?name=foo"),
+ success: function( msg ) {
+ equal( msg, "bar", "Check for GET" );
+ start();
+ }
+ });
+ });
+
+ test( "jQuery.ajax - simple post with options.method", function() {
@rwaldron
rwaldron Oct 25, 2012 jQuery member

use asyncTest

@rwaldron rwaldron commented on an outdated diff Oct 25, 2012
test/unit/ajax.js
@@ -2217,6 +2217,33 @@ if ( jQuery.ajax && ( !isLocal || hasPHP ) ) {
});
});
+ test( "jQuery.ajax - simple get with options.method", function() {
+ expect( 1 );
+ stop();
@rwaldron
rwaldron Oct 25, 2012 jQuery member

remove

@rwaldron rwaldron commented on an outdated diff Oct 25, 2012
test/unit/ajax.js
+ test( "jQuery.ajax - simple get with options.method", function() {
+ expect( 1 );
+ stop();
+ jQuery.ajax({
+ method: "GET",
+ url: url("data/name.php?name=foo"),
+ success: function( msg ) {
+ equal( msg, "bar", "Check for GET" );
+ start();
+ }
+ });
+ });
+
+ test( "jQuery.ajax - simple post with options.method", function() {
+ expect( 1 );
+ stop();
@rwaldron
rwaldron Oct 25, 2012 jQuery member

remove

@farmdawgnation

Alright, well, in that case I'm going to finish testing and push my fix to the bug @jaubourg found and make the changes @rwldrn just requested to the test suite and push that. :)

@rwaldron
Member
@farmdawgnation

So, trying to run the entire test suite in IE 6 appears to be crashing the browser on Sauce Scout and I can't get a working VM right now. The AJAX suite only has issues with: "Opera is incapable of doing .setRequestHeader('If-Modified-Since').", but from what I can tell that test is failing in master as well. All other AJAX module tests pass.

@dmethvin
Member

go ahead and commit to this branch to update your pull, and we'll see what botio thinks.

@farmdawgnation

Done. Update size diff.

Sizes - compared to master @ 5dc37bb0b56e9eba94d10eabeea2d49496d80c3f
    267668       (+87)  dist/jquery.js                                         
     94023       (+24)  dist/jquery.min.js                                     
     33615       (+12)  dist/jquery.min.js.gz
@dmethvin
Member

/botio test

@jaubourg jaubourg and 3 others commented on an outdated diff Oct 25, 2012
src/ajax.js
@@ -426,6 +426,9 @@ jQuery.extend({
}
};
+ // Alias method option to type as per ticket #12004.
+ s.type = s.method || s.type;
+
@jaubourg
jaubourg Oct 25, 2012 jQuery member

This still won't work. Consider if you set method on ajaxSettings and type on the options given to ajax. This will always use the value of methodin ajaxSettings. That's why I still think it would be better to handle this in ajaxSetup: the logic is not that simple.

@farmdawgnation
farmdawgnation Oct 25, 2012

Doh!

facepalm

Now I'm understanding @dmethvin's remark about global settings being a bad idea. Ok. I'm walking out the door. I can look at refactoring this later.

@dmethvin
dmethvin Oct 25, 2012 jQuery member

I think this just goes to show, no pull request that starts "I was in the mood for a bite-sized feature this morning..." will be simple.

@farmdawgnation
farmdawgnation Oct 26, 2012

One day I'll learn.....

@curiousdannii
curiousdannii Oct 26, 2012

Would this account for all possibilities?

s.type = options.method || options.type || s.method || s.type;

@farmdawgnation
farmdawgnation Oct 26, 2012

I suspect it would. @dmethvin and @jaubourg - I'm not particularly a fan of a four condition OR, but dropping it in ajaxSetup doesn't exactly let us escape from something like that... how do you guys feel about @curiousdannii's solution?

@dmethvin
Member

I think that implementation should work, but the unit tests don't really test any of this so those need some more cases tested.

@farmdawgnation

So, I'm seeing some weirdness in the unit tests with these recent commits. Specifically, in Chrome, the test labeled "script, Remote with POST" yields the following error periodically in Chrome "ajax error, status code: 412". Not entirely sure what's up with that. Firefox, Opera, and Safari appear to be kosher. I don't know if that's something funky in Chrome or in my configuration of Apache.

Moving onto testing the IE's. (yay).

@farmdawgnation

Ah, balls, I had to run out the door before finish that IE testing and forgot about it. Sorry gents. Let me spin up sauce today and run those unit tests.

@farmdawgnation

Ok, so the IE's are being particularly snarky with me. I don't know enough about the way the test suite is designed to be able to determine if I bunked something up or not. The worse part is these errors don't seem to be consistent across runs. :| I'm running the IE's in a remote VM hosted on Sauce, so I guess it's possible my network tunnel isn't working well right now? Hoping @dmethvin can shed some light.

Screen shots below are from IE7-10. 6 behaved essentially the same way so I stopped taking screen shots. heh.

IE7

IE8

IE9

IE10

@dmethvin
Member

@farmdawgnation don't be discouraged, there's a lot of moving parts here. I can take what you've got and work with it.

@farmdawgnation

Ok, cool @dmethvin. I was genuine about wanting to see the working solution to these tests explained if possible. I do want to continue to try and become more involved in jQuery, and understanding the test suite is going to be a part of that.

@farmdawgnation

Alright @jaubourg and @dmethvin. Let me know how the latest revision of the tests look.

@farmdawgnation

FYI. I tested in FF and Chrome. I want to wait on the review before running the entire suite. Cheers.

@dmethvin
Member

@jaubourg is on the verge of landing some rewrites to the ajax unit tests, so I'll wait for those before landing.

@jaubourg
Member

Tests look much better now and the new ajaxTest will make them even better... it's the conversation here that triggered the idea of a helper btw.

@farmdawgnation

Nice! 👍 Looking forward to seeing it.

@jaubourg
Member

FYI, I toyed around with a generic aliasing solution (detecting aliases defined in jQuery.ajaxSettings.alias and setting the proper option in ajaxExtend). It's only +28 min/gzipped which is quite good for such a generic approach. Using the approach, you could override type to set dataType if you so desired:

$.ajaxSetup({
    alias: {
        type: "dataType"
    }
});

So it would break existing requests but new projects could start doing things like shown below (and it would work as expected).

$.ajax( "myService.php", {
    method: "GET",
    type: "json",
});

I am a bit surprised this doesn't take more bytes. And note that the limitation is that you have to set global options using ajaxSetup() (but that's how everyone should do it in the first place anyway).

Anyway, I wanted to share the unit test I had for this:

jQuery.each( [ "method", "type" ], function( _, globalOption ) {

    function request( option ) {
        var options = {
                url: url("data/echoData.php"),
                data: "hello",
                success: function( msg ) {
                    strictEqual( msg, "hello", "Check for POST (no override)" );
                }
            };
        if ( option ) {
            options[ option ] = "GET";
            options.success = function( msg ) {
                strictEqual( msg, "", "Check for no POST (overriding with " + option + ")" );
            }
        }
        return options;
    }

    ajaxTest( "#12004 - jQuery.ajax() - method is an alias of type - " + globalOption + " set globally", 3, {
        setup: function() {
            var options = {};
            options[ globalOption ] = "POST";
            jQuery.ajaxSetup( options )
        },
        requests: [
            request("type"),
            request("method"),
            request()
        ]
    });

});

It doesn't require any additional php script and uses the new ajaxTest helper. Thought it would be a good example.

@dmethvin
Member

I think we should wait until we've needed to alias enough settings for this to be a net savings. Otherwise it seems like a YAGNI change. In general we shouldn't be open to aliasing a lot of settings, it's just that this one was named incorrectly from the start 😀 .

@jaubourg
Member

While I understand the generic approach may seem heavyweight, fact is the specific approach is more than half the size of the generic one. It's not like we haven't been asked for a means to use type instead of dataType, the problem is we cannot do that without breaking everything... unless we have a facility for people to do what they want in their own environment. We're talking 28 bytes against 15 here, something to ponder I think.

@dmethvin
Member

I don't think we'd ever want to advocate that anyone use type for dataType, that just confuses things further because type has an existing meaning in the API and everyone would need to look at the decoder ring to know the new names. If you tried to use a plugin that expected type to be the method, but you'd renamed it to dataType it would be a mess. That's handing people a loaded gun and asking them to be careful.

@jaubourg
Member

Well, you can already do crazy stuff with ajax options inside a prefilter, so the "loaded gun" is already in everyone's hand. I only gave the type/dataType example to show how flexible the generic approach is. Again, it's 28 bytes against 15 bytes and will be very handy when we decide to deprecate or plain remove type. A compat plugin would just need something along the line of $.ajaxSetup({ alias: { type: "method" } }) to be backward compatible because the approach works both ways. YAGNI is fine when it actually applies,IMHO.

@dmethvin
Member

Sorry but I still don't see the need for a generalized name remapping layer in ajax, even if it only costs a few bytes. I would not want to document this for general use by the public, especially since I think we can all agree that global ajax options are a very bad practice in any codebase that includes third-party code expecting the documented defaults.

I just want to get the naturally-named method in here, and am comfortable with stopping there.

@dmethvin dmethvin closed this Nov 28, 2012
@jaubourg
Member

No, I strongly disagree with that, Dave.

Global ajax options are not bad practice, they are just awfully badly implemented in jQuery because they mix actual global configuration options and plain default options. It's the latter that causes us problems all the time, including in this very PR, not the former, far from it.

I'm perfectly fine with contents, converters and flatOptions (the latter being undocumented btw) being exposed globally as a means to configure $.ajax(), just like I'm fine with prefilters because they all fall in the global configuration category. Is the fact they're specifically designed to break code expectations throughout an entire application an issue? Of course, not. That's why they exist in the first place.

I'm also fine with being able to override those global configurations on a per-request basis. Most useful example is probably a converter being overriden because a specific webservice cannot output properly formatted data.

No, the real problem lies with default options that are not at all related to configuration. This covers pretty much any option that wasn't mentionned here already. mimeType and timeout come to mind. While it seems convenient to have default for those, they turn out to be more trouble than they are worth because they break code expectations mindlessly. They do not apply to a range of requests, they don't cover several request/reponse scenarios, they're just obfuscating code and making things unpredictable for no sensible reason.

I am in favour of an alias configuration, undocumented like flatOptions. We screwed up with the name of several options. We won't fix all of them but we could use an aliasing facility as a means to deprecate then remove at least type here while leaving a backward compatibility hook. If we screw up with naming stuff, so do our users, something to ponder when we gave them prefilters as a means to create and support their own options.

s.type = options.method || options.type || s.method || s.type; does the job but it's as ad hoc, clunky and unreadable as it can get. Now, it's not @farmdawgnation's fault. I would have written it exactly like this but I think we know better than pushing the first idea we have in the repo because it gets the work done. We do, right? I know in which state I found $.ajax() and its always ad-hoc/minimal/patchy code. Never again, please.

So, no, we won't agree on the global options being bad practice part of your argumentation, because it's just not quite where the problem lies.

Also, I compare sizes because a special cased code being in the same order of magnitude in code size as a generic approach is not random. How I wish it was always the case. Nobody would have bothered me with the bump in size of the ajax module :P

@dmethvin
Member

Okay, well I still feel like this isn't code well spent. Let's wait until we encounter the problem before we add this. We have so much more to do before 1.9 so I'd prefer to just land this and move to the next item on the list.

@dmethvin
Member

Hmm, didn't mean to close this...

@dmethvin dmethvin reopened this Nov 28, 2012
@jaubourg
Member

You probably hit the close & comment button... I always have to think twice everytime I comment on a PR :(

@farmdawgnation

The buttons are indeed confusing. ;) Alright, so am I cool to run these tests in the entire fleet of browsers or do I need to make any additional changes to them?

@dmethvin
Member

We still have to fix up the tests, right? This needs a rebase against the new ajax test work.

@farmdawgnation

Ok, where's that at right now? Do I need to wait until it goes through its own review before doing so?

@dmethvin
Member

@jaubourg landed it already, it's ready to go. @jaubourg could you fix up the tests for this? I'm working on a couple of other pulls at the moment.

@jaubourg
Member

Not right now, no. It's 2:30am :P and I was about to go to bed... I so hate when I just write a comment and it takes forever :(

@farmdawgnation: what you can do is revert your last commit, then rebase (this shouldn't cause any problem without the unit test), then re-put your test in or, preferably, look at the one I put in a comment up there. Last part isn't mandatory. I can handle the unit testing so that you don't have too much work.

@jaubourg
Member

@dmethvin I'll merge this tomorrow morning my time... meaning in a few hours :P

@jaubourg
Member

@farmdawgnation, I commited your solution in a new commit and in your name: 081c4ef I used the unit test I had in a subsequent commit: 6378a19

Thanks for your work on this and be prepared for @dmethvin asking you to sign something with your blood regarding copyright and stuff ;)

@jaubourg jaubourg closed this Nov 28, 2012
@farmdawgnation

Excellent! :) Glad to be able to help.

This'll teach me to call something in jQuery bite-sized... although it wasn't quite as painful as the mobile webkit nastiness I dealt with on my last go-round. XD

Cheers.

@jaubourg
Member

Welcome to our world :P

@dmethvin dmethvin referenced this pull request in jquery/api.jquery.com Dec 14, 2014
Closed

jQuery.ajax() supports "method" in preference to "type" #609

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