cryogen-project / cryogen-core

Cryogen's core
Eclipse Public License 1.0
69 stars 62 forks source link

Shallower project structure #117

Closed dominicfreeston closed 5 years ago

dominicfreeston commented 5 years ago

This ~(mostly)~ addresses issue #98. It will obviously require some related changes to the docs and the template (and maybe a quick migration guide for people who want to update), which I'm also happy to help with.

~This leaves the output into resources/public/ because the ring server relies on this. I don't know if it can be changed.~

Project Structure Changes:

Other code changes:

The test coverage, although useful, is obviously not total so there's certainly a risk I've broken other things. I've ran this against all 4 included templates and everything seems to be working OK.

Thanks 😃

yogthos commented 5 years ago

The change looks good to me, and it should be possible to specify the public folder for the wrap-resource middleware as described here.

dominicfreeston commented 5 years ago

Ok so looking into it, we can make the ring server point to public instead of resource/public simply by changing resource-response to file-response and route/resources to route/files in server.clj within the Cryogen lein template.

The downside of this is that it will make migration marginally more difficult. Though if it's worth changing probably might as well break everything at once and have some migration instruction - the easiest way might just be "check out the latest template and port your content over".

yogthos commented 5 years ago

One idea could be to add an optional config variable for the path, so it would default to public, but you could explicitly set it to resources/public or something else entirely. This should help with the migration.

dominicfreeston commented 5 years ago

Yeah that certainly could be useful. A few things to think about first:

I'm happy to look into these at some point if you think it's necessary, but it might take me a little while before I manage to get round to it. If you're happy without the safeguards and the more stateful approach then it should be really straightforward. I'm also happy to revert it back to resource/public if that's better, I'm personally much less bothered about the location of the generated content.

yogthos commented 5 years ago

I'd say specifying the path is mostly a nice to have, but I agree that having a guard would be good. I'd vote for merging things in without that, and creating an enhancement issue for later. Ideally, I think it would be best to pass the config around explicitly.

lacarmen commented 5 years ago

I do like the idea of being able to configure the output location. It would make GH pages deployment a lot simpler.

I'm okay with setting the path in compile-assets but I think we should at least have the safeguards that you mentioned.

Though if it's worth changing probably might as well break everything at once and have some migration instruction - the easiest way might just be "check out the latest template and port your content over".

I'm okay with the "break everything at once" approach as well :)

dominicfreeston commented 5 years ago

Ok thanks for all the feedback! Making GH deploy easier would be a nice win. Don’t mind leaving this open in case there’s need to fix something else before it’s finished, or merge into a dev branch, whatever works for you

dominicfreeston commented 5 years ago

OK there's now an option for setting paths (:public-dest). There's some breaking changes in there with moving things out into a separate namespace - let me know what you think.

I've gone for setting the path in the var for the sake of reducing the scope of the change - I did have a go at passing the config around but then it felt like I should be changing much more when going down that path. Probably best to leave as a separate change in the future?

I've added path-validation to ensure :public-dest, :resources and the main folders (content, themes, src, target) don't overlap, with some tests to check this. I split some things out in to a new cryogen-core.config namespace.


Some other thoughts:


server.clj in the main cryogen repo would need to look something like this for things to work properly:

(ns cryogen.server
  (:require [compojure.core :refer [GET defroutes]]
            [compojure.route :as route]
            [ring.util.response :refer [redirect file-response]]
            [ring.util.codec :refer [url-decode]]
            [ring.middleware.file :refer [wrap-file]]
            [cryogen-core.watcher :refer [start-watcher!]]
            [cryogen-core.plugins :refer [load-plugins]]
            [cryogen-core.compiler :refer [compile-assets-timed]]
            [cryogen-core.config :refer [resolve-config]]
            [cryogen-core.io :refer [path]]))

(defn init []
  (load-plugins)
  (compile-assets-timed)
  (let [ignored-files (-> (resolve-config) :ignored-files)]
    (start-watcher! "content" ignored-files compile-assets-timed)))

(defn wrap-subdirectories
  [handler]
  (fn [request]
    (let [req-uri (.substring (url-decode (:uri request)) 1)
          res-path (if (or (= req-uri "") (= req-uri "/"))
                     (path "/index.html")
                     (path (str req-uri ".html")))]
      (or (file-response res-path {:root (:public-dest (resolve-config))})
          (handler request)))))

(defroutes routes
  (GET "/" [] (redirect (let [config (resolve-config)]
                          (path (:blog-prefix config)
                                (when-not (:clean-urls? config) "index.html")))))
  (route/files "/" {:root (:public-dest (resolve-config))})
  (route/not-found "Page not found"))

(def handler (wrap-subdirectories routes))
lacarmen commented 5 years ago

Thanks @dominicfreeston! I'll try to review/test soon. 😄

lacarmen commented 5 years ago

I did have a go at passing the config around but then it felt like I should be changing much more when going down that path. Probably best to leave as a separate change in the future?

I'm okay with this.

I was wondering if we should make the resources be with reference to the content folder rather than root, which would simplify a few things including what folders to watch for changes, and might be a bit more intuitive.

I'd prefer this. I find it's a bit weird having assets like css and img floating around the root folder. I think config.edn should be moved into the content folder as well, otherwise the watcher doesn't pick up on config changes (unless you have other ideas to address this).

compile-sass->css! doesn't need a base-dir (it's just root folder) so just pass in whole config

If we go with assets relative to content/ then the sass code will have to be updated a bit as well.


I tested with each of the templates as well and nothing seems to be broken :) I think we can merge this after the assets are addressed.

dominicfreeston commented 5 years ago

OK great, I'm away this weekend so not sure I'll get to it before next week - I think making the root content will require some changes to how theme assets are handled as well (since the path is specified from root at the moment)

dominicfreeston commented 5 years ago

@lacarmen apologies for the delay

Now changed so all resources paths are with reference to content/ - makes it much easier to ensure the output path doesn't overlap with anything else 👍

I've also updated it so that all paths in a theme are with reference to the theme folder (this will require updating the default lotus theme) - seems more consistent and predictable, what do you think?

As part of the changes I've modified how the config-specified resources for a theme are loaded, with the nice side-effect that they now get printed to the console as part of the "copying theme resources" phase. The main risk is that I no longer use deep-merge, I just load up the sass-src and resources, is there anything else we'd expect a theme to be able to specify that I might be losing by doing this?

lacarmen commented 5 years ago

Thanks @dominicfreeston! I'll try to get around to testing this week.

I've also updated it so that all paths in a theme are with reference to the theme folder (this will require updating the default lotus theme) - seems more consistent and predictable, what do you think?

Sounds good to me

The main risk is that I no longer use deep-merge, I just load up the sass-src and resources, is there anything else we'd expect a theme to be able to specify that I might be losing by doing this?

Not sure about this one off the top of my head - I'll investigate.

I'd like to release this after #119. I don't think there will be any conflicts in cryogen-core, but server.clj in the main repo might need to be updated. Do you mind taking a look at this? (I just merged #119 and cryogen-project/cryogen#181)

dominicfreeston commented 5 years ago

Just to update, I haven't forgotten about this, just been quite busy - should be able to get to it this weekend. Todo:

  1. resolve conflict on this PR
  2. update server.clj, project structure + README in main cryogen repo
  3. update docs repo

Let me know if there's anything else that needs doing for this

lacarmen commented 5 years ago

No worries @dominicfreeston. I appreciate the work you've done on this!

I think that list covers everything. The tests could use some beefing up as you mentioned, but it's not critical for this PR. (Of course I'm not going to stop you though if you want to add to them 😄)

dominicfreeston commented 5 years ago

@lacarmen OK I've opened a couple of PRs against the other relevant repos, I think that's everything 😅.

The main thing missing is updating version numbers (I wasn't sure if you wanted to go up to 0.2.0 because of quite big breaking changes or if you wanted to just bump it as normal).

Thanks for your help and patience!

lacarmen commented 5 years ago

@dominicfreeston I tested the whole thing together and looks like it's all good! Thanks a bunch for taking this on 😄

I can bump up all the numbers - the core will go up to 0.2.0 and the template will go to 0.5.0.