duct-framework / duct

Server-side application framework for Clojure
MIT License
1.13k stars 51 forks source link

Allow external profiles to modify config.edn file #89

Closed werenall closed 5 years ago

werenall commented 5 years ago

External profiles, to be really useful, need to be able to modify config.edn file (and probably some others like dev.edn too). This PR is an attempt to enable such a feature by adding a :config-mods attribute to profiles. It's a vector of functions that will be applied to the rendered config.edn. We came to a conclusion that as simple solution as we do in project.clj ({{#deps}}{{&.}}{{/deps}}) is not an option here. Profiles very often not only need to add new key-value pairs to the config but also modify an existent key at deeper lvl. Perhaps we could have both: :extra-config and :config-mods... Please check commit 166b4e4 for example usage of :config-mods.

I know that this PR is not fully finished (unexplained ripoffs from leiningen.template and lack of docstrings). But I wanted to push it through before weekend. Will continue on that on Monday.

weavejester commented 5 years ago

I'd like to avoid transformations to config.edn in the template, as that's the purpose of modules in Duct. If a transformation is written as a module, rather than into the template, then the module can be applied to existing projects as well to new projects.

werenall commented 5 years ago

I just realised I forgot to clean it up in some places. Closed for a while.

werenall commented 5 years ago

This is as far as we could get this far. Please check out this project to see what we're aiming for. As you can see in README we still need (or at least we believe we do) one missing piece of configuration - we are not sure how to inject routes :duct.routes/cascading collection. Technically we could do it within a module, but we think it's too obscure.

Also a delighter for us would be an ability to inject :host "0.0.0.0" :port 4001 to :repl-options in project.clj. However I'd understand if that's too personal opinion.

weavejester commented 5 years ago

Overall the code looks fine, though needs some cleaning up in terms of formatting, indentation, etc.

I think we can remove :extra-config and instead have keys :modules, :profile-base and :profile-dev.

I think we can replace the k/v stuff by mapping the key value pairs to strings.

Adding stuff to the routes should be be a case of adding a variable to the web profile hint. Could you give an example of what sort of routes are needed?

Adding a :repl-options key is fine as well, I think.

werenall commented 5 years ago
  1. Re routes:
    [#ig/ref :<project-name>.handler/root
    #ig/ref :<project-name>.handler/config]

We want to include /root that works in the same way that your /example works. /config is a route our webapp calls to get some info from server. Key definition for /root Key definition for /config

So what do you mean by a variable exactly? Same as what we do with deps? A vector that will be concatenated later? I'd be happy with that solution. Of course there's an issue with ordering of that routes, but I have no idea how to handle that in a simple way. I guess that that amount of human config is manageable.

  1. Re k/v stuff replaced by strings: I've experimented for quite some time already with multiline strings. I tried \n, \r, \nr, Unicode for newline and nothing worked as I expected. \n for instance jumps to newline, but starts from col0.
weavejester commented 5 years ago

So what do you mean by a variable exactly?

The :vars key specifies additional variables that are passed to the templates. These are global, so you could add an extra-cascading-routes variable to the configuration. The downside is that you'd only be able to use this variable in one profile, because there wouldn't be any merging functionality. However, that might be fine in this case.

I've experimented for quite some time already with multiline strings. I tried \n, \r, \nr, Unicode for newline and nothing worked as I expected. \n for instance jumps to newline, but starts from col0.

You know the indentation for extra config is one space, so you can just join on " \n", no?

werenall commented 5 years ago

extra-cascading-routes - Why there won't be any merging functionality? You want this to be winner takes all kind of attribute? I like it. This way we are limited, but guaranteed a good result.

As for the indentation - this is doable, but I suspect that it's going to be ugly. Consider this code: :modules {:duct.module/cljs {:main (symbol (str project-ns ".client"))}}

Here we don't know what will that project-ns be. It could be as short as foobar but can also be as long as amazingcompany.with-even-better-app. So yeah, solution to this could be this:

(apply str (repeat (count "amazingcompany") " "))
=> "              "

but... isn't that too hacky?

weavejester commented 5 years ago

Why there won't be any merging functionality? You want this to be winner takes all kind of attribute? I like it. This way we are limited, but guaranteed a good result.

Well, :vars is already a winner takes all, because it uses a merge. See: duct.clj#L88.

To be clear, I'm not suggesting introducing any new code; the :vars key is already there, so we just need to update the config.edn template.

Here we don't know what will that project-ns be. It could be as short as foobar but can also be as long as amazingcompany.with-even-better-app.

Why would that affect the indentation?

Let me show you what I mean with three examples:

;; special case for empty map
{:duct.module/example {}}
":duct.module/example {}"

;; for non-empty maps, pretty print and indent all lines by 1 space,
;; then place on line below key
{:duct.module/example {:main 'foobar.client}}
=> ":duct.module/example\n {:main foobar.client}" 

;; if the value is a string, use that verbatim
{:duct.module/example "\n {:main\n foobar.client}"}
=> ":duct.module/example\n {:main\n foobar.client}"
werenall commented 5 years ago

Oh, now I see. I thought you were aiming for something like this:

{keyword value
         value continues
         value continues}

All clear now. I should be done with it by tomorrow then 😄

werenall commented 5 years ago

Ok, I think that should do now. Look, provided that chunk of config:

:modules {:hydrogen.module.cljs/core {}
          :blah {:a 1}
          :blah2 " {:a 1}"
          :duct.module/example "\n {:main\n foobar.client}"
          :duct.module/example2 "\n {:main\n  foobar.client}"}

this gets generated:

 :hydrogen.module.cljs/core {}
 :blah
 {:a 1}
 :blah2 {:a 1}
 :duct.module/example
 {:main
 foobar.client}
 :duct.module/example2
 {:main
  foobar.client}
weavejester commented 5 years ago

Thanks for the update. I have a few suggestions/changes.

werenall commented 5 years ago

Hi! Have you had some time to review if those changes really resolve your comments? 🙂

weavejester commented 5 years ago

As a heads up, I'm going to release version 0.12.0 of Duct, with the limited support for external profile hints we've added so far. This is just so that profiles are not blocking updates to Duct, and in version 0.12.1 onwards we'll incorporate this PR and more that extend the functionality.

weavejester commented 5 years ago

This all looks fine. Can you squash your changes down?

werenall commented 5 years ago

Sure thing! We just pushed the squashed commit. Please let us know if this is what you had in mind.

weavejester commented 5 years ago

Almost there. Can you change the commit message to:

Allow external profiles to modify configuration

Adds more ways for external profiles to alter the configuration when
creating a new project.

Allows the use of the following keys: :modules, :profile-base,
:profile-dev and :repl-options.

Also adds :cascading-routes for customizing the cascading router.
werenall commented 5 years ago

Sure thing. Done :)

weavejester commented 5 years ago

Released as 0.12.1.