Bug Tracker

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#3087 closed enhancement (fixed)

Request: minor refactor of "ajax" function to have a separate 'factory' method for creating XHR instances

Reported by: shadedecho Owned by: flesler
Priority: minor Milestone: 1.3
Component: ajax Version: 1.2.6
Keywords: ajax Cc:
Blocked by: Blocking:

Description

The "ajax" function is kind of monolithic in that there are some pieces of its functionality that would prvoide some very helpful extensibility if they were separated out into other functions.

Namely, the creation of the XHR instance is embedded deep in this function, which makes overriding of that behavior (for unit testing, extensions with other XHR compatible objects, etc) nearly impossible without re-implementing the rest of the "ajax" function's logic.

Other frameworks (Dojo, YUI, Prototype, and ExtJS, for instance) all have "factory" functions which their various ajax mechanisms use to create the XHR instance to use for the request. Something very similar would be helpful with jQuery.

For instance, it could be something like:

function XHRTransport() {

return new XMLHttpRequest();

}

Obviously, that's too simple, because you'd want the already present logic of instantiating the ActiveX object for IE browsers, etc. But you get the idea, a function that returns the XHR instance and then can be used inline inside of the ajax function, without any further changes, just replacing that block of code with a call to this function.

This was discussed on the dev list and the suggestion was made to put in a ticket here, possibly for a patch or for consideration in a subsequent release.

Attachments (2)

ajax_transport.diff (1.0 KB) - added by choan 11 years ago.
Makes the Ajax transport replaceable
custom-xhr.patch (1.3 KB) - added by flesler 11 years ago.
New proposition

Download all attachments as: .zip

Change History (21)

comment:1 Changed 11 years ago by flesler

Component: coreajax
need: ReviewTest Case

Hi shadedecho, thanks for adding the ticket.

Note that you're not saying why do you think this is important, argumenting that "other frameworks do" is not enough to justify a modification.

Please add a test case where you really need to override this behavior. Also, if you have a patch in mind, feel free to attach it.

Changed 11 years ago by choan

Attachment: ajax_transport.diff added

Makes the Ajax transport replaceable

comment:2 Changed 11 years ago by choan

As I offered to provide a patch, here it goes (all test passed).

For the "why", I think this change would be useful in mocking the Ajax transport for unit testing.

comment:3 Changed 11 years ago by shadedecho

(for unit testing, extensions with other XHR compatible objects, etc)

That was my attempt at providing a brief "why". Choan has suggested the "unit testing" argument, which I just echo'd. Let me explain the other one in more detail:

http://flxhr.flensed.com/

flXHR is designed to be a drop-in replacement for the browser's native XHR object. That is, it is API-compatible with the native XHR, so it can be dropped-in to existing code which knows how to speak with and deal with that interface, and it should work seamlessly just like XHR. However, it provides a number of very helpful extensions, including (because it's based on Flash) direct, cross-domain communication (with server policy authorization), more robust error callback handling, built-in timeouts, and convenience methods for configuration.

If you look at Demos 7-10 on this page: http://flxhr.flensed.com/demo.php

You'll see that I provide proof-of-concept simple demos where I adapt the other major JS frameworks (Dojo, ExtJS, YUI, and Prototype) to swap in the flXHR for the native XHR, and the rest of the framework's mechanics work properly against it.

Basically, my hope is that this can become a viable alternative to the other cross-domain "workarounds" like IFRAMEs and dynamic SCRIPT tags, because I think it's more efficient and I think it's more secure. In that way, perhaps flXHR could eventually become an optional or default "plugin" of sorts for jQuery (and other frameworks) which removes the need to rely on the other wonky workarounds and allows more direct code because you only ever have to speak "XHR-API". :)

You can see also on that Demo page that I have a placeholder at Demo 11 for if I can find a way to swap in flXHR into jQuery similarly to how I adapt the others.

Personally, I *strongly* prefer using jQuery over any other framework, so it's been quite a burr in my side while developing flXHR that I've been unable to adapt jQuery to use flXHR, while I can adapt the other competitors just fine.

Of course, you can consider that either a flaw or a feature, but I just believe flexibility is better for all involved.

comment:4 Changed 11 years ago by choan

shadedecho, does my path provide enough hooks to replace the XHR transport with your fiXHR?

comment:5 Changed 11 years ago by shadedecho

Yes, I was able to use your patch to be able to make a patched version of 1.2.6, which I updated the demos on my page with, and have as available for download for anyone trying to use flXHR with jQuery.

Any idea on if/when this patch might make it into an official jQuery release?

comment:6 Changed 11 years ago by flesler

Owner: set to flesler
Status: newassigned

I'm really not sure, I'll ask to other developers in the team. It isn't a big change (that's a good thing), but doesn't seem like something that many people would need (bad thing).

Creating a different XHR that respects all the API seems very specific to me. Let me ask around.

comment:7 Changed 11 years ago by joern

Resolution: wontfix
Status: assignedclosed

Unit-testing works well without this modification. Extracting the factory method for just one user isn't enough.

comment:8 Changed 11 years ago by shadedecho

I think that's a really poor decision. It isn't just for "one user"... I've got well over a thousand people using my library now, more everyday, and many of them have emailed asking for better support with jQuery. I had hoped there was light at the end of the tunnel given the patch provided in this ticket. Now I have to tell them that jQuery devs decided flXHR was irrelevant. That's fine, it's your right to do so.

I would have thought such a small change (like 5 or 6 lines total) with the potential to benefit all jQuery users and other libraries would not have received such a casual dismissal.

I think it's a shame. It makes me feel like my choice of supporting jQuery the last several years in all my sites, in deference to other frameworks, was perhaps a bad choice, given that the other ones play nicely with others and cleary jQuery does not.

Sorry to have wasted your time. I'll move on to some other framework which is more friendly.

comment:9 in reply to:  8 Changed 11 years ago by john

Replying to shadedecho:

I responded on your blog, but I'll post it here, as well, for posterity:

Why not just replace the XMLHttpRequest object with your custom one? You wouldn’t have to modify any libraries and the user would get the full benefit.

I’ve created fake XMLHttpRequest objects in the past - simply saving a reference to the old object and adding in my new methods (to support cross-domain requests). It certainly sounds like something similar can be done here.

“But on principle alone, I think their poor social skills are something that should at least be called out and kept to task.”

I think that’s just being mean. We discussed this extensively on the mailing list - with you - and the only possible use case we could come up with was possibly doing mock unit testing (external to your library). But once it was determined that not even unit testing was a valid use case there really wasn’t much left.

A point to consider is that for every single change we make to the API (and every new API point we add) we have to document and support it indefinitely - if we were to ever change it or remove it we would have to provide alternatives. We never take API changes lightly - which is why we always look for multiple use cases whenever we add a new one. At this point, this change does not warrant it. Especially since when alternatives exist.

Sorry to have wasted your time. I'll move on to some other framework which is more friendly.

I'm confused - you didn't ask a single other framework to add anything - how are we any more or less friendly than them? We took the time to talk with you extensively on the mailing list, and in here, and considered your patch and recommendation. Considering that a viable alternative exists and that no other use cases exist it's increasingly hard to consider this API change.

We are absolutely still open to reconsidering - we're always open and love to be proven wrong. I hope you'll reconsider and keep the discussion going with us.

comment:10 Changed 11 years ago by shadedecho

I also commented in response on the blog, apologizing for unintentionally coming across as mean with some poor choice of words on my part. For posterity sake, this blog thread is located here:

http://www.flensed.com/fresh/?p=10


I addressed several of your comments on my blog response, so I won't duplicate them here as well. But to address a few here that were not addressed over there:

"...and considered your patch and recommendation..."

I didn't make the patch. It came from another user on your dev list, I assume someone who's familiar with your dev processes, I don't really know. They obviously felt (at least at first) there was some merit to my request. I was thrilled to see that someone "on the inside" (at least from my perspective) understood where I was coming from.

I would not have been so presumptuous as to tell jQuery how to change their library, I was just asking if there was a simple way to make the XHR instantiation part alterable. I was glad to see that according to the patch, it seemed like it was a simple change, and hoped that this would be enough to prevent it from having too much push back.

I certainly was not (and am not) trying to start a salvo over the matter.

"I'm confused - you didn't ask a single other framework to add anything - how are we any more or less friendly than them?"

I have corresponded with other frameworks before in the past, on other topics, and even when questions or suggestions to them were not accepted, I didn't come away from the experience feeling like I'd been dismissed simply because I was "just one person" asking.

Moreover, I didn't have to ask this request of the other frameworks, because I assume somewhere along the way, someone already had similar thoughts of flexibility, extensibility, cooperation, interoperation, etc, and they chose to create an XHR factory method already. Because jQuery is still my favorite library, I definitely was interested in pursuing if there was a nicer, cleaner way of adapting my project into jQuery, not really just for my benefit, but so that flXHR and jQuery could co-exist out in the world and not have to be mutually exclusive for anyone who chooses to use one or both.


My biggest reason for the request was not just for flXHR, but more in general to fight for a landscape in open source software that allows the best interoperability. Moreover, on the specific topic of XHR, and how it's changing in the new generation of browsers, I'm attempting to make the point (and prove it) that the best/better way is to consolidate around a common, well-known API (native XHR), and allow drop-in replacements (hot-swappable, essentially) to deal with the various issues that arise, such as cross-domain security. But that effort will only ever be as good as the cooperation level I can achieve with all the other major players who are already in the game.

For the other frameworks, this question was much simpler. For jQuery, I had hoped it would not be a difficult battle.

Again, I apologize for some of my reactionary wording, it was not meant to be insulting or mean, but rather to draw a clear distinction about the opinions I had, and the impression I developed from my brief foray into the jQuery dev world.

comment:11 Changed 11 years ago by shadedecho

As stated on my blog, in response to John's request, I will re-join the effort on the jquery-dev list to explore ways to make $.ajax more flexible in a broader way that works for all involved. I believe that will not only benefit flXHR, but it'll benefit lots of other projects and the open-source community as a whole.

comment:12 Changed 11 years ago by joern

Resolution: wontfix
Status: closedreopened

#3213 could be a usecase for this refactoring (the patch proposed there isn't appropiate).

comment:13 Changed 11 years ago by cmcnulty

I completely agree with Joern. After I submitted #3213, I did a bit more research and found this ticket, which I agree is a better solution. I also found #2167 and #2128 which I think are additional use cases for this ticket.

I'm glad to see this has been updated from willnotfix!

-Charles

comment:14 Changed 11 years ago by flesler

need: Test CaseCommit
Resolution: fixed
Status: reopenedclosed

Applied at [5805].

comment:15 Changed 11 years ago by shadedecho

I'm really pleased to see this improvement patch. Thanks again, Ariel!

Changed 11 years ago by flesler

Attachment: custom-xhr.patch added

New proposition

comment:16 Changed 11 years ago by joern

Resolution: fixed
Status: closedreopened

The patch looks good. Here is the option description for the wiki:

{{APIOption|xhr|Function|Callback for creating the XMLHttpRequest object. Defaults to the ActiveXObject when available (IE), the XMLHttpRequest otherwise. Override to provide your own implementation for XMLHttpRequest or enhancements to the factory.<nowiki></nowiki>}}

comment:17 Changed 11 years ago by flesler

Applied at [5807].

comment:18 Changed 11 years ago by flesler

Resolution: fixed
Status: reopenedclosed

comment:19 Changed 11 years ago by flesler

Here's the manager plugin: http://plugins.jquery.com/project/XHR

Note: See TracTickets for help on using tickets.