Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

fixes to $.ajaxJSONP #660

Closed
wants to merge 1 commit into from

2 participants

@zertosh

When making JSONP requests:

  1. settings.beforeSend wasn't being called.
  2. The ajaxBeforeSend and ajaxError events weren't being fired.
  3. There was inconsistent behavior with timeouts between $.ajax and $.ajaxJSONP. In $.ajax when there is a timeout, ajaxError is called and that subsequentely calls ajaxComplete. In $.ajaxJSONP, a timeout skips ajaxError and calls ajaxComplete twice. First with a status of abort and then again with a status of timeout.
  4. When the request is aborted either by (1) calling xhr.abort() or (2) because of a timeout, or (3) the request raises an onerror event, the global window[callbackName] is set to function empty () {} instead of deleting it. So the global scope stays polluted and empty() retains a reference to <script> which then can't be garbage collected.
  5. When a request is aborted before completion/timeout by calling xhr.abort() and there is a timeout set, the timeout would not to be cleared.
  6. If you didn't specify an error callback in the options of $.ajaxJSONP and there is an error then there is no cleaning up of <script> or window[callbackName], the error event isn't fired, and $.active isn't updated. (Kinda related to 4).

I only wrote tests for (1) and (6). Any help?

@mislav mislav referenced this pull request from a commit
@mislav mislav proper Ajax callbacks tests
Introduces several failing cases pointed out in #660
134a28c
@mislav
Collaborator

Hey @zertosh

Thanks! I have merged your fixes (and made an additional fix) in 4769efd...eb6065c

I haven't used your tests because beforehand I've written much more robust callback, abort, and timeout tests and ensured there are failing cases for each of your reported issues above, then applied your patch which fixed all but one of them. (I've changed manual abort() to call ajaxError instead of ajaxComplete.)

Behavior described under (4) is by design. If the request is manually aborted or there is a timeout, we want the callback to be an empty function instead of unset because SCRIPT will finish eventually and will try to call the callback. However, empty doesn't contain a reference to SCRIPT, so it's not a big deal. Yes, the global namespace gets polluted a bit.

@mislav mislav closed this
@lopper lopper referenced this pull request from a commit in buddydvd/zepto
@mislav mislav proper Ajax callbacks tests
Introduces several failing cases pointed out in #660
40ab9c5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 12, 2012
  1. @zertosh

    fixes to $.ajaxJSONP

    zertosh authored
This page is out of date. Refresh to see the latest.
Showing with 43 additions and 11 deletions.
  1. +19 −11 src/ajax.js
  2. +24 −0 test/ajax.html
View
30 src/ajax.js
@@ -75,32 +75,40 @@
var callbackName = 'jsonp' + (++jsonpID),
script = document.createElement('script'),
- abort = function(){
+ cleanup = function() {
+ clearTimeout(abortTimeout)
$(script).remove()
- if (callbackName in window) window[callbackName] = empty
+ delete window[callbackName]
+ },
+ abort = function(){
+ cleanup()
ajaxComplete('abort', xhr, options)
},
xhr = { abort: abort }, abortTimeout
- if (options.error) script.onerror = function() {
- xhr.abort()
- options.error()
+ serializeData(options)
+
+ if (ajaxBeforeSend(xhr, options) === false) {
+ ajaxError(null, 'abort', xhr, options)
+ return false
}
window[callbackName] = function(data){
- clearTimeout(abortTimeout)
- $(script).remove()
- delete window[callbackName]
+ cleanup()
ajaxSuccess(data, xhr, options)
}
- serializeData(options)
+ script.onerror = function() {
+ cleanup()
+ ajaxError(null, 'error', xhr, options)
+ }
+
script.src = options.url.replace(/=\?/, '=' + callbackName)
$('head').append(script)
if (options.timeout > 0) abortTimeout = setTimeout(function(){
- xhr.abort()
- ajaxComplete('timeout', xhr, options)
+ cleanup()
+ ajaxError(null, 'timeout', xhr, options)
}, options.timeout)
return xhr
View
24 test/ajax.html
@@ -134,6 +134,17 @@
})
},
+ testAjaxGetJSONPBeforeSendCallback: function(t){
+ t.pause()
+ var xhr = $.ajaxJSONP({
+ url: 'fixtures/jsonp.js?callback=?&timestamp='+(+new Date),
+ beforeSend: function() {
+ t.assert(true)
+ deferredResume(t)
+ }
+ })
+ },
+
testAjaxGetJSONPErrorCallback: function(t){
t.pause()
$.ajax({
@@ -146,6 +157,19 @@
})
},
+ testAjaxGetJSONPErrorWithoutErrorCallback: function(t){
+ t.pause()
+ $.ajax({
+ type: 'GET',
+ url: 'fixtures/404.js?timestamp='+(+new Date),
+ dataType: 'jsonp',
+ complete: function() {
+ t.assert(true)
+ deferredResume(t)
+ }
+ })
+ },
+
testJSONPdataType: function(t){
t.pause()
$.ajax({
Something went wrong with that request. Please try again.