[PATCH] The Ajax mega-patch

That's a nice bunch of additions. The only thing I'm a bit curious about is the need for #4128 (multiple update receivers) -- are there any specific use cases for this, that can't be solved by say, generating JavaScript on the server? It just doesn't really look like something too useful.

Anyway, I think the patch is heading in the right direction. When the remaining stuff is finished and more test coverage is added, I'm looking forward to see this as part of Prototype. Thanks for doing the hard work and for the help to clean up the bug backlog! :)

Replying to madrobby:

The only thing I'm a bit curious about is the need for #4128 (multiple update receivers)

Well, to be honest, that didn't came to my mind by itself, I just put it in there because of the ticket - I can't imagine why I would need it myself. It was easy, just don't put the container parameter through the dollar function immediately, but try to iterate over it. If most of you feel like it do (that this would seldom be used) I can throw it out.

I'm more interested what you think about #6352. I'm thinking of extending my patch to include an option to generate smallest HTTP request possible (w/ stripped headers), suitable for polling. This includes stripping out things like Referer and User-agent. I don't feed comfortable doing it, but on the other hand, those are bigger then the rest of the request by themselves and doing that really could improve performance.

As for latency problems, I have a stupidly simple idea to fix it - sequentially number the requests as they are made. If a response comes in but another request to the same resource with bigger number already finished, discard the response. Of course, there would be more fine-grained complexities, but the idea is basically it.

This is an updated patch. There are no major changes, but now it's really ready for the core.

A majority of the changes doesn't have tests because they're impossible to test without a cross-browser mock object. I may have to write one for my next series of patches.

Summary

Fix for #4128 (multiple receivers) is no more included. This wasn't such a good idea because a large majority doesn't need this. Who does, write callbacks. Also, my fix for #5741 (bad active request count) was removed because it wasn't a good solution for detecting failed requests. It turned out it's more complicated. My next patch will deal with this.

Changes, fixes:

• HTTP method can be entered in both lowercase and uppercase, but when sending uppercase is used (can't test);
• fixed both String.toQueryParams and Hash.toQueryString when handling empty values (tests);
• added encoding option (for POST) with a default of "UTF-8" (can't test);
• now recognizes all JavaScript MIME types from a response (can't test);
• allows for PUT bodies (can't test);
• allows for HTTP authentication with new username and password options (can't test);
• query parameters can be entered as string (old) or as a hash (new), internally they are handled as hash (tested);
• fixes the "connection:close" fix for old Gecko versions (#5606), enables keep-alive now;
• request headers are handled internally as hash (not flat array), you can now pass custom headers as hash, too;
• renamed responseIsSuccess() to success and removed responseIsFailure() method that did nothing more than negate responseIsSuccess (and was never used);
• header() method is renamed to getHeader for clarity and consistency;
• onSuccess/onFailure are now being called after onComplete, following the principle of least surprise;
• random unconnected code tweaks (keeping functionality unchanged) and reordering of some Ajax.Request methods to achieve some sort of natural order while reading source.

Importance

This mega-patch is very important and public API is completely backwards-compatible. Please apply it and test for yourself on Windows and OS X. It should be applied for the 1.5 release if possible.

I decided not to fix #6352 (removeHeaders option) and #5592 (latency problems) for now because this needs much though and advanced testing. In my next mega-patch I will try to fix these with an addition of #5741 (request count), #4267 (server down) and #5393 (unload) - these all deal with Ajax, HTTP headers and transport.status. It is obvious that this will be hard to patch and test and that it's definitely not suited for a major release.

Comments welcome, -Mislav

Great work, Mislav. I encourage you to pursue micro- rather than mega-patches for future work since it makes incremental review and adoption much more efficient.

Thanks.

As for your comment about mega- vs. micro- ... you're completely right. But as I started patching this I just couldn't stop - one fix came naturally after the other. Soon it became obvious that if I split the patch, those overlapping would break others once they would be applied. Therefore I rolled this one as single.

After this is applied I will start on next series - all micro-patches, of course :)

It became obvious to me that a XMLHttpRequest mock object should be written to test all transport-specific stuff because unit tests are not always run with a Webrick server in the background. Alternatively, we could enhance the script that runs the server to emulate a more real-world server (with latency, errors and stuff), but then Prototype could only be unit-tested in a Ruby environment.

(In [5448]) prototype: A slew of Ajax improvements. Closes #6366.

mislav, thanks very much for the patch. I had to make a couple of changes:

The test for the Connection: close fix failed to account for the fact that String.prototype.match returns null when no match is found. It now reads (navigator.userAgent.match(/Gecko\/(\d{4})/) || [0,2005])[1] < 2005 .

The Content-type header test failed to account for "; charset=". That regex now reads /^(text|application)\/(x-)?(java|ecma)script(;.*)?$/i .

Also, it seems like you forgot to include test/unit/fixtures/content.html in your patch. Let me know if the one I committed isn't right.

Great work!

Thanks :)

Hm, I thought all Mozilla browsers would match "Gecko/xxxx". But I agree a sanity check is in order. Your fix for the regex is wise, too.

Check my watched tickets while you're preparing the release, there are a lot more micro-patches in there that are 1.5-ready and waiting.

Replying to mislav:

1. #4672, by allowing PUT requests to have bodies;

The description is misleading. PUT requests are still not allowed; they are still simulated via POST.

This change broke Safari because the username and password was always set on the request even if not specified, resulting in http://undefined:undefined@example.com/ when HTTP Basic auth is in use. I've submitted bug #6638 which includes a patch.

Part of the code committed in this ticket badly breaks Ajax.Request(). For example, you can no longer pass in the following parameter string:

foo1=bar1&foo2[]=bar2&foo2[]=bar3&foo2[]=bar4

This is a perfectly valid string under RFC. However, the new code attempts to convert this string from string to array and back to string, in the process mangling and losing parameters.

Since PHP uses the 'foo[]=' syntax to pass an array of data across page loads, this is a major issue.

Patch:

--- prototype.js        18 Nov 2006 04:01:12 -0000      1.3
+++ prototype.js        18 Nov 2006 06:47:42 -0000
@@ -714,8 +714,8 @@
     Object.extend(this.options, options || {});

     this.options.method = this.options.method.toLowerCase();
-    this.options.parameters = $H(typeof this.options.parameters == 'string' ?
-      this.options.parameters.toQueryParams() : this.options.parameters);
+    this.options.parameters = typeof this.options.parameters == 'string' ?
+      this.options.parameters : this.options.parameters.toQueryString();
   }
 }

@@ -732,21 +732,21 @@

   request: function(url) {
     var params = this.options.parameters;
-    if (params.any()) params['_'] = '';
+    if (params.length) params += '&_=';

     if (!['get', 'post'].include(this.options.method)) {
       // simulate other verbs over post
-      params['_method'] = this.options.method;
+      params += (params.length ? '&' : '') + '_method=' + this.options.method;
       this.options.method = 'post';
     }

     this.url = url;

     // when GET, append parameters to URL
-    if (this.options.method == 'get' && params.any())
-      this.url += (this.url.indexOf('?') >= 0 ? '&' : '?') +
-        params.toQueryString();
+    if (this.options.method == 'get' && params.length)
+      this.url += (this.url.indexOf('?') >= 0 ? '&' : '?') + params;

Let's move forward, not backwards. See #6645 - I would really appreciate if you posted failing tests as a patch there.