search

mikehostetler / amplify

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

issue 44: jsonp requests can now be cached consistantly #86

Closed nicholascloud closed 11 years ago

nicholascloud commented 11 years ago

Amplify now ignores the callback parameter on jsonp URLs when building a cache-key. It also forces all jsonp requests to be cacheable (by adding cache:true to the ajax parameters).

dcneiner commented 11 years ago

@nicholascloud So sorry for the delay on this, I'll be looking at this tomorrow.

dcneiner commented 11 years ago

@nicholascloud, thanks again for your work on this. I finally got to review this, and have a few things I'd like if you could fix before we merge this request:

  1. There are some changes that are whitespace only, and they take the tab indents from our existing format and change them to 2-space indents. This makes it a little harder to see what was actually changed and what was just white space. If you could fix that (mainly its just lines 51 - 63 of src/request.js that are whitepsace only) and also make sure the new lines added to that file are tab indented to match the rest of that file.
  2. When I run the tests using a version less than jQuery 1.8, the tests fail. I ran these tests using http://localhost:1982/test/request/test.html?jquery=1.5.1. Where the value for jQuery can be any of the versions in external/jquery-X.X.X.js. Can you see if this is a test issue or an actual code failure in the older versions of jQuery?

Let me know if you have any questions. Thanks again!

nicholascloud commented 11 years ago

Hi @dcneiner, I've reformatted the files to be tab-4 instead of space-2, sorry about that.

Also, all versions of jquery are passing for me in Chrome. Are you using a specific browser? (See my /test-jquery-versions.js script to see how I "automated" this.)

EDIT: these also all pass in FireFox.

opening http://localhost:1982/test/index.html?jquery=1.4.1 opening http://localhost:1982/test/index.html?jquery=1.4.2 opening http://localhost:1982/test/index.html?jquery=1.4.3 opening http://localhost:1982/test/index.html?jquery=1.4.4 opening http://localhost:1982/test/index.html?jquery=1.4 opening http://localhost:1982/test/index.html?jquery=1.5.1 opening http://localhost:1982/test/index.html?jquery=1.5.2 opening http://localhost:1982/test/index.html?jquery=1.5 opening http://localhost:1982/test/index.html?jquery=1.6.1 opening http://localhost:1982/test/index.html?jquery=1.6.2 opening http://localhost:1982/test/index.html?jquery=1.6.3 opening http://localhost:1982/test/index.html?jquery=1.6.4 opening http://localhost:1982/test/index.html?jquery=1.6 opening http://localhost:1982/test/index.html?jquery=1.7.1 opening http://localhost:1982/test/index.html?jquery=1.7.2 opening http://localhost:1982/test/index.html?jquery=1.7 opening http://localhost:1982/test/index.html?jquery=1.8.0 opening http://localhost:1982/test/index.html?jquery=1.8.1 opening http://localhost:1982/test/index.html?jquery=1.8.2