search

day8 / re-frame-http-fx

A re-frame "effects handler" for performing Ajax tasks (via cljs-ajax)
MIT License
259 stars 27 forks source link

Don't require cljs-ajax' API wholesale #29

Closed nathell closed 3 years ago

nathell commented 5 years ago

About this PR

This PR changes the day8.re-frame.http-fx ns definition to only require the namespaces within cljs-ajax that are essential to the fx, rather than the whole API. It doesn't impact functionality in any way.

Why?

Short answer: to avoid pulling in cognitect.transit.

Longer answer: cljs-ajax's formats include transit-request-format and transit-response-format, to facilitate efficient sending of Clojure/Script data structures over the wire. That's all well and good, but consumers of re-frame-http-fx may not need it.

Now, Closure Compiler is normally quite efficient in optimizing it away if it's not needed. However, a complication might arise when some other part of the code does need it, and ClojureScript's code splitting facility is in use.

Consider this scenario:

Without this PR, transit will be compiled into A even though it's never used when B is not loaded. With it, the compiler is able to lift transit out of A and into B.

This is exactly what we do at WorksHub. In our case, enabling this change causes our main JS code (advanced optimizations, uncompressed) to weigh 25 KiB less.

danielcompton commented 5 years ago

Thanks for this, rationale makes sense. Do we need the ajax.xhrio require though?

antonmos commented 3 years ago

Would be great to merge this!

superstructor commented 3 years ago

The advantage is pretty minimal, but measurable and there are conceivable benefits based on the scenario described by the OP.

As measured in my build report after resolving merge conflicts, master on left and this PR on right: image

From this we can confirm that transit is optimised out with the change, and it drops a few KB.

superstructor commented 3 years ago

Conflicts resolved on local machine and pushed to master.

Thanks @nathell 👍 😃