refact($http): use the $jsonpCallbacks service

Use the built-in service to handling callbacks to `$http.jsonp` requests. Closes #3073 Closes #14795
angular · Jun 21, 2016 · fa3214c · fa3214c
1 parent a8cacfe
commit fa3214c
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 27 deletions.
diff --git 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
@@ -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
@@ -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,25 +330,27 @@ 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();
     });
 
 
     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);
 
 
       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);
     });