Comments (18)
Maybe it would make sense to return response with status 0, like:
{:status 0
:success false
:error-code :bad-response-body
:error exception
:original-status 200 (or whatever)
...}
Status code 0 would probably enable most existing code to handle these responses as error. I think e.g. Chrome uses status code 0 already in some cases (request timeout, CORS).
from cljs-http.
@bsteuber I think this is still the best default behaviour. I think we should do the same here as clj-http. Note, that you can always, build your own custom HTTP client by choosing the needed middleware. Take a look here:
https://github.com/r0man/cljs-http/blob/master/src/cljs_http/client.cljs#L266
from cljs-http.
In my case I get an exception thrown, which I mind a little, but my Problem is that I don't know how to catch it. try
doesn't seem to work:
(defn testex []
(go
(let [response (<! (try (http/get "http://localhost:3000/sim/error" {:with-credentials? false})
(catch :default e
(println "error" e))))]
(println response))))
The URL returns HTTP 500 with content-type "application/json" and not a json body.
What would be the correct approach here? Here's the stacktrace:
Uncaught SyntaxError: Unexpected token S in JSON at position 0
at JSON.parse (<anonymous>)
at cljs_http$util$json_decode (util.cljs?rel=1509894674732:63)
at core.cljs:4829
at Function.cljs.core.update_in.cljs$core$IFn$_invoke$arity$3 (core.cljs:4829)
at cljs$core$update_in (core.cljs:4820)
at cljs_http$client$decode_body (client.cljs?rel=1509894676467:83)
at Function.<anonymous> (client.cljs?rel=1509894676467:181)
at Function.cljs.core.apply.cljs$core$IFn$_invoke$arity$2 (core.cljs:3685)
at cljs$core$apply (core.cljs:3676)
at async.cljs?rel=1509880110795:712
cljs_http$util$json_decode @ util.cljs?rel=1509894674732:63
(anonymous) @ core.cljs:4829
cljs.core.update_in.cljs$core$IFn$_invoke$arity$3 @ core.cljs:4829
cljs$core$update_in @ core.cljs:4820
cljs_http$client$decode_body @ client.cljs?rel=1509894676467:83
(anonymous) @ client.cljs?rel=1509894676467:181
cljs.core.apply.cljs$core$IFn$_invoke$arity$2 @ core.cljs:3685
cljs$core$apply @ core.cljs:3676
(anonymous) @ async.cljs?rel=1509880110795:712
(anonymous) @ async.cljs?rel=1509880110795:702
cljs$core$async$state_machine__29062__auto____1 @ async.cljs?rel=1509880110795:702
cljs$core$async$state_machine__29062__auto__ @ async.cljs?rel=1509880110795:702
cljs$core$async$impl$ioc_helpers$run_state_machine @ ioc_helpers.cljs?rel=1509880107319:35
cljs$core$async$impl$ioc_helpers$run_state_machine_wrapped @ ioc_helpers.cljs?rel=1509880107319:39
(anonymous) @ ioc_helpers.cljs?rel=1509880107319:48
(anonymous) @ channels.cljs?rel=1509880106192:61
cljs$core$async$impl$dispatch$process_messages @ dispatch.cljs?rel=1509880105746:19
channel.port1.onmessage @ nexttick.js:211
from cljs-http.
I just hit this too. @r0man I have to say I'm confused why you think this is the best default behavior. If cljs-http gets bad json from the server, what it does right now is print a stack trace to the console, which, of course, is shorn of any context so it's impossible to tell what line of calling code even caused the problem. This is baffling since cljs-http already has an error reporting mechanism. Why not just catch exceptions and report them through the error mechanism? I also don't even begin to understand how that piece of code could help calling code. Are you suggesting that we just fork cljs-http?
from cljs-http.
try-catch could be added on decode-body
: https://github.com/r0man/cljs-http/blob/master/src/cljs_http/client.cljs#L103 and if decode fn throws error, add :error-code
to the response, and maybe the original exception on some property.
Changing this would cause response to be returned (with empty body) when body can't be decoded.
This would break existing code, because users don't check :error-code
but only :status
of response, and I don't think it would make sense changing status code in this case. Not sure if this is a problem though.
from cljs-http.
I'm probably missing something, but right now, if you hit this bug, the entire state machine vaporizes and it never returns. So whatever you do, it surely has to be better than that. You can't break any calling code in this code path because (I think) it never returns to the calling code.
from cljs-http.
Yes, that's the thing. Because currently response is never returned, current user code probably isn't prepared to check if the returned body was decoded successfully. Returning response without decoded body might break code which isn't prepared for these responses.
from cljs-http.
Another option would be to use something like this:
http://swannodette.github.io/2013/08/31/asynchronous-error-handling
https://github.com/alexanderkiel/async-error
@jmlsf I don't have time to work on this myself at the moment, so a patch is very welcome. No need to fork this, just talk to me. I'm open to suggestions. ;)
from cljs-http.
I believe the problem is here:
(defn wrap-json-response
"Decode application/json responses."
[client]
(fn [request]
(-> #(decode-body % util/json-decode "application/json" (:request-method request))
(async/map [(client request)]))))
The issue is that util/json-decode
call js/JSON.parse
, which can throw.
I think the way to fix this is to change decode-body
to catch exceptions and return what @Deraen suggested like this:
(defn decode-body
"Decocde the :body of `response` with `decode-fn` if the content type matches."
[response decode-fn content-type request-method]
(try
(if (and (not= :head request-method)
(not= 204 (:status response))
(re-find (re-pattern (str "(?i)" (escape-special content-type)))
(str (clojure.core/get (:headers response) "content-type" ""))))
(update-in response [:body] decode-fn)
response)
(catch e
(merge response
{:status 0
:success false
:error-code :bad-response-body
:error e
:original-status (:status response)}))))
The alternative is to push the exception on the channel instead of the call to merge
above, but that would change the library quite a bit because callers would now be expected to use <?
. From @Deraen's comments above, I assume this would not be good. Maybe I misunderstand what @r0man was suggesting?
I should point out I'm happy to try my hand at a PR, but I wanted to see if I was barking up the wrong tree first.
from cljs-http.
@jmlsf @Deraen Yes, I suggested going with <?
, even if it is a breaking change. We can communicate this and bumb the version.
However, I'm open to anything. What kind of API would you prefer as the user of this lib? I'm asking because I actually don't use this lib much at the moment myself, and wonder if all those keys you have to check are not a bit overwhelming.
:status
, :original-status
, :error
and :error-code
are a bit too much for my taste :)
from cljs-http.
I think my two goals are (1) provide a programatic way to deal with errors and (2) make it debuggable. Dealing with 2 is fairly easy: as long as you communicate the error in some way, either by returning a status object or throwing an exception. But dealing with (1) requires that we catch exceptions where they occur in the middleware and assign and document error codes (i.e., :error-code :bad-response-body
). I think that pretty much requires that we have an error/status object with a bunch of fields, because some of them might come from the network layer and some might come from the middleware.
If we were allowed to break the interface, I think the conventions around <?
would be really nice because it provides an unequivocal way of knowing whether things succeeded or not: if the result is a js/Error, it's an error. Otherwise it succeeded. The calling client can use go-try
and <?
if they want. The implementation is pretty easy: just build the status object (if needed) by setting the properties on a js/Error object and put!
it on the channel if there is an error. Otherwise, put what is currently in :body
on the channel directly.
One thing that's nice about using an error object to indicate failure is that you don't have to have :original-status
and :original-code
. You can just leave them be and set the :error / :error-code in the middleware. :success
and :status
would be information about the network call.
I do like the idea of trying to isolate anything that is known to throw an error (like JSON.parse) and assigning (and documenting) an error code for it.
from cljs-http.
@jmlsf I think <?
should return either an instance of ex-info
(which could include more info via ex-data
) or a complete ring response map, with {:status ... :body .. etc }
, not only the value of :body.
from cljs-http.
Sorry you are right of course. At a minimum people will need the status code.
from cljs-http.
I have an identical issue where the server back-end is sending a VALID JSON response but is sending an HTTP 400 error response. The traceback is:
from cljs-http.
I hit what I believe is the same problem when decoding EDN with an unsupported type: "No reader function for tag ordered/set."
from cljs-http.
@kanaka Custom EDN readers are not supported yet I think. We need something similiar to:
https://github.com/dakrone/clj-http/blob/3.x/src/clj_http/client.clj#L64
PR welcome!
from cljs-http.
@r0man custom readers would be great but my more immediate concern was that there doesn't appear to be a way to cleanly catch invalid/unsupported data so that it can be reported properly to the user.
from cljs-http.
@kanaka Yes, that is still an issue. I think we need something what I described here:
#110 (comment)
from cljs-http.
Related Issues (20)
- How to decode an http/get transit json response to a edn? HOT 3
- previewing a request before it gets sent HOT 1
- A request that should return an HTTP 405 instead returns status 0 HOT 3
- GET request URL string with multiple values for one parameter HOT 5
- Unit test failing HOT 3
- Struggling with CORS post request to compojure / ring backend using ring.core / cljs-http... HOT 8
- Response body flattens `schema` object HOT 1
- Binary response data gets mangled HOT 2
- react native support HOT 1
- The custom channel seems doesn't work HOT 3
- jsonp throws exception due to goog.net.Jsonp api changes HOT 3
- README probably has a typo
- jsonp call results in error: TypeError: this.uri_.cloneWithParams is not a function at goog.net.Jsonp.send HOT 8
- Why would the cljs-http.client request be blocked by the cors policy if the clj-http.client request isn't? HOT 1
- Improve README to describe core functionality.
- Uncaught ReferenceError: cljs_http is not defined HOT 2
- Binary data doesn't seem to be included with multipart post? HOT 1
- Mangled JSON replies HOT 2
- Function redefinition warnings with Clojure/Script 1.11 HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from cljs-http.