mikehostetler / amplify

AmplifyJS
http://amplifyjs.com
GNU General Public License v2.0
1.45k stars 143 forks source link

Caching broken for JSONP requests in jQuery #44

Closed zachleat closed 10 years ago

zachleat commented 12 years ago

Because jQuery uses its own expando property to establish a unique global for the JSONP callback, amplify's caching mechanism is broken. It works again if you override using jQuery's jsonpCallback property. Perhaps a warning or amplify should explicitly set its own jsonpCallback?

jdsharp commented 12 years ago

Hi Zach,

Can you submit a pull request with a failing test for this?

zachleat commented 12 years ago

I didn't have time to do this, but here's the workaround I used:

amplify.request.cache.jsonpToLocalStorage = function(resource, settings, ajaxSettings, xhr)
{
    var ajaxSettingsCopy = $.extend({}, ajaxSettings);
    ajaxSettingsCopy.url = ajaxSettingsCopy.url.replace(/\&?callback=[^&]*\&?/gi, '');

    return amplify.request.cache.localStorage(resource, settings, ajaxSettingsCopy, xhr);
};

and then use the jsonpToLocalStorage type instead of localStorage.

genereddick commented 12 years ago

I'm having the same issue (I think). In making a jsonp request with ?callback=? a unique callback id is generated by jQuery, which creates a unique cache name property ("myurl.com?callback=111111") based on the entire url including the callback id.

On a follow on request a new unique callback id is generated, thus a new cache name (myurl.com?callback=222222") and thus the earlier cache is ignored and a new http request is made instead.

elijahmanor commented 11 years ago

This is still and issue. I am about to record about this feature for my 1st Pluralsight course due on Saturday. Anyone have a pull request for it?

I'm a little swamped at the moment. I did do a PR for mockjax yesterday, but don't think I'll get around to a PR on this one.

dcneiner commented 11 years ago

I was looking into it this morning and since jQuery supports a number of options, the fix will have to account for the following:

  1. Pay attention to the dataType, and only attempt this fix if its set to jsonp
  2. Correctly look to the jsonp option to see if we should look for callback= or something else.
  3. Look to jsonpCallback to see the name of callback to remove from the url
  4. Make sure cache is set to true on AJAX Settings, else a _=TIMESTAMP will also be added to the URL

Number 4 should already be handled due to the way amplify will just pass on the cache setting to $.ajax and it will not be false in the case of someone wanting to cache a JSONP request.

rniemeyer commented 11 years ago

Was looking into this one for a few minutes and found one interesting thing to note: jQuery 1.8+ tries to safely reuse the jsonp callback names whenever possible. So, in some simple examples to reproduce this issue, the caching worked properly as the URL does not change (it does each time prior to 1.8).

nicholascloud commented 11 years ago

I have created a pull request to fix this issue.