Bring some sanity to request parsing and error handling (#2335)

* Update docs/request.md
* Bring some sanity to request parsing and error handling

- The browser can do JSON parsing itself. Let's defer to that where
  possible. (A few IE hacks are required here, though.)
- Don't propagate any error that occurs before `deserialize`/`extract`.
- Allow sending raw array buffers/blobs/etc. to `deserialize`.
- Align behavior more closely with the XHR spec.
- Send the more useful parsed response to `deserialize`, not the less
  useful string response.

docs/change-log.md

@@ -39,6 +39,7 @@
 - route, request: Interpolated arguments are URL-escaped (and for declared routes, URL-unescaped) automatically. If you want to use a raw route parameter, use a variadic parameter like in `/asset/:path.../view`. This was previously only available in `m.route` route definitions, but it's now usable in both that and where paths are accepted. ([#2361](https://github.com/MithrilJS/mithril.js/pull/2361))
 - route, request: Interpolated arguments are *not* appended to the query string. This means `m.request({url: "/api/user/:id/get", params: {id: user.id}})` would result in a request like `GET /api/user/1/get`, not one like `GET /api/user/1/get?id=1`. If you really need it in both places, pass the same value via two separate parameters with the non-query-string parameter renamed, like in `m.request({url: "/api/user/:urlID/get", params: {id: user.id, urlID: user.id}})`. ([#2361](https://github.com/MithrilJS/mithril.js/pull/2361))
 - route, request: `m.route.set`, `m.request`, and `m.jsonp` all use the same path template syntax now, and vary only in how they receive their parameters. Furthermore, declared routes in `m.route` shares the same syntax and semantics, but acts in reverse as if via pattern matching. ([#2361](https://github.com/MithrilJS/mithril.js/pull/2361))
+- request: `options.responseType` now defaults to `"json"` if `extract` is absent, and `deserialize` receives the parsed response, not the raw string. If you want the old behavior, [use `responseType: "text"`](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/responseType). ([#2335](https://github.com/MithrilJS/mithril.js/pull/2335))
 
 #### News
 

docs/request.md

@@ -51,13 +51,13 @@ Argument                  | Type                              | Required | Descr
 `options.password`        | `String`                          | No       | A password for HTTP authorization. Defaults to `undefined`. This option is provided for `XMLHttpRequest` compatibility, but you should avoid using it because it sends the password in plain text over the network.
 `options.withCredentials` | `Boolean`                         | No       | Whether to send cookies to 3rd party domains. Defaults to `false`
 `options.timeout`         | `Number`                          | No       | The amount of milliseconds a request can take before automatically being [terminated](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/timeout).  Defaults to `undefined`.
-`options.responseType`    | `String`                          | No       | The expected [type](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/responseType) of the response. Defaults to `undefined`.
+`options.responseType`    | `String`                          | No       | The expected [type](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/responseType) of the response. Defaults to `""` if `extract` is defined, `"json"` if missing. If `responseType: "json"`, it internally performs `JSON.parse(responseText)`.
 `options.config`          | `xhr = Function(xhr)`             | No       | Exposes the underlying XMLHttpRequest object for low-level configuration. Defaults to the [identity function](https://en.wikipedia.org/wiki/Identity_function).
 `options.headers`         | `Object`                          | No       | Headers to append to the request before sending it (applied right before `options.config`).
 `options.type`            | `any = Function(any)`             | No       | A constructor to be applied to each object in the response. Defaults to the [identity function](https://en.wikipedia.org/wiki/Identity_function).
 `options.serialize`       | `string = Function(any)`          | No       | A serialization method to be applied to `data`. Defaults to `JSON.stringify`, or if `options.data` is an instance of [`FormData`](https://developer.mozilla.org/en/docs/Web/API/FormData), defaults to the [identity function](https://en.wikipedia.org/wiki/Identity_function) (i.e. `function(value) {return value}`).
-`options.deserialize`     | `any = Function(string)`          | No       | A deserialization method to be applied to the `xhr.responseText`. Defaults to a small wrapper around `JSON.parse` that returns `null` for empty responses. If `extract` is defined, `deserialize` will be skipped.
+`options.deserialize`     | `any = Function(any)`          | No       | A deserialization method to be applied to the `xhr.response` or normalized `xhr.responseText`. Defaults to the [identity function](https://en.wikipedia.org/wiki/Identity_function). If `extract` is defined, `deserialize` will be skipped.
-`options.extract`         | `any = Function(xhr, options)`    | No       | A hook to specify how the XMLHttpRequest response should be read. Useful for processing response data, reading headers and cookies. By default this is a function that returns `xhr.responseText`, which is in turn passed to `deserialize`. If a custom `extract` callback is provided, the `xhr` parameter is the XMLHttpRequest instance used for the request, and `options` is the object that was passed to the `m.request` call. Additionally, `deserialize` will be skipped and the value returned from the extract callback will be left as-is when the promise resolves. Furthermore, when an extract callback is provided, exceptions are *not* thrown when the server response status code indicates an error.
+`options.extract`         | `any = Function(xhr, options)`    | No       | A hook to specify how the XMLHttpRequest response should be read. Useful for processing response data, reading headers and cookies. By default this is a function that returns `options.deserialize(parsedResponse)`, throwing an exception when the server response status code indicates an error or when the response is syntactically invalid. If a custom `extract` callback is provided, the `xhr` parameter is the XMLHttpRequest instance used for the request, and `options` is the object that was passed to the `m.request` call. Additionally, `deserialize` will be skipped and the value returned from the extract callback will be left as-is when the promise resolves.
 `options.useBody`         | `Boolean`                         | No       | Force the use of the HTTP body section for `data` in `GET` requests when set to `true`, or the use of querystring for other HTTP methods when set to `false`. Defaults to `false` for `GET` requests and `true` for other methods.
 `options.background`      | `Boolean`                         | No       | If `false`, redraws mounted components upon completion of the request. If `true`, it does not. Defaults to `false`.
 **returns**               | `Promise`                         |          | A promise that resolves to the response data, after it has been piped through the `extract`, `deserialize` and `type` methods

request/request.js

@@ -79,7 +79,7 @@ module.exports = function($window, Promise) {
 			}
 			if (args.withCredentials) xhr.withCredentials = args.withCredentials
 			if (args.timeout) xhr.timeout = args.timeout
-			if (args.responseType) xhr.responseType = args.responseType
+			xhr.responseType = args.responseType || (typeof args.extract === "function" ? "" : "json")
 
 			for (var key in args.headers) {
 				if ({}.hasOwnProperty.call(args.headers, key)) {
@@ -96,19 +96,38 @@ module.exports = function($window, Promise) {
 				if (xhr.readyState === 4) {
 					try {
 						var success = (xhr.status >= 200 && xhr.status < 300) || xhr.status === 304 || (/^file:\/\//i).test(url)
-						var response = xhr.responseText
+						// When the response type isn't "" or "text",
+						// `xhr.responseText` is the wrong thing to use.
+						// Browsers do the right thing and throw here, and we
+						// should honor that and do the right thing by
+						// preferring `xhr.response` where possible/practical.
+						var response = xhr.response, message
+
+						if (response == null) {
+							try {
+								response = xhr.responseText
+								// Note: this snippet is intentionally *after*
+								// `xhr.responseText` is accessed, since the
+								// above will throw in modern browsers (thus
+								// skipping the rest of this section). It's an
+								// IE hack to detect and work around the lack of
+								// native `responseType: "json"` support there.
+								if (typeof args.extract !== "function" && xhr.responseType === "json") response = JSON.parse(response)
+							}
+							catch (e) { response = null }
+						}
+
 						if (typeof args.extract === "function") {
 							response = args.extract(xhr, args)
 							success = true
 						} else if (typeof args.deserialize === "function") {
 							response = args.deserialize(response)
-						} else {
-							try {response = response ? JSON.parse(response) : null}
-							catch (e) {throw new Error("Invalid JSON: " + response)}
 						}
 						if (success) resolve(response)
 						else {
-							var error = new Error(xhr.responseText)
+							try { message = xhr.responseText }
+							catch (e) { message = response }
+							var error = new Error(message)
 							error.code = xhr.status
 							error.response = response
 							reject(error)

request/tests/test-request.js

@@ -290,7 +290,7 @@ o.spec("xhr", function() {
 				}
 			})
 			xhr({method: "GET", url: "/item", deserialize: deserialize}).then(function(data) {
-				o(data).equals("{\"test\":123}")
+				o(data).deepEquals({test: 123})
 			}).then(done)
 		})
 		o("deserialize parameter works in POST", function(done) {
@@ -304,12 +304,12 @@ o.spec("xhr", function() {
 				}
 			})
 			xhr({method: "POST", url: "/item", deserialize: deserialize}).then(function(data) {
-				o(data).equals("{\"test\":123}")
+				o(data).deepEquals({test: 123})
 			}).then(done)
 		})
 		o("extract parameter works in GET", function(done) {
 			var extract = function() {
-				return JSON.stringify({test: 123})
+				return {test: 123}
 			}
 
 			mock.$defineRoutes({
@@ -318,12 +318,12 @@ o.spec("xhr", function() {
 				}
 			})
 			xhr({method: "GET", url: "/item", extract: extract}).then(function(data) {
-				o(data).equals("{\"test\":123}")
+				o(data).deepEquals({test: 123})
 			}).then(done)
 		})
 		o("extract parameter works in POST", function(done) {
 			var extract = function() {
-				return JSON.stringify({test: 123})
+				return {test: 123}
 			}
 
 			mock.$defineRoutes({
@@ -332,7 +332,7 @@ o.spec("xhr", function() {
 				}
 			})
 			xhr({method: "POST", url: "/item", extract: extract}).then(function(data) {
-				o(data).equals("{\"test\":123}")
+				o(data).deepEquals({test: 123})
 			}).then(done)
 		})
 		o("ignores deserialize if extract is defined", function(done) {
@@ -545,7 +545,8 @@ o.spec("xhr", function() {
 			})
 			xhr({method: "GET", url: "/item"}).catch(function(e) {
 				o(e instanceof Error).equals(true)
-				o(e.message).equals(JSON.stringify({error: "error"}))
+				o(e.message).equals("[object Object]")
+				o(e.response).deepEquals({error: "error"})
 				o(e.code).equals(500)
 			}).then(done)
 		})
@@ -568,7 +569,8 @@ o.spec("xhr", function() {
 				}
 			})
 			xhr({method: "GET", url: "/item"}).catch(function(e) {
-				o(e.message).equals("Invalid JSON: error")
+				o(e.message).equals("null")
+				o(e.response).equals(null)
 			}).then(done)
 		})
 		o("triggers all branched catches upon rejection", function(done) {

test-utils/xhrMock.js

@@ -43,13 +43,28 @@ module.exports = function() {
 				args.user = user
 				args.password = password
 			}
+			this.responseType = ""
+			this.response = null
+			Object.defineProperty(this, "responseText", {get: function() {
+				if (this.responseType === "" || this.responseType === "text") {
+					return this.response
+				} else {
+					throw new Error("Failed to read the 'responseText' property from 'XMLHttpRequest': The value is only accessible if the object's 'responseType' is '' or 'text' (was '" + this.responseType + "').")
+				}
+			}})
 			this.send = function(body) {
 				var self = this
 				if(!aborted) {
 					var handler = routes[args.method + " " + args.pathname] || serverErrorHandler.bind(null, args.pathname)
 					var data = handler({url: args.pathname, query: args.search || {}, body: body || null})
 					self.status = data.status
-					self.responseText = data.responseText
+					// Match spec
+					if (self.responseType === "json") {
+						try { self.response = JSON.parse(data.responseText) }
+						catch (e) { /* ignore */ }
+					} else {
+						self.response = data.responseText
+					}
 				} else {
 					self.status = 0
 				}