From a7ccb7531c92fb976c6058aef2bb18316075efb2 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. Closes #7031 --- src/ng/httpBackend.js | 65 +++++++++++++++++++++++--------------- test/ng/httpBackendSpec.js | 52 ++++++++---------------------- 2 files changed, 54 insertions(+), 63 deletions(-) diff --git a/src/ng/httpBackend.js b/src/ng/httpBackend.js index 63b07456..ba458595 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; - if (msie && msie <= 8) { - script.onreadystatechange = function() { - if (/loaded|complete/.test(script.readyState)) { - doneWrapper(); + 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 <= 8) { + script.onreadystatechange = function() { + 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 a549b6a8..46cefea7 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);