fix($httpBackend): don't error when JSONP callback called with no par…

…ameter This change brings Angular's JSONP behaviour closer in line with jQuery's. It will no longer treat a callback called with no data as an error, and will no longer support IE8 via the onreadystatechanged event. BREAKING CHANGE: Previously, the JSONP backend code would support IE8 by relying on the readystatechanged events. This is no longer the case, as these events do not provide adequate useful information for deeming whether or not a response is an error. Previously, a JSONP response which did not pass data into the callback would be given a status of -2, and treated as an error. Now, this situation will instead be given a status of 200, despite the lack of data. This is useful for interaction with certain APIs. Previously, the onload and onerror callbacks were added to the JSONP script tag. These have been replaced with jQuery events, in order to gain access to the event object. This means that it is now difficult to test if the callbacks are registered or not. This is possible with jQuery, using the $.data("events") method, however it is currently impossible with jqLite. This is not expected to break applications. Closes angular#4987
caitp · Mar 19, 2014 · f559143 · f559143
1 parent 748a6c8
commit f559143
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 95 deletions.
diff --git a/src/ng/httpBackend.js b/src/ng/httpBackend.js
@@ -49,16 +49,13 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
       var callbackId = '_' + (callbacks.counter++).toString(36);
       callbacks[callbackId] = function(data) {
         callbacks[callbackId].data = data;
+        callbacks[callbackId].called = true;
       };
 
       var jsonpDone = jsonpReq(url.replace('JSON_CALLBACK', 'angular.callbacks.' + callbackId),
-          function() {
-        if (callbacks[callbackId].data) {
-          completeRequest(callback, 200, callbacks[callbackId].data);
-        } else {
-          completeRequest(callback, status || -2);
-        }
-        callbacks[callbackId] = angular.noop;
+          callbackId, function(status, text) {
+        completeRequest(callback, status, callbacks[callbackId].data, "", text);
+        callbacks[callbackId] = noop;
       });
     } else {
 
@@ -158,33 +155,39 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
     }
   };
 
-  function jsonpReq(url, done) {
+  function jsonpReq(url, callbackId, done) {
     // we can't use jQuery/jqLite here because jQuery does crazy shit with script elements, e.g.:
     // - fetches local scripts via XHR and evals them
     // - adds and immediately removes script elements from the document
-    var script = rawDocument.createElement('script'),
-        doneWrapper = function() {
-          script.onreadystatechange = script.onload = script.onerror = null;
-          rawDocument.body.removeChild(script);
-          if (done) done();
-        };
-
-    script.type = 'text/javascript';
+    var script = rawDocument.createElement('script'), callback = null;
+    script.type = "text/javascript";
     script.src = url;
-
-    if (msie && msie <= 8) {
-      script.onreadystatechange = function() {
-        if (/loaded|complete/.test(script.readyState)) {
-          doneWrapper();
+    script.async = true;
+
+    callback = function(event) {
+      removeEventListenerFn(script, "load", callback);
+      removeEventListenerFn(script, "error", callback);
+      rawDocument.body.removeChild(script);
+      script = null;
+      var status = -1;
+      var text = "unknown";
+
+      if (event) {
+        if (event.type === "load" && !callbacks[callbackId].called) {
+          event = { type: "error" };
         }
-      };
-    } else {
-      script.onload = script.onerror = function() {
-        doneWrapper();
-      };
-    }
+        text = event.type;
+        status = event.type === "error" ? 404 : 200;
+      }
+
+      if (done) {
+        done(status, text);
+      }
+    };
 
+    addEventListenerFn(script, "load", callback);
+    addEventListenerFn(script, "error", callback);
     rawDocument.body.appendChild(script);
-    return doneWrapper;
+    return callback;
   }
 }
diff --git a/test/ng/httpBackendSpec.js b/test/ng/httpBackendSpec.js
@@ -39,7 +39,10 @@ describe('$httpBackend', function() {
     fakeDocument = {
       $$scripts: [],
       createElement: jasmine.createSpy('createElement').andCallFake(function() {
-        return {};
+        return {
+          addEventListener: function() {},
+          attachEvent: function() {}
+        };
       }),
       body: {
         appendChild: jasmine.createSpy('body.appendChild').andCallFake(function(script) {
@@ -325,13 +328,7 @@ describe('$httpBackend', function() {
 
       expect(url[1]).toBe('http://example.org/path');
       callbacks[url[2]]('some-data');
-
-      if (script.onreadystatechange) {
-        script.readyState = 'complete';
-        script.onreadystatechange();
-      } else {
-        script.onload();
-      }
+      browserTrigger(script, "load");
 
       expect(callback).toHaveBeenCalledOnce();
     });
@@ -346,71 +343,13 @@ describe('$httpBackend', function() {
           callbackId = script.src.match(SCRIPT_URL)[2];
 
       callbacks[callbackId]('some-data');
-
-      if (script.onreadystatechange) {
-        script.readyState = 'complete';
-        script.onreadystatechange();
-      } else {
-        script.onload();
-      }
+      browserTrigger(script, "load");
 
       expect(callbacks[callbackId]).toBe(angular.noop);
       expect(fakeDocument.body.removeChild).toHaveBeenCalledOnceWith(script);
     });
 
 
-    if(msie<=8) {
-
-      it('should attach onreadystatechange handler to the script object', function() {
-        $backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, noop);
-
-        expect(fakeDocument.$$scripts[0].onreadystatechange).toEqual(jasmine.any(Function));
-
-        var script = fakeDocument.$$scripts[0];
-
-        script.readyState = 'complete';
-        script.onreadystatechange();
-
-        expect(script.onreadystatechange).toBe(null);
-      });
-
-    } else {
-
-      it('should attach onload and onerror handlers to the script object', function() {
-        $backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, noop);
-
-        expect(fakeDocument.$$scripts[0].onload).toEqual(jasmine.any(Function));
-        expect(fakeDocument.$$scripts[0].onerror).toEqual(jasmine.any(Function));
-
-        var script = fakeDocument.$$scripts[0];
-        script.onload();
-
-        expect(script.onload).toBe(null);
-        expect(script.onerror).toBe(null);
-      });
-
-    }
-
-    it('should call callback with status -2 when script fails to load', function() {
-      callback.andCallFake(function(status, response) {
-        expect(status).toBe(-2);
-        expect(response).toBeUndefined();
-      });
-
-      $backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback);
-      expect(fakeDocument.$$scripts.length).toBe(1);
-
-      var script = fakeDocument.$$scripts.shift();
-      if (script.onreadystatechange) {
-        script.readyState = 'complete';
-        script.onreadystatechange();
-      } else {
-        script.onload();
-      }
-      expect(callback).toHaveBeenCalledOnce();
-    });
-
-
     it('should set url to current location if not specified or empty string', function() {
       $backend('JSONP', undefined, null, callback);
       expect(fakeDocument.$$scripts[0].src).toBe($browser.url());