Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

Conversation

nathell
Copy link
Contributor

@nathell nathell commented Mar 8, 2019

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:

  • Module A uses re-frame-http-fx, but never uses the transit-*-formats
  • Module B depends on A, and uses transit for something else altogether

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
Copy link
Contributor

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

@antonmos
Copy link

Would be great to merge this!

@superstructor
Copy link
Contributor

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
Copy link
Contributor

Conflicts resolved on local machine and pushed to master.

Thanks @nathell 👍 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants