Updated remoting.xhr API to use promises. Removed access to the native

remoting/webapp/crd/js/xhr.js

Index: remoting/webapp/crd/js/xhr.js
diff --git a/remoting/webapp/crd/js/xhr.js b/remoting/webapp/crd/js/xhr.js
index 8bf20878cdbe2535300b02035f604c49529af26b..561631fba9bf58e4818fc117ed92e9037b58c8be 100644
--- a/remoting/webapp/crd/js/xhr.js
+++ b/remoting/webapp/crd/js/xhr.js
@@ -12,26 +12,83 @@
 /** @suppress {duplicate} */
 var remoting = remoting || {};
 
-/** Namespace for XHR functions */
-/** @type {Object} */
-remoting.xhr = remoting.xhr || {};
-
 /**
- * Takes an associative array of parameters and urlencodes it.
+ * Executes an arbitrary HTTP method asynchronously.
  *
- * @param {Object<string,string>} paramHash The parameter key/value pairs.
- * @return {string} URLEncoded version of paramHash.
+ * @constructor
+ * @param {remoting.XhrParams} params
  */
-remoting.xhr.urlencodeParamHash = function(paramHash) {
-  var paramArray = [];
-  for (var key in paramHash) {
-    paramArray.push(encodeURIComponent(key) +
-                     '=' + encodeURIComponent(paramHash[key]));
+remoting.Xhr = function(params) {
+  // Extract fields that can be used more or less as-is.
+  var method = params.method;
+  var url = params.url;
+  var headers = remoting.Xhr.removeNullFields_(params.headers);
+  var withCredentials = params.withCredentials || false;
+  var onDone = params.onDone;
+
+  // Validate the response type and save it for later.
+  base.debug.assert(
+      params.responseType === undefined ||
+        params.responseType == 'text' ||
+        params.responseType == 'json' ||
+        params.responseType == 'none');

[Jamie, 2015/03/12 20:15:43]

Nit: Indentation.

[John Williams, 2015/03/14 03:35:33]

Done.

+  /** @const {string} */
+  this.responseType_ = params.responseType || 'text';
+
+  // Apply URL parameters.
+  var parameterString = '';
+  if (typeof(params.urlParams) === 'string') {
+    parameterString = params.urlParams;
+  } else if (typeof(params.urlParams) === 'object') {
+    parameterString = remoting.Xhr.urlencodeParamHash(
+        remoting.Xhr.removeNullFields_(params.urlParams));
   }
-  if (paramArray.length > 0) {
-    return paramArray.join('&');
+  if (parameterString) {
+    base.debug.assert(url.indexOf('?') == -1);
+    url += '?' + parameterString;
   }
-  return '';
+
+  // Check that the content spec is consistent.
+  if ((Number(params.textContent !== undefined) +
+       Number(params.formContent !== undefined) +
+       Number(params.jsonContent !== undefined)) > 1) {
+    throw new Error(
+        'may only specify one of textContent, formContent, and jsonContent');
+  }
+
+  // Convert the content fields to a single text content variable.
+  /** @type {?string} */
+  var content = null;
+  if (params.textContent !== undefined) {
+    content = params.textContent;
+  } else if (params.formContent !== undefined) {
+    if (!('Content-type' in headers)) {
+      headers['Content-type'] = 'application/x-www-form-urlencoded';
+    }
+    content = remoting.Xhr.urlencodeParamHash(params.formContent);
+  } else if (params.jsonContent !== undefined) {
+    if (!('Content-type' in headers)) {
+      headers['Content-type'] = 'application/json; charset=UTF-8';
+    }
+    content = JSON.stringify(params.jsonContent);
+  }
+
+  // Apply the oauthToken field.
+  if (params.oauthToken !== undefined) {
+    base.debug.assert(!('Authorization' in headers));
+    headers['Authorization'] = 'Bearer ' + params.oauthToken;
+  }
+
+  /** @const {!base.Deferred<remoting.Xhr.Response>} */
+  this.deferred_ = new base.Deferred();
+  if (onDone) {
+    this.then(onDone);
+  }
+
+  /** @const {!XMLHttpRequest} */
+  this.nativeXhr_ = new XMLHttpRequest();
+
+  this.startInternal_(method, url, content, headers, withCredentials);

[Jamie, 2015/03/12 20:15:43]

I would prefer that the ctor doesn't implicitly send the XHR. An explicit
start() method that returns a Promise seems like a better design.

I think it's also clearer because as you have it right now, you have to proxy
the then() method, and the call-site reads as <noun>.then() instead of the more
conventional <noun>.<verb>().then().

[John Williams, 2015/03/14 03:35:33]

Done.

 };
 
 /**
@@ -41,8 +98,6 @@ remoting.xhr.urlencodeParamHash = function(paramHash) {
  *
  * url: The URL to request.
  *
- * onDone: Function to call when the XHR finishes.
-
  * urlParams: (optional) Parameters to be appended to the URL.
  *     Null-valued parameters are omitted.
  *
@@ -61,106 +116,90 @@ remoting.xhr.urlencodeParamHash = function(paramHash) {
  *
  * withCredentials: (optional) Value of the XHR's withCredentials field.
  *
+ * useOauth: (optional) If true, use OAuth2 to authenticate the request.
+ *     Not compatible with the oauthToken field.
+ *
  * oauthToken: (optional) An OAuth2 token used to construct an
- *     Authentication header.
+ *     Authentication header.  Not compatible with the useOauth field.
+ *
+ * responseType: (optional) Request a response of a specific type,
+ *    either 'text', 'json', or 'none' to ignore the response body
+ *    entirely.  Default: 'text'.

[Jamie, 2015/03/12 20:15:43]

Maybe make this an enumerated type so that the test in the ctor can be skipped
in favour of compiler-enforced checking?

[John Williams, 2015/03/14 03:35:33]

Done, but I'm not sure I like the result.  Enumerated types are just so wordy in
Closure code.

  *
  * @typedef {{
  *   method: string,
  *   url:string,
- *   onDone:(function(XMLHttpRequest):void),
  *   urlParams:(string|Object<string,?string>|undefined),
  *   textContent:(string|undefined),
  *   formContent:(Object|undefined),
  *   jsonContent:(*|undefined),
  *   headers:(Object<string,?string>|undefined),
  *   withCredentials:(boolean|undefined),
- *   oauthToken:(string|undefined)
+ *   useOauth:(boolean|undefined),
+ *   oauthToken:(string|undefined),
+ *   responseType:(string|undefined),
+ *   onDone:((function(remoting.Xhr.Response):void)|undefined)
  * }}
  */
 remoting.XhrParams;
 
 /**
- * Returns a copy of the input object with all null or undefined
- * fields removed.
+ * Values produced by a request.
  *
- * @param {Object<string,?string>|undefined} input
- * @return {!Object<string,string>}
- * @private
+ * status: The HTTP status code.
+ *
+ * statusText: The HTTP status description.
+ *
+ * responseUrl: The response URL, if any.
+ *
+ * responseText: The content of the response as a string.  Valid only
+ *     if responseType is set to 'text' or the request failed.
+ *
+ * responseJson: The content of the response as a parsed JSON value.
+ *     Valid only if the request succeeded and the responseType is set
+ *     to 'json'.
+ *
+ * @typedef {{
+ *   status:number,
+ *   statusText:string,
+ *   responseUrl:string,
+ *   responseText:string,
+ *   responseJson:*
+ * }}
  */
-remoting.xhr.removeNullFields_ = function(input) {
-  /** @type {!Object<string,string>} */
-  var result = {};
-  if (input) {
-    for (var field in input) {
-      var value = input[field];
-      if (value != null) {
-        result[field] = value;
-      }
-    }
-  }
-  return result;
+remoting.Xhr.Response;
+
+/**
+ * @param {function(remoting.Xhr.Response):void} onDone
+ * @return {void}
+ */
+remoting.Xhr.prototype.then = function(onDone) {
+  this.promise().then(onDone);
 };
 
 /**
- * Executes an arbitrary HTTP method asynchronously.
+ * Gets a promise that is resolved when the request completes.
  *
- * @param {remoting.XhrParams} params
- * @return {XMLHttpRequest} The XMLHttpRequest object.
+ * If an HTTP status code outside the 200..299 range is returned, the
+ * promise is rejected with a remoting.Error by default.  Setting
+ * resolveOnErrorStatus to true in the constructor parameters allows
+ * the promise to resolve for any HTTP status code.

[Jamie, 2015/03/12 20:15:43]

There's no other mention of resolveOnErrorStatus. Is this a stale comment?

[John Williams, 2015/03/14 03:35:33]

Fixed.

+ *
+ * Any error that prevents the HTTP request from succeeding causes
+ * this promise to be rejected.
+ *
+ * @return {!Promise<!remoting.Xhr.Response>}
  */
-remoting.xhr.start = function(params) {
-  // Extract fields that can be used more or less as-is.
-  var method = params.method;
-  var url = params.url;
-  var onDone = params.onDone;
-  var headers = remoting.xhr.removeNullFields_(params.headers);
-  var withCredentials = params.withCredentials || false;
-
-  // Apply URL parameters.
-  var parameterString = '';
-  if (typeof(params.urlParams) === 'string') {
-    parameterString = params.urlParams;
-  } else if (typeof(params.urlParams) === 'object') {
-    parameterString = remoting.xhr.urlencodeParamHash(
-        remoting.xhr.removeNullFields_(params.urlParams));
-  }
-  if (parameterString) {
-    base.debug.assert(url.indexOf('?') == -1);
-    url += '?' + parameterString;
-  }
-
-  // Check that the content spec is consistent.
-  if ((Number(params.textContent !== undefined) +
-       Number(params.formContent !== undefined) +
-       Number(params.jsonContent !== undefined)) > 1) {
-    throw new Error(
-        'may only specify one of textContent, formContent, and jsonContent');
-  }
-
-  // Convert the content fields to a single text content variable.
-  /** @type {?string} */
-  var content = null;
-  if (params.textContent !== undefined) {
-    content = params.textContent;
-  } else if (params.formContent !== undefined) {
-    if (!('Content-type' in headers)) {
-      headers['Content-type'] = 'application/x-www-form-urlencoded';
-    }
-    content = remoting.xhr.urlencodeParamHash(params.formContent);
-  } else if (params.jsonContent !== undefined) {
-    if (!('Content-type' in headers)) {
-      headers['Content-type'] = 'application/json; charset=UTF-8';
-    }
-    content = JSON.stringify(params.jsonContent);
-  }
-
-  // Apply the oauthToken field.
-  if (params.oauthToken !== undefined) {
-    base.debug.assert(!('Authorization' in headers));
-    headers['Authorization'] = 'Bearer ' + params.oauthToken;
-  }
+remoting.Xhr.prototype.promise = function() {
+  return this.deferred_.promise();
+};
 
-  return remoting.xhr.startInternal_(
-      method, url, onDone, content, headers, withCredentials);
+/**
+ * Aborts the HTTP request.  Does nothing is the request has finished
+ * already.
+ */
+remoting.Xhr.prototype.abort = function() {
+  return this.nativeXhr_.abort();
 };
 
 /**
@@ -168,23 +207,16 @@ remoting.xhr.start = function(params) {
  *
  * @param {string} method
  * @param {string} url
- * @param {function(XMLHttpRequest):void} onDone
  * @param {?string} content
  * @param {!Object<string,string>} headers
  * @param {boolean} withCredentials
- * @return {XMLHttpRequest} The XMLHttpRequest object.
  * @private
  */
-remoting.xhr.startInternal_ = function(
-    method, url, onDone, content, headers, withCredentials) {
-  /** @type {XMLHttpRequest} */
-  var xhr = new XMLHttpRequest();
-  xhr.onreadystatechange = function() {
-    if (xhr.readyState != 4) {
-      return;
-    }
-    onDone(xhr);
-  };
+remoting.Xhr.prototype.startInternal_ = function(
+    method, url, content, headers, withCredentials) {
+  var xhr = this.nativeXhr_;
+
+  xhr.onreadystatechange = this.onReadyStateChange_.bind(this);
 
   xhr.open(method, url, true);
   for (var key in headers) {
@@ -192,21 +224,80 @@ remoting.xhr.startInternal_ = function(
   }
   xhr.withCredentials = withCredentials;
   xhr.send(content);
-  return xhr;
+};
+
+remoting.Xhr.prototype.onReadyStateChange_ = function(onDone) {
+  var xhr = this.nativeXhr_;
+
+  if (xhr.readyState != 4) {
+    return;
+  }
+
+  var succeeded = 200 <= xhr.status && xhr.status < 300;
+  var response = {
+    status: xhr.status,
+    statusText: xhr.statusText,
+    responseUrl: xhr.responseURL,
+    responseText: !succeeded || this.responseType_ == 'text' ?
+        xhr.responseText : '<invalid responseText>',
+    responseJson: succeeded && this.responseType_ == 'json' ?
+        xhr.response : ['<invalid responseJson>']
+  };
+  this.deferred_.resolve(response);
+};
+
+/**
+ * Returns a copy of the input object with all null or undefined
+ * fields removed.
+ *
+ * @param {Object<string,?string>|undefined} input
+ * @return {!Object<string,string>}
+ * @private
+ */
+remoting.Xhr.removeNullFields_ = function(input) {
+  /** @type {!Object<string,string>} */
+  var result = {};
+  if (input) {
+    for (var field in input) {
+      var value = input[field];
+      if (value != null) {
+        result[field] = value;
+      }
+    }
+  }
+  return result;
+};
+
+/**
+ * Takes an associative array of parameters and urlencodes it.
+ *
+ * @param {Object<string,string>} paramHash The parameter key/value pairs.
+ * @return {string} URLEncoded version of paramHash.
+ */
+remoting.Xhr.urlencodeParamHash = function(paramHash) {
+  var paramArray = [];
+  for (var key in paramHash) {
+    paramArray.push(encodeURIComponent(key) +
+                     '=' + encodeURIComponent(paramHash[key]));
+  }
+  if (paramArray.length > 0) {
+    return paramArray.join('&');
+  }
+  return '';
 };
 
 /**
  * Generic success/failure response proxy.
+ * TODO(jrw): Stop using this.
  *
  * @param {function():void} onDone
  * @param {function(!remoting.Error):void} onError
- * @return {function(XMLHttpRequest):void}
+ * @return {function(remoting.Xhr.Response):void}
  */
-remoting.xhr.defaultResponse = function(onDone, onError) {
-  /** @param {XMLHttpRequest} xhr */
-  var result = function(xhr) {
-    var error =
-        remoting.Error.fromHttpStatus(/** @type {number} */ (xhr.status));
+remoting.Xhr.defaultResponse = function(onDone, onError) {
+  /** @param {remoting.Xhr.Response} xhrr */
+  var result = function(xhrr) {
+    var error = remoting.Error.fromHttpStatus(xhrr.status);
     if (!error.isError()) {
       onDone();
     } else {