Fixes #35 - JSONP client error in IE8

- adds additional unit tests for potential error conditions
- catches errors that may occur when attemping to cleanup the script node
- adds ability to hard code the callback name

.jshintignore

@@ -1,2 +1,3 @@
 node_modules
 parsers
+test/**/fixtures

README.md

@@ -229,7 +229,7 @@ Change Log
 ----------
 
 .next
-- nothing yet
+- fixes issues with uglified JSONP client in IE 8
 
 0.9.2
 - allow strings to represent request objects, the string value is treated as the path property

client/jsonp.js

@@ -8,6 +8,8 @@
 (function (define, global, document) {
 	'use strict';
 
+	var undef;
+
 	define(function (require) {
 
 		var when, UrlBuilder;
@@ -23,24 +25,28 @@
 			catch (e) {
 				// IE doesn't like to delete properties on the window object
 				if (propertyName in scope) {
-					scope[propertyName] = undefined;
+					scope[propertyName] = undef;
 				}
 			}
 		}
 
 		function cleanupScriptNode(response) {
-			if (response.raw && response.raw.parentNode) {
-				response.raw.parentNode.removeChild(response.raw);
+			try {
+				if (response.raw && response.raw.parentNode) {
+					response.raw.parentNode.removeChild(response.raw);
+				}
+			} catch (e) {
+				// ignore
 			}
 		}
 
-		function registerCallback(prefix, resolver, response) {
-			var name;
-
-			do {
-				name = prefix + Math.floor(new Date().getTime() * Math.random());
+		function registerCallback(prefix, resolver, response, name) {
+			if (!name) {
+				do {
+					name = prefix + Math.floor(new Date().getTime() * Math.random());
+				}
+				while (name in global);
 			}
-			while (name in global);
 
 			global[name] = function jsonpCallback(data) {
 				response.entity = data;
@@ -59,8 +65,14 @@
 		 *
 		 * @param {string} request.path the URL to load
 		 * @param {Object} [request.params] parameters to bind to the path
-		 * @param {string} [request.callback.param='callback'] the parameter name for which the callback function name is the value
-		 * @param {string} [request.callback.prefix='jsonp'] prefix for the callback function, as the callback is attached to the window object, a unique, unobtrusive prefix is desired
+		 * @param {string} [request.callback.param='callback'] the parameter name for
+		 *   which the callback function name is the value
+		 * @param {string} [request.callback.prefix='jsonp'] prefix for the callback
+		 *   function, as the callback is attached to the window object, a unique,
+		 *   unobtrusive prefix is desired
+		 * @param {string} [request.callback.name=<generated>] pins the name of the
+		 *   callback function, useful for cases where the server doesn't allow
+		 *   custom callback names. Generally not recomended.
 		 *
 		 * @returns {Promise<Response>}
 		 */
@@ -77,7 +89,7 @@
 
 			d = when.defer();
 			request.callback = request.callback || {};
-			callbackName = registerCallback(request.callback.prefix || 'jsonp', d.resolver, response);
+			callbackName = registerCallback(request.callback.prefix || 'jsonp', d.resolver, response, request.callback.name);
 			callbackParams = {};
 			callbackParams[request.callback.param || 'callback'] = callbackName;
 
@@ -93,18 +105,22 @@
 			script.async = true;
 			script.src = new UrlBuilder(request.path, request.params).build(callbackParams);
 
-			script.onerror = function () {
-				if (global[callbackName]) {
+			function handlePossibleError() {
+				if (typeof global[callbackName] === 'function') {
 					response.error = 'loaderror';
 					clearProperty(global, callbackName);
 					cleanupScriptNode(response);
 					d.reject(response);
 				}
+			}
+			script.onerror = function () {
+				handlePossibleError();
 			};
 			script.onload = script.onreadystatechange = function (e) {
 				// script tag load callbacks are completely non-standard
+				// handle case where onreadystatechange is fired for an error instead of onerror
 				if ((e && (e.type === 'load' || e.type === 'error')) || script.readyState === 'loaded') {
-					script.onerror(e);
+					handlePossibleError();
 				}
 			};
 

test/buster.js

@@ -35,6 +35,7 @@ config['rest:browser'] = {
 		'node_modules/poly/**/*.js',
 		'node_modules/when/**/*.js',
 		'node_modules/wire/**/*.js',
+		'test/**/fixtures/**',
 		{ path: '/wait', backend: 'http://example.com' }
 	],
 	libs: [

test/client/fixtures/data.js

@@ -0,0 +1 @@
+callback({ data: true });

test/client/fixtures/throw.js

@@ -0,0 +1 @@
+notcallback({ data: true });

test/client/jsonp-test-browser.js

@@ -24,7 +24,7 @@
 		rest = require('rest');
 
 		buster.testCase('rest/client/jsonp', {
-			'should make a GET by default': function () {
+			'should make a cross origin request': function () {
 				var request = { path: 'http://ajax.googleapis.com/ajax/services/search/web?v=1.0', params: { q: 'javascript' } };
 				return client(request).then(function (response) {
 					assert(response.entity.responseData);
@@ -34,17 +34,17 @@
 				}).otherwise(fail);
 			},
 			'should use the jsonp client from the jsonp interceptor by default': function () {
-				var request = { path: 'http://ajax.googleapis.com/ajax/services/search/web?v=1.0', params: { q: 'html5' } };
+				var request = { path: '/test/client/fixtures/data.js', callback: { name: 'callback' } };
 				return jsonpInterceptor()(request).then(function (response) {
-					assert(response.entity.responseData);
+					assert(response.entity.data);
 					assert.same(request, response.request);
 					refute(request.canceled);
 					refute(response.raw.parentNode);
 				}).otherwise(fail);
 			},
 			'should abort the request if canceled': function () {
 				var request, response;
-				request = { path: 'http://ajax.googleapis.com/ajax/services/search/web?v=1.0', params: { q: 'html5' } };
+				request = { path: '/test/client/fixtures/data.js', callback: { name: 'callback' } };
 				response = client(request).then(
 					fail,
 					failOnThrow(function (response) {
@@ -77,6 +77,24 @@
 					})
 				);
 			},
+			'should error if callback not invoked': function () {
+				var request = { path: '/test/client/fixtures/noop.js' };
+				return client(request).then(
+					fail,
+					failOnThrow(function (response) {
+						assert.same('loaderror', response.error);
+					})
+				);
+			},
+			'should error if script throws': function () {
+				var request = { path: '/test/client/fixtures/throw.js' };
+				return client(request).then(
+					fail,
+					failOnThrow(function (response) {
+						assert.same('loaderror', response.error);
+					})
+				);
+			},
 			'should normalize a string to a request object': function () {
 				var request = 'http://ajax.googleapis.com/ajax/services/search/web?v=1.0&q=javascript';
 				return client(request).then(function (response) {