From d8f2a9d4309afcdfec01e0d75bf6e2ba5d250925 Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Tue, 18 Mar 2014 11:11:39 -0400 Subject: [PATCH] fix($httpBackend): don't error when JSONP callback called with no parameter This change brings Angular's JSONP behaviour closer in line with jQuery's. The feature has already landed in the 1.3 branch as 6680b7b, however this alternative version is intended to implement the feature in an IE8-compatible fashion. --- src/ng/httpBackend.js | 63 +++++++++++++++++++++++--------------- test/ng/httpBackendSpec.js | 52 +++++++++---------------------- 2 files changed, 53 insertions(+), 62 deletions(-) diff --git a/src/ng/httpBackend.js b/src/ng/httpBackend.js index 63b0745658da..ba4585959e7f 100644 --- 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 { @@ -160,33 +157,51 @@ 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; + 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" }; + } + text = event.type; + status = event.type === "error" ? 404 : 200; + } + + if (done) { + done(status, text); + } + }; + + addEventListenerFn(script, "load", callback); + addEventListenerFn(script, "error", callback); - if (msie && msie <= 8) { + if (msie <= 8) { script.onreadystatechange = function() { - if (/loaded|complete/.test(script.readyState)) { - doneWrapper(); + if (isString(script.readyState) && /loaded|complete/.test(script.readyState)) { + script.onreadystatechange = null; + callback({ + type: 'load' + }); } }; - } else { - script.onload = script.onerror = function() { - doneWrapper(); - }; } rawDocument.body.appendChild(script); - return doneWrapper; + return callback; } } diff --git a/test/ng/httpBackendSpec.js b/test/ng/httpBackendSpec.js index a549b6a894a3..46cefea719fa 100644 --- a/test/ng/httpBackendSpec.js +++ b/test/ng/httpBackendSpec.js @@ -39,7 +39,17 @@ describe('$httpBackend', function() { fakeDocument = { $$scripts: [], createElement: jasmine.createSpy('createElement').andCallFake(function() { - return {}; + // msie8 depends on modifying readyState for testing. This property is readonly, + // so it requires a fake object. For other browsers, we do need to make use of + // event listener registration/deregistration, so these stubs are needed. + if (msie <= 8) return { + attachEvent: noop, + detachEvent: noop, + addEventListener: noop, + removeEventListener: noop + }; + // Return a proper script element... + return document.createElement(arguments[0]); }), body: { appendChild: jasmine.createSpy('body.appendChild').andCallFake(function(script) { @@ -356,7 +366,7 @@ describe('$httpBackend', function() { script.readyState = 'complete'; script.onreadystatechange(); } else { - script.onload(); + browserTrigger(script, "load"); } expect(callback).toHaveBeenCalledOnce(); @@ -372,12 +382,11 @@ 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); @@ -385,7 +394,7 @@ describe('$httpBackend', function() { }); - if(msie<=8) { + if (msie <= 8) { it('should attach onreadystatechange handler to the script object', function() { $backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, noop); @@ -400,42 +409,9 @@ describe('$httpBackend', function() { 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);