search

abogoyavlensky / clojure-kamal-example

Full-stack Clojure/Script setup with deployment using Kamal
MIT License
9 stars 1 forks source link

hash-css shouldn't be a build hook #2

Open thheller opened 4 months ago

thheller commented 4 months ago

This might come over as nitpicky, but you really shouldn't be using shadow-cljs :build-hooks for hash-css.

A good indicator for when things should be a build hook is if they use build-state. You are not using it at all, so this absolutely should not be a hook. I feel it makes the setup needlessly complicated. I would advise removing that build hook entirely and just instead directly call the function from here

https://github.com/abogoyavlensky/clojure-kamal-example/blob/8e1ae3673c550abd112479df17a578ebbb3bbdbf/build.clj#L28-L34

Heck, the whole api.util.build becomes obsolete if you just move the code into build.clj. You could even remove the copy-file hook and just do that from within build.clj.

thheller commented 4 months ago

It is also not great to use copy-file and copying the same file onto itself, as that makes the whole build thing not repeatable.

abogoyavlensky commented 4 months ago

@thheller Hi! First of all, thank you a lot for the Shadow CLJS! Your feedback is greatly appreciated. 🙏

I feel it makes the setup needlessly complicated. I would advise removing that build hook entirely and just instead directly call the function from here

Yes, I agree, will rework this part as you suggested.

It is also not great to use copy-file and copying the same file onto itself, as that makes the whole build thing not repeatable.

Makes sense. I didn't plan to use the build function outside the Docker image build, so overriding the same file wan't seemed to me a problem. It's reproducible at the Docker image level. But in general you are completely right and I'll think about improvements.