diff --git a/docs/change-log.md b/docs/change-log.md index 99498a63..c0bb1199 100644 --- a/docs/change-log.md +++ b/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 diff --git a/docs/request.md b/docs/request.md index a8964f54..d805f671 100644 --- a/docs/request.md +++ b/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.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.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 `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 diff --git a/request/request.js b/request/request.js index 3b87eb75..cc08d8d8 100644 --- a/request/request.js +++ b/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) diff --git a/request/tests/test-request.js b/request/tests/test-request.js index 562448cd..57aaa513 100644 --- a/request/tests/test-request.js +++ b/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) { diff --git a/test-utils/xhrMock.js b/test-utils/xhrMock.js index 71c2f5b7..cd67d26c 100644 --- a/test-utils/xhrMock.js +++ b/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 }