Skip to content

Don't reject m.request Promise if extract callback supplied #2006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 13, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/change-log.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
- API: `m.redraw()` is always asynchronous ([#1592](https://github.com/MithrilJS/mithril.js/pull/1592))
- API: `m.mount()` will only render its own root when called, it will not trigger a `redraw()` ([#1592](https://github.com/MithrilJS/mithril.js/pull/1592))
- API: Assigning to `vnode.state` (as in `vnode.state = ...`) is no longer supported. Instead, an error is thrown if `vnode.state` changes upon the invocation of a lifecycle hook.
- API: `m.request` will no longer reject the Promise on server errors (eg. status >= 400) if the caller supplies an `extract` callback. This gives applications more control over handling server responses.

#### News

Expand Down
6 changes: 4 additions & 2 deletions docs/request.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ Argument | Type | Required | Descr
`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 not automatically be parsed as JSON.
`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.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
Expand All @@ -81,6 +81,8 @@ A call to `m.request` returns a [promise](promise.md) and triggers a redraw upon

By default, `m.request` assumes the response is in JSON format and parses it into a Javascript object (or array).

If the HTTP response status code indicates an error, the returned Promise will be rejected. Supplying an extract callback will prevent the promise rejection.

---

### Typical usage
Expand Down Expand Up @@ -426,7 +428,7 @@ m.request({

### Retrieving response details

By default Mithril attempts to parse a response as JSON and returns `xhr.responseText`. It may be useful to inspect a server response in more detail, this can be accomplished by passing a custom `options.extract` function:
By default Mithril attempts to parse `xhr.responseText` as JSON and returns the parsed object. It may be useful to inspect a server response in more detail and process it manually. This can be accomplished by passing a custom `options.extract` function:

```javascript
m.request({
Expand Down
2 changes: 1 addition & 1 deletion request/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ module.exports = function($window, Promise) {
if (xhr.readyState === 4) {
try {
var response = (args.extract !== extract) ? args.extract(xhr, args) : args.deserialize(args.extract(xhr, args))
if ((xhr.status >= 200 && xhr.status < 300) || xhr.status === 304 || FILE_PROTOCOL_REGEX.test(args.url)) {
if (args.extract !== extract || (xhr.status >= 200 && xhr.status < 300) || xhr.status === 304 || FILE_PROTOCOL_REGEX.test(args.url)) {
resolve(cast(args.type, response))
}
else {
Expand Down
14 changes: 14 additions & 0 deletions request/tests/test-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -519,5 +519,19 @@ o.spec("xhr", function() {
o(e instanceof Error).equals(true)
}).then(done)
})
o("does not reject on error when extract provided", function(done) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also add a test to ensure that a throwing extract still errors? Or is it already tested (in which this one needs its name clarified)?

mock.$defineRoutes({
"GET /item": function() {
return {status: 500, responseText: JSON.stringify({message: "error"})}
}
})
xhr({
method: "GET", url: "/item",
extract: function(xhr) {return JSON.parse(xhr.responseText)}
}).then(function(data) {
o(data.message).equals("error")
done()
})
})
})
})