http-remote :status-code is always 500 on error

[danskarda]

I wrote global-error-action to notify user about communication errors. My idea was to dispatch on status-code and explain different codes (like 403 forbidden or 401 unauthorized). However htttp-remote error handler overrides all status codes with 500.

https://github.com/fulcrologic/fulcro/blob/3d6442da84d240f0dbbe2338aa43349dcaca6f03/src/main/com/fulcrologic/fulcro/networking/http_remote.cljs#L351

Is it possible that by accident merge parameters are swapped and intention was to provide default value, not override? Or was it intention?

Thank you.

[awkay]

I think the deal was that sometimes there is no status code? I honestly don't remember.

I've made a change on the http-errors branch of the repository and released it as 3.6.0-RC6-SNAPSHOT (sha fcd08f7c8a621d84e71711df2aef91361cea2f7d). It lets you override that behavior.

If that works, let me know and I'll merge it.

If you see some other data that the preprocessor should have, then we can also consider adding it to the signature

[danskarda]

I tested your branch and it works ok.

I thought swapping merge parameters to (merge {:status-code 500} error-result) would be enough. It provides a default value for status-code but does not replace it. But I do not know how much code of other applications depends on error code being equal to 500.

On the other hand there is also was-network-error? function and remote-error in app definition. So if users stick to defaults, they should be fine.

Some random thoughts / alternative solutions to new hook function: a. one can go through all places of error-handler calls and add :status-code 500 there b. extract-response function can store http status in two keys - :status-code and :http-status-code where status-code can be updated by middleware / handlers, http-status-code will be always returned original http response code c. there is also response-middleware so I would expect this could be the place for adding default status-codes d. rather than adding preprocess-error you can allow users to override error-handler. The benefit of providing error-handler is, one can remove log/errors printing references to Fulcro code in console

Just my $0.02. I do not want to overengineer this part. Just had a feeling there are too many hooks and callbacks.

[danskarda]

Sorry I closed the issue by accident

[awkay]

Right, reordering the merge is a breaking change, so I won't do that.

I like (b), but the change I made allows you to look at the actual result and "do whatever", so it is maximally flexible at that layer at least, but you are right that being able to sub the error-handler is probably too complex, given that you could just copy the entire remote and change it however you want.

You're right that the middleware is a good place to process the response in some way, and I do expect that. The constraint of not wanting to make breaking changes it the main critical concern for me, and the current remote has been working for me in production products for years. So, I'm not terribly concerned about expanding the functionality a lot.

That said, I'll give it a bit more consideration, because once I release this, then I'm stuck with it :D

[danskarda]

As I wrote I was able to make a workaround using response midleware:

(defn wrap-response-save-error
  [handler]
  (let [handler (or handler (http-remote/wrap-fulcro-response))]
    (fn [response]
      (let [response (handler response)]
        (if-let [status-code  (:status-code response)]
          (assoc response :http-status-code status-code)
          response)))))

I am fine with preprocess-error solution. I just had a impression there are too many hooks, when general solution exist is enough.

[awkay]

Fixed by PR #530