luminus-framework / luminus-template

a template project for the Luminus framework
http://www.luminusweb.net/
MIT License
645 stars 147 forks source link

`as-transit` function in ajax.cljs needs modification to work with `+re-frame` profile hint #551

Closed harishcm closed 2 years ago

harishcm commented 2 years ago

Hi!

There seems to be a slight issue with the as-transit function in the file ajax.cljs when using the +re-frame profile hint.

When using +re-frame, day8/re-frame-http-fx gets added as a dependency, and the intended way to make ajax requests appears to be by using the :http-xhrio effects handler.

Using

{:http-xhrio (as-transit {:method :get
                          :uri "/api/something"
                          :on-success [::do-something]})}

results in the following error at the browser console:

router.cljc:204 Uncaught Error: ["keywords are not allowed as response formats in ajax calls: " :transit]
    at Object.ajax$util$throw_error [as throw_error] (util.cljc:12)
    at Object.ajax$interceptors$get_response_format [as get_response_format] (interceptors.cljc:231)
    at Object.ajax$simple$normalize_request [as normalize_request] (simple.cljc:42)
    at Object.ajax$simple$ajax_request [as ajax_request] (simple.cljc:70)
    at day8$re_frame$http_fx$http_effect (http_fx.cljs:95)
    at re_frame$fx$do_fx_after (fx.cljc:59)
    at Object.re_frame$interceptor$invoke_interceptor_fn [as invoke_interceptor_fn] (interceptor.cljc:70)
    at Object.re_frame$interceptor$invoke_interceptors [as invoke_interceptors] (interceptor.cljc:108)
    at Object.re_frame$interceptor$execute [as execute] (interceptor.cljc:201)
    at Object.re_frame$events$handle [as handle] (events.cljc:65)
ajax$util$throw_error @ util.cljc:12
ajax$interceptors$get_response_format @ interceptors.cljc:231
ajax$simple$normalize_request @ simple.cljc:42
ajax$simple$ajax_request @ simple.cljc:70
day8$re_frame$http_fx$http_effect @ http_fx.cljs:95
re_frame$fx$do_fx_after @ fx.cljc:59
re_frame$interceptor$invoke_interceptor_fn @ interceptor.cljc:70
re_frame$interceptor$invoke_interceptors @ interceptor.cljc:108
re_frame$interceptor$execute @ interceptor.cljc:201
re_frame$events$handle @ events.cljc:65
eval @ router.cljc:179
eval @ router.cljc:198
eval @ router.cljc:146
eval @ router.cljc:169
G__66437 @ router.cljc:187
channel.port1.onmessage @ nexttick.js:214

The problem seems to be that the :http-xhrio effects handler is dependent upon the ajax-request API of cljs-ajax,which does not allow using keywords (e.g. :transit) to specify formats.

From the day8/re-frame-http-fx README:

image

From the cljs-ajax README:

image


Modifying the as-transit function (for the +re-frame profile) to the following seems to work:

;; injects transit serialization config into request options
(defn as-transit [opts]
  (merge {:format          (ajax/transit-request-format
                             {:writer (transit/writer :json time/time-serialization-handlers)})
          :response-format (ajax/transit-response-format
                             {:reader (transit/reader :json time/time-deserialization-handlers)})}
         opts))

For your kind consideration. Many thanks!

yogthos commented 2 years ago

Thanks for digging into this, the fix makes sense to me. I just pushed out an update for the template with the fix included. Let me know if everything looks good.

harishcm commented 2 years ago

Thanks for the very quick fix! I've tested out the update and it looks good. Just a slight code style indentation issue at merge (see below) - but it makes no difference to the functionality. Thanks!

(defn as-transit [opts]                                                                             
      (merge {:format          (ajax/transit-request-format                                         
                                 {:writer (transit/writer :json time/time-serialization-handlers)}) 
              :response-format (ajax/transit-response-format                                        
                                 {:reader (transit/reader :json time/time-deserialization-handlers)})}
             opts)) 
yogthos commented 2 years ago

Good catch, I'll update the indenting and have it pushed out with the next release.