Skip to content

Commit a7ccb75

Browse files
caitpcaitp
committed
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 angular#7031
1 parent 6bea059 commit a7ccb75

2 files changed

Lines changed: 53 additions & 62 deletions

File tree

‎src/ng/httpBackend.js‎

Lines changed: 39 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -49,16 +49,13 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
4949
var callbackId = '_' + (callbacks.counter++).toString(36);
5050
callbacks[callbackId] = function(data) {
5151
callbacks[callbackId].data = data;
52+
callbacks[callbackId].called = true;
5253
};
5354

5455
var jsonpDone = jsonpReq(url.replace('JSON_CALLBACK', 'angular.callbacks.' + callbackId),
55-
function() {
56-
if (callbacks[callbackId].data) {
57-
completeRequest(callback, 200, callbacks[callbackId].data);
58-
} else {
59-
completeRequest(callback, status || -2);
60-
}
61-
callbacks[callbackId] = angular.noop;
56+
callbackId, function(status, text) {
57+
completeRequest(callback, status, callbacks[callbackId].data, "", text);
58+
callbacks[callbackId] = noop;
6259
});
6360
} else {
6461

@@ -160,33 +157,51 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
160157
}
161158
};
162159

163-
function jsonpReq(url, done) {
160+
function jsonpReq(url, callbackId, done) {
164161
// we can't use jQuery/jqLite here because jQuery does crazy shit with script elements, e.g.:
165162
// - fetches local scripts via XHR and evals them
166163
// - adds and immediately removes script elements from the document
167-
var script = rawDocument.createElement('script'),
168-
doneWrapper = function() {
169-
script.onreadystatechange = script.onload = script.onerror = null;
170-
rawDocument.body.removeChild(script);
171-
if (done) done();
172-
};
173-
174-
script.type = 'text/javascript';
164+
var script = rawDocument.createElement('script'), callback = null;
165+
script.type = "text/javascript";
175166
script.src = url;
167+
script.async = true;
168+
169+
callback = function(event) {
170+
removeEventListenerFn(script, "load", callback);
171+
removeEventListenerFn(script, "error", callback);
172+
rawDocument.body.removeChild(script);
173+
script = null;
174+
var status = -1;
175+
var text = "unknown";
176+
177+
if (event) {
178+
if (event.type === "load" && !callbacks[callbackId].called) {
179+
event = { type: "error" };
180+
}
181+
text = event.type;
182+
status = event.type === "error" ? 404 : 200;
183+
}
184+
185+
if (done) {
186+
done(status, text);
187+
}
188+
};
189+
190+
addEventListenerFn(script, "load", callback);
191+
addEventListenerFn(script, "error", callback);
176192

177-
if (msie && msie <= 8) {
193+
if (msie <= 8) {
178194
script.onreadystatechange = function() {
179-
if (/loaded|complete/.test(script.readyState)) {
180-
doneWrapper();
195+
if (isString(script.readyState) && /loaded|complete/.test(script.readyState)) {
196+
script.onreadystatechange = null;
197+
callback({
198+
type: 'load'
199+
});
181200
}
182201
};
183-
} else {
184-
script.onload = script.onerror = function() {
185-
doneWrapper();
186-
};
187202
}
188203

189204
rawDocument.body.appendChild(script);
190-
return doneWrapper;
205+
return callback;
191206
}
192207
}

‎test/ng/httpBackendSpec.js‎

Lines changed: 14 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,17 @@ describe('$httpBackend', function() {
3939
fakeDocument = {
4040
$$scripts: [],
4141
createElement: jasmine.createSpy('createElement').andCallFake(function() {
42-
return {};
42+
// msie8 depends on modifying readyState for testing. This property is readonly,
43+
// so it requires a fake object. For other browsers, we do need to make use of
44+
// event listener registration/deregistration, so these stubs are needed.
45+
if (msie <= 8) return {
46+
attachEvent: noop,
47+
detachEvent: noop,
48+
addEventListener: noop,
49+
removeEventListener: noop
50+
};
51+
// Return a proper script element...
52+
return document.createElement(arguments[0]);
4353
}),
4454
body: {
4555
appendChild: jasmine.createSpy('body.appendChild').andCallFake(function(script) {
@@ -356,7 +366,7 @@ describe('$httpBackend', function() {
356366
script.readyState = 'complete';
357367
script.onreadystatechange();
358368
} else {
359-
script.onload();
369+
browserTrigger(script, "load");
360370
}
361371

362372
expect(callback).toHaveBeenCalledOnce();
@@ -372,20 +382,19 @@ describe('$httpBackend', function() {
372382
callbackId = script.src.match(SCRIPT_URL)[2];
373383

374384
callbacks[callbackId]('some-data');
375-
376385
if (script.onreadystatechange) {
377386
script.readyState = 'complete';
378387
script.onreadystatechange();
379388
} else {
380-
script.onload();
389+
browserTrigger(script, "load");
381390
}
382391

383392
expect(callbacks[callbackId]).toBe(angular.noop);
384393
expect(fakeDocument.body.removeChild).toHaveBeenCalledOnceWith(script);
385394
});
386395

387396

388-
if(msie<=8) {
397+
if (msie <= 8) {
389398

390399
it('should attach onreadystatechange handler to the script object', function() {
391400
$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, noop);
@@ -400,42 +409,9 @@ describe('$httpBackend', function() {
400409
expect(script.onreadystatechange).toBe(null);
401410
});
402411

403-
} else {
404-
405-
it('should attach onload and onerror handlers to the script object', function() {
406-
$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, noop);
407-
408-
expect(fakeDocument.$$scripts[0].onload).toEqual(jasmine.any(Function));
409-
expect(fakeDocument.$$scripts[0].onerror).toEqual(jasmine.any(Function));
410-
411-
var script = fakeDocument.$$scripts[0];
412-
script.onload();
413-
414-
expect(script.onload).toBe(null);
415-
expect(script.onerror).toBe(null);
416-
});
417412

418413
}
419414

420-
it('should call callback with status -2 when script fails to load', function() {
421-
callback.andCallFake(function(status, response) {
422-
expect(status).toBe(-2);
423-
expect(response).toBeUndefined();
424-
});
425-
426-
$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback);
427-
expect(fakeDocument.$$scripts.length).toBe(1);
428-
429-
var script = fakeDocument.$$scripts.shift();
430-
if (script.onreadystatechange) {
431-
script.readyState = 'complete';
432-
script.onreadystatechange();
433-
} else {
434-
script.onload();
435-
}
436-
expect(callback).toHaveBeenCalledOnce();
437-
});
438-
439415

440416
it('should set url to current location if not specified or empty string', function() {
441417
$backend('JSONP', undefined, null, callback);

0 commit comments

Comments
 (0)