MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications
https://mithril.js.org
MIT License
14.02k stars 925 forks source link

Include error code in xhr request error? #1876

Closed hamaluik closed 7 years ago

hamaluik commented 7 years ago

Right now, I have no good way of retrieving the error code from any errors that the server sends, as an error object is created with the server response and the parsed message with no contextual information.

This issue is related to #1866

Right now, the xhr error code looks like: (https://github.com/MithrilJS/mithril.js/blob/next/request/request.js#L91-L98)

if ((xhr.status >= 200 && xhr.status < 300) || xhr.status === 304 || FILE_PROTOCOL_REGEX.test(args.url)) {
    resolve(cast(args.type, response))
}
else {
    var error = new Error(xhr.responseText)
    for (var key in response) error[key] = response[key]
    reject(error)
}

Which throws an unrecognizable error if the response status isn't in the 200 range.

If my web service responds with a 401 with no body (a perfectly valid response), there is no way for me to determine if the error was a 401 or some other error (say a 500 or something) without modifying what is on the server, which isn't always possible. I also cannot use the extract method as a workaround as the error throwing works completely aside from the extract method.

For example:

m.request({
    method: "POST",
    url: "/auth/login",
    data: {name: "jim", pass: "password1"}
})
.then(function(result) {
    console.log(result)
})
.catch(function(error) {
    // is this a 404? A 500? A 403? A 401? Who knows!
   console.error(error);
})

My suggestion:

Modify the https://github.com/MithrilJS/mithril.js/blob/next/request/request.js#L91-L98 to be something like:

if ((xhr.status >= 200 && xhr.status < 300) || xhr.status === 304 || FILE_PROTOCOL_REGEX.test(args.url)) {
    resolve(cast(args.type, response))
}
else {
    var error = new Error(xhr.responseText)
    error.code = xhr.status;
    for (var key in response) error[key] = response[key]
    reject(error)
}

Or if going along with #1866:

var error = new Error(xhr.responseText);
error.code = xhr.status;
error.response = response;
reject(error);

This has a potential issue if the server body has a key name code, but the same can be said about the message key that already exists.

pdfernhout commented 7 years ago

Another possible change is to just include the xhr as a field of the Error. For jQuery, you get the xhr when there is an error callback: http://api.jquery.com/jQuery.ajax/

But, to use what is available now, consider there a loop in the error handler that takes fields from the response and puts them into the error:

for (var key in response) error[key] = response[key]

where response is defined earlier by:

var response = (args.extract !== extract) ? args.extract(xhr, args) : args.deserialize(args.extract(xhr, args))

So, you could process the xhr result yourself in extract, and, if there is an error, indicate it some way in the response. But that is essentially duplicating some the error handling yourself.

pdfernhout commented 7 years ago

Here is a related example of an extract function that sets status to xhr.status (mentioned on Gitter just now): https://mithril.js.org/request.html#retrieving-response-details

hamaluik commented 7 years ago

@pdfernhout I was trying that and for some reason wasn't getting what I was looking for. I must have been suffering from late-in-the-day brain or something because it's definitely working now. Thanks!

Still, I feel that it's a bit awkward to have to implement custom overriding behaviour just to get this common piece of information.