From fa3214cfb8b4687dc7439b2fddd6eebc4560fc5c Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 21 Jun 2016 19:05:34 +0100 Subject: [PATCH] refact($http): use the $jsonpCallbacks service Use the built-in service to handling callbacks to `$http.jsonp` requests. Closes #3073 Closes #14795 --- src/ng/http.js | 2 ++ src/ng/httpBackend.js | 29 +++++++++++------------- test/ng/httpBackendSpec.js | 46 +++++++++++++++++++++++++++++--------- 3 files changed, 50 insertions(+), 27 deletions(-) diff --git a/src/ng/http.js b/src/ng/http.js index e98186096ce2..1d0a16fa7e6d 100644 --- a/src/ng/http.js +++ b/src/ng/http.js @@ -1122,6 +1122,8 @@ function $HttpProvider() { * * @description * Shortcut method to perform `JSONP` request. + * If you would like to customise where and how the callbacks are stored then try overriding + * or decorating the {@link jsonpCallbacks} service. * * @param {string} url Relative or absolute URL specifying the destination of the request. * The name of the callback should be the string `JSON_CALLBACK`. diff --git a/src/ng/httpBackend.js b/src/ng/httpBackend.js index add2af6e7c0d..3ed7548a610d 100644 --- a/src/ng/httpBackend.js +++ b/src/ng/httpBackend.js @@ -32,7 +32,7 @@ function $xhrFactoryProvider() { /** * @ngdoc service * @name $httpBackend - * @requires $window + * @requires $jsonpCallbacks * @requires $document * @requires $xhrFactory * @@ -47,8 +47,8 @@ function $xhrFactoryProvider() { * $httpBackend} which can be trained with responses. */ function $HttpBackendProvider() { - this.$get = ['$browser', '$window', '$document', '$xhrFactory', function($browser, $window, $document, $xhrFactory) { - return createHttpBackend($browser, $xhrFactory, $browser.defer, $window.angular.callbacks, $document[0]); + this.$get = ['$browser', '$jsonpCallbacks', '$document', '$xhrFactory', function($browser, $jsonpCallbacks, $document, $xhrFactory) { + return createHttpBackend($browser, $xhrFactory, $browser.defer, $jsonpCallbacks, $document[0]); }]; } @@ -58,17 +58,13 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc $browser.$$incOutstandingRequestCount(); url = url || $browser.url(); - if (lowercase(method) == 'jsonp') { - 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), - callbackId, function(status, text) { - completeRequest(callback, status, callbacks[callbackId].data, "", text); - callbacks[callbackId] = noop; + if (lowercase(method) === 'jsonp') { + var callbackPath = callbacks.createCallback(url); + var jsonpDone = jsonpReq(url, callbackPath, function(status, text) { + // jsonpReq only ever sets status to 200 (OK), 404 (ERROR) or -1 (WAITING) + var response = (status === 200) && callbacks.getResponse(callbackPath); + completeRequest(callback, status, response, "", text); + callbacks.removeCallback(callbackPath); }); } else { @@ -170,7 +166,8 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc } }; - function jsonpReq(url, callbackId, done) { + function jsonpReq(url, callbackPath, done) { + url = url.replace('JSON_CALLBACK', callbackPath); // we can't use jQuery/jqLite here because jQuery does crazy stuff with script elements, e.g.: // - fetches local scripts via XHR and evals them // - adds and immediately removes script elements from the document @@ -188,7 +185,7 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc var text = "unknown"; if (event) { - if (event.type === "load" && !callbacks[callbackId].called) { + if (event.type === "load" && !callbacks.wasCalled(callbackPath)) { event = { type: "error" }; } text = event.type; diff --git a/test/ng/httpBackendSpec.js b/test/ng/httpBackendSpec.js index af3175acfd6d..bf27cf290a02 100644 --- a/test/ng/httpBackendSpec.js +++ b/test/ng/httpBackendSpec.js @@ -3,13 +3,13 @@ describe('$httpBackend', function() { - var $backend, $browser, callbacks, + var $backend, $browser, $jsonpCallbacks, xhr, fakeDocument, callback; - beforeEach(inject(function($injector) { - callbacks = {counter: 0}; + $browser = $injector.get('$browser'); + fakeDocument = { $$scripts: [], createElement: jasmine.createSpy('createElement').and.callFake(function() { @@ -28,7 +28,27 @@ describe('$httpBackend', function() { }) } }; - $backend = createHttpBackend($browser, createMockXhr, $browser.defer, callbacks, fakeDocument); + + $jsonpCallbacks = { + createCallback: function(url) { + $jsonpCallbacks[url] = function(data) { + $jsonpCallbacks[url].called = true; + $jsonpCallbacks[url].data = data; + }; + return url; + }, + wasCalled: function(callbackPath) { + return $jsonpCallbacks[callbackPath].called; + }, + getResponse: function(callbackPath) { + return $jsonpCallbacks[callbackPath].data; + }, + removeCallback: function(callbackPath) { + delete $jsonpCallbacks[callbackPath]; + } + }; + + $backend = createHttpBackend($browser, createMockXhr, $browser.defer, $jsonpCallbacks, fakeDocument); callback = jasmine.createSpy('done'); })); @@ -235,7 +255,7 @@ describe('$httpBackend', function() { it('should call $xhrFactory with method and url', function() { var mockXhrFactory = jasmine.createSpy('mockXhrFactory').and.callFake(createMockXhr); - $backend = createHttpBackend($browser, mockXhrFactory, $browser.defer, callbacks, fakeDocument); + $backend = createHttpBackend($browser, mockXhrFactory, $browser.defer, $jsonpCallbacks, fakeDocument); $backend('GET', '/some-url', 'some-data', noop); expect(mockXhrFactory).toHaveBeenCalledWith('GET', '/some-url'); }); @@ -294,7 +314,7 @@ describe('$httpBackend', function() { describe('JSONP', function() { - var SCRIPT_URL = /([^\?]*)\?cb=angular\.callbacks\.(.*)/; + var SCRIPT_URL = /([^\?]*)\?cb=(.*)/; it('should add script tag for JSONP request', function() { @@ -310,7 +330,7 @@ describe('$httpBackend', function() { url = script.src.match(SCRIPT_URL); expect(url[1]).toBe('http://example.org/path'); - callbacks[url[2]]('some-data'); + $jsonpCallbacks[url[2]]('some-data'); browserTrigger(script, "load"); expect(callback).toHaveBeenCalledOnce(); @@ -318,6 +338,8 @@ describe('$httpBackend', function() { it('should clean up the callback and remove the script', function() { + spyOn($jsonpCallbacks, 'removeCallback').and.callThrough(); + $backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback); expect(fakeDocument.$$scripts.length).toBe(1); @@ -325,10 +347,10 @@ describe('$httpBackend', function() { var script = fakeDocument.$$scripts.shift(), callbackId = script.src.match(SCRIPT_URL)[2]; - callbacks[callbackId]('some-data'); + $jsonpCallbacks[callbackId]('some-data'); browserTrigger(script, "load"); - expect(callbacks[callbackId]).toBe(angular.noop); + expect($jsonpCallbacks.removeCallback).toHaveBeenCalledOnceWith(callbackId); expect(fakeDocument.body.removeChild).toHaveBeenCalledOnceWith(script); }); @@ -343,7 +365,9 @@ describe('$httpBackend', function() { }); - it('should abort request on timeout and replace callback with noop', function() { + it('should abort request on timeout and remove JSONP callback', function() { + spyOn($jsonpCallbacks, 'removeCallback').and.callThrough(); + callback.and.callFake(function(status, response) { expect(status).toBe(-1); }); @@ -359,7 +383,7 @@ describe('$httpBackend', function() { expect(fakeDocument.$$scripts.length).toBe(0); expect(callback).toHaveBeenCalledOnce(); - expect(callbacks[callbackId]).toBe(angular.noop); + expect($jsonpCallbacks.removeCallback).toHaveBeenCalledOnceWith(callbackId); });