duct-framework / duct

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

Introduce external profiles #84

Closed werenall closed 5 years ago

werenall commented 5 years ago

Please refer to https://github.com/magnetcoop/hydrogen.example.duct-profile for example usage.

weavejester commented 5 years ago

Thank you for the PR. There are a few things I think we can do to simplify this design. Often, a good starting place is often to consider the data that defines a profile.

At minimum, we need to be able to add files and directories to the project we're creating:

We also want templates, and it would be nice if we could introduce our own variables, over the default variables that Duct introduces to the template.

Finally, we'd like a way of adding extra dependencies to the project file:

If we know what dependencies we have ahead of time, then we don't need a rewrite-clj dependency, or to rewrite existing Clojure code.

So a profile might end up looking something like:

{:files      {"resources/{{dirs}}/example.png" "my-profile/example.png"}
 :dirs       ["src/{{dirs}}/handlers"]
 :templates  {"src/{{dirs}}/example.clj" "my-profile/example.clj"}
 :extra-vars {:foo "example"}
 :extra-deps [['example/dependency "0.1.0"]]}

When we've decided on the shape of our data, we can consider how to produce it. Since we're taking our cue from Leiningen's templates, we can use a function:

(defn example [project-info]
  {:files ...
   :dirs ...
   ...})

We can pass in some sort of map that gives basic information about the project, like the project name. A map ensures that we can add more information in future.

Next, we want a function that finds a profile given its name:

(find-profile "+example") => #function[...]

This can look locally first, then use your code for finding an external profile.

Once we have all our profiles, we want something to intelligently merge them:

(reduce merge-profiles base-profile profiles) => {...}

Then something to ensure the resulting merged profile is value:

(assert-valid-profile! merged-profile)

And finally something to execute the profile:

(create-project-from-profile merged-profile project-vars)

So overall, the core of the template might look like:

(let [profiles       (map find-profile hints)
      merged-profile (reduce merge-profiles base-profile profiles)]
  (assert-valid-profile! merged-profile)
  (create-project-from-profile merged-profile (project-vars project))

I'll take a look at this further over the weekend and Monday, but those are my initial thoughts on the design. I don't think we need rewrite-clj, and if we're going to be loading namespaces dynamically, then I think mimicking Leiningen's template lookup isn't a bad choice.

werenall commented 5 years ago

Just read your response again, this time very carefully. I understand now that your intention is that instead of making a pluggable acceptance of external profiles, we should rework one step earlier: profile-files and profile-data from current implementation should become the find-profile you proposed and work for each profile, no matter if it's native or external (that concept would actually cease to exist then because functionally they'll be the same)

However what I don't understand yet is how you want to create project.clj of the resulting project? Up to this point Duct simply used project.clj file written in Stencil format. But now that Duct would have dynamically created list of dependencies, it wouldn't be an option anymore, no? The only idea that comes to my mind is generating content of project.clj in memory and then spitting it to resulting directory. Actually that idea is divisible into two: we could generate a definite project.clj or generate a Stencil template for it.

Also we need to make sure that project.clj would be properly formatted before finishing the job. Should we go with running lein cljfmt fix as part of the script?

weavejester commented 5 years ago

Just read your response again, this time very carefully. I understand now that your intention is that instead of making a pluggable acceptance of external profiles, we should rework one step earlier: profile-files and profile-data from current implementation should become the find-profile you proposed and work for each profile, no matter if it's native or external (that concept would actually cease to exist then because functionally they'll be the same)

Right. My apologies if I wasn't clear before. The current multimethod design is fine for internal use, but I think we need to improve it a little if it's going to be a public API.

However what I don't understand yet is how you want to create project.clj of the resulting project? Up to this point Duct simply used project.clj file written in Stencil format. But now that Duct would have dynamically created list of dependencies, it wouldn't be an option anymore, no?

You might be overthinking this a little. We can have a project.clj template that looks like:

(defproject {{raw-name}} "0.1.0-SNAPSHOT"
  :dependencies [[org.clojure/clojure "1.10.0"]
                 [duct/core "0.7.0"]
                 [duct/module.logging "0.4.0"]
                 {{extra-deps}}]])

We'll need to tweak the indentation of extra-deps a little, or it'll look wonky, but otherwise it's just the same stencil template we're using before.

The key to this design is that we don't write any files until we've resolved data from all the profiles.

So consider a simplified base profile to go with our simplified project.clj template:

(defn base [{:keys [project-name]}]
  {:templates ["base/project.clj"]
   :vars {:raw-name project-name}})

And then consider two profiles, +foo and +bar:

(defn foo [_]
  {:extra-deps '[[foo "0.1.0"]]})

(defn bar [_]
  {:templates ["bar/example.clj"]
   :extra-deps '[[bar "0.1.0]]})

In order to render this project, we first find all of the profiles, then run the profile functions, and finally combine all of the data structures together:

{:templates ["base/project.clj" "bar/example.clj"]
 :vars {:raw-name "example"}
 :extra-deps '[[foo "0.1.0"] [bar "0.1.0"]]}

From there we generate the project files.

So we should know all of the dependencies before we write the project.clj file. There's no need to rewrite it with rewrite-clj, or to use cljfmt, or anything like that. We can just use normal Stencil templates with a bit of manual indenting when we generate the extra-deps string.

werenall commented 5 years ago

Thanks for your reply! Now it's way easier. Please review my recent changes. I realised that we don't need to write new method/function that'd combine the power of profile-data and profile-files. We can just use :deps as a key to the data and do everything as we were already doing. Also I don't group modules by "nativity" anymore. I just leverage the :default method of profile-data and profile-files multimethods. I tested it with +ataraxy +hydrogen.example and it works pretty amazingly. Even indentation of dependencies is impeccable. You can see one flaw when you use +ataraxy +cljs +hydrogen.example. Both ataraxy and cljs introduce [duct/module.web "0.7.0"] and my merging function is too dumb to handle that properly.

Unfortunately I don't have any more time to continue with that for now. Please let us know what you think and someone from our team will pick up where I left.

weavejester commented 5 years ago

Thanks for your work on this. The external plugins and project template are perfect, but I think some more work needs to be done on the profiles. I'm going to use your code as a basis and work on this over the weekend. I should have something ready by the end of Monday.

weavejester commented 5 years ago

I've taken your PR and adapted it, as I wanted a different API for writing template profiles, but the functionality should now be in Duct master. I want to test it out a little before releasing it, however.

To add in an external/remote profile, you need to create a package called: <namespace>/duct-template. It should have a namespace in it named: <namespace>.duct-template. Inside the namespace are functions named <hint>-profile. This will create a profile hint: +<namespace>/<hint>.

For example, if I had a package called weavejester/duct-template, and a function in it weavejester.duct-template/foo-profile, then I could use this as a profile hint called +weavejester/foo.

Take a look at the duct.duct-template namespace for how the default profiles are defined.

iarenaza commented 5 years ago

Thanks a lot James!

We'll have a look at the default profiles and create our own (first) profile in the following days. We'll tell you how it goes :smiley:

werenall commented 5 years ago

One thing bothers me in this design. As far as I understood, all external hints require format with a slash: +weavejester/example, +weavejester/cljs and so on. But that requires, that all hints are defined in one package, isn't it? I mean, to have +weavejester/example hint, it needs to be defined in weavejester.duct-template namespace. And that namespace has to live in weavejester/duct-template package. And analogical situation happens to +weavejester/cljs.

I think it would be nice to be able to have templates that share same organization but also have them in separate packages as well. For instance, I'd like to be able to have hydrogen.cljs/duct-template and hydrogen.datomic/duct-template packages. But then how do I define a profile? With current implementation I can't achieve +hydrogen/datomic because my classpath is already hydrogen.datomic.duct-template. Closest to my intention would be +hydrogen.datomic/datomic but that's redundant. Do you think that could be improved somehow?

werenall commented 5 years ago

Hmm... I'm afraid that :dirs attribute is completely ignored. I'll investigate and try to open a new PR by Friday.

weavejester commented 5 years ago

My intent around the current naming scheme is that it gives users a clear way of finding the associated package, while also allowing a single package to have multiple profiles. I also wanted to limit the number of packages queried and downloaded, to keep the template feeling reasonably quick.

Could you explain a little more about your reasoning behind splitting up +hydrogen/cljs and +hydrogen/datomic into separate packages?

werenall commented 5 years ago

Sure! Our concern is that some of the templates we want to write are going to be open-sourced. However some others (like Datomic) are meant to be part of a "Pro" service. They will be hosted in different maven repo.

weavejester commented 5 years ago

That's a pretty valid reason! I'll investigate solutions for this, and I should have something by the weekend. Possibly the easiest way is to search packages hydrogen/duct-template and hydrogen.cljs/duct-template. I don't like the idea of increasing the latency, but I also don't like the idea of requiring a package for each profile in situations where it's not necessary.

weavejester commented 5 years ago

I've been looking into this, and one thought I've had is that in Clojars at least, someone can create a foo.bar/baz package without needing to own foo. Given that, should the package names be hydrogen.cljs/duct-template, or hydrogen/cljs.duct-template or hydrogen/cljs-duct-template or something else entirely?