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

Enquiry: Separate out hiccup rendering from helper functions ?? #75

Closed
davesann opened this issue Feb 7, 2013 · 30 comments
Closed

Enquiry: Separate out hiccup rendering from helper functions ?? #75

davesann opened this issue Feb 7, 2013 · 30 comments

Comments

@davesann
Copy link

davesann commented Feb 7, 2013

This is not so much an issue as an enquiry.

I use both hiccup and crate and now dommy and it occured to me that hiccup is doing two distinct functions today. (and Crate replicates some this).

  1. rendering hiccup data structures
  2. providing a library of functions to build hiccup data structures.

Do you think it might be worth considering splitting the two into two independent libraries?

  1. A rendering library
  2. A set of generic clojure library functions as a base to be portable across multiple clojure implementations (cljs and maybe clr)
@asmala
Copy link
Collaborator

asmala commented Jan 9, 2014

I wholeheartedly agree. I envision the hiccup ecosystem to look something like this:

  • Core data structure generation library, called e.g. hiccup-gen
  • Compilation libraries for different targets
  • Data structure libraries for HTML frameworks

For purposes of legacy support and convenience, hiccup would continue to exist but would merely pull in hiccup-gen and hiccup-compiler, and define hiccup.middleware.

@weavejester, what do you think? I could take a stab at refactoring the current repo into two sub-projects (using lein-sub) and wire in ClojureScript compilation, with crossover support or cljx.

@weavejester
Copy link
Owner

This seems a reasonable suggestion. Somehow the original issue got lost in my inbox, or never arrived.

Not sure I'm a fan of the name "hiccup-gen" though!

@asmala
Copy link
Collaborator

asmala commented Jan 10, 2014

How about hiccup-tags or hiccup-elems?

Aside from that though, any concerns or comments on the implementation suggestion above? If not, I'll get to work refctoring the repo on a feature branch and will submit a pull request later.

@asmala
Copy link
Collaborator

asmala commented Jan 12, 2014

@weavejester, could you take a peek at this branch I'm working on to see if this is going in a direction you like?

I pulled out all the tag generation into a sub-project. I had to shuffle around some fns and rewrite some of the test, but the original functionality is essentially intact, with all tests both in the sub-project and the main project (after lein install on the sub) passing. The place where things are most shaky at the moment is hiccup.page in the sub-project and hiccup.html in the main one, as I decided to separate the HTML page tag generation from the HTML page compilation macros.

Let me know what you think.

@weavejester
Copy link
Owner

I think I'd be inclined to have two sub-projects, hiccup-common and hiccup-compiler, with the top-level hiccup project pulling both in as a dependency, in the same way Ring works.

@asmala
Copy link
Collaborator

asmala commented Jan 12, 2014

Yup, thinking the same thing. So far I've separated out hiccup-common (currently called hiccup-tags) to its own project but will give hiccup-compiler the same treatment soon and add lein-sub config to the root project.clj. Other than that, any other thoughts on the code, esp. the aforementioned tricky bits?

Also, I'm tempted to give hiccup.middleware its own project as well since not everyone will be running on Ring (think e.g. Pedestal or other ClojureScript apps), but not sure if that's overkill for a single simple function. Probably will just end up putting that with hiccup-common. What do you think?

@asmala
Copy link
Collaborator

asmala commented Jan 14, 2014

@weavejester, I've now separated hiccup-compiler and hiccup-common, complete with lein-sub support and updated README and Travis config. The next step would be to add cross-compilation support (Clojure and ClojureScript) to hiccup-common using either cljx or crossovers.

Before I proceed, would you like to take a look at the code so far? The two areas that could definitely use some feedback are hiccup.page (under hiccup-common) and hiccup.html (under hiccup-compiler). It would also be great to run the code through a performance benchmark to make sure that the refactoring hasn't accidentally caused a significant performance hit.

@asmala
Copy link
Collaborator

asmala commented Jan 20, 2014

Okay, the cljs port of hiccup-common is now working. Thoughts welcome before I clean it up and send a pull request. ;-)

@weavejester
Copy link
Owner

You might want to look at the new ClojureScript syntax for including macros:

(:require [hiccup.def :refer :all :include-macros true])

This would allow you to remove the hiccup.def-macros namespace in hiccup-common.

I also wonder whether it wouldn't be better to restrict the the hiccup-compiler to be just the compiler.

@asmala
Copy link
Collaborator

asmala commented Jan 20, 2014

Oh, thanks. I wasn't aware of :include-macros.

In this particular case, however, I have to write two different implementations of the defelem macro, since the Clojure version (in hiccup.def) uses vars for updating ^:arglists and Clojurescript version (in hiccup.def-macros) doesn't since Clojurescript doesn't have vars. I find it a rather clumsy implementation but not sure how to merge those two definitions… Thoughts?

As for hiccup-compiler being just a compiler, where do you suggest putting the html, html5, etc. macros? They all require the compiler. The rest is already under hiccup-common.

@weavejester
Copy link
Owner

Can't the defelem macro simply add a check at runtime to determine the capabilities of the environment its executing in?

Regarding the hiccup-compiler, why not have a dynamic var that contains the compiler? For example:

(set! hiccup.core/*compiler* hiccup.clojure/compiler)

Then the html macro could use the compiler specified. One could even make it a little easier by checking for a default compiler, or set of default compilers.

@asmala
Copy link
Collaborator

asmala commented Jan 26, 2014

I've been trying to get this to work and I have to admit my macro-fu is failing me. I could theoretically use ns-resolve to check for alter-var-root and alter-meta! but ClojureScript doesn't have ns-resolve. And even if it would, I'm not sure how to check for support for var which is a special form, not a function. Any pointers?

Edit: alter-meta! appears to exist but is not of much use here since ClojureScript doesn't have vars.

@weavejester
Copy link
Owner

Let's divide this up into two problems. The first is how to add the wrap-attrs functionality to the function. Ideally we'd like a solution that works in both Clojure and ClojureScript.

There's a function called name-with-attributes in the tools.macro library that simplifies the process of constructing forms that look like defn. This will allow us to get rid of the alter-var-root code and the equivalent in ClojureScript.

(defmacro defelem [name & fdecl]
  (let [[name fdecl] (name-with-attributes name fdecl)]
    `(def ~name (wrap-attrs (fn ~@fdecl)))))

Next, we want a mechanism to alter the arglist metadata. We could do that by parsing fdecl, but since ClojureScript doesn't have vars, it only matters in Clojure. So instead, consider a function like:

(defn ^:no-doc update-elem-arglists! [var-name]
  #+clj (alter-meta! (ns-resolve *ns* var-name) update-in [:arglists] update-arglists))

In Clojure, we update the argument list, but in ClojureScript, the function does nothing.

Putting it all together:

(defmacro defelem [name & fdecl]
  (let [[name fdecl] (name-with-attributes name fdecl)]
    `(do (def ~name (wrap-attrs (fn ~@fdecl)))
         (update-elem-arglists! ~name))))

@asmala
Copy link
Collaborator

asmala commented Jan 28, 2014

Thanks, I'll give that a go later this week.

@asmala
Copy link
Collaborator

asmala commented Jan 28, 2014

Hmm, this approach still won't work since ^:arglists is only added when a function is created by defn. With the current approach name-with-attributes returns meta that doesn't include ^:arglists.

@weavejester
Copy link
Owner

Oh, that's pretty annoying. I can't think of a clean solution, barring using a variant on your def/gensym idea, which seems a little problematic in Clojure, or copying the private sigs function out of clojure.core.

Let's shelve this for now. It's not something important enough to stop a merge, just something that needs to be sorted out eventually.

@asmala
Copy link
Collaborator

asmala commented Jan 28, 2014

Yeah… I'll mark it with a "TODO" for someone smarter than myself to figure out.

Do you see any objections to the current codebase or should I go about doing some clean-up, merging in a master, and sending a pull request?

@weavejester
Copy link
Owner

Aside from factoring out the hiccup.core/html macro from the compiler project?

@asmala
Copy link
Collaborator

asmala commented Jan 28, 2014

Oh, right, meant to comment on that. Is there a reason why you'd like to have the HTML string generation macros (currently in hiccup.html) out from hiccup-compiler? It wouldn't be too hard to move it to hiccup-common and refactor it e.g. using the dynamic var approach you proposed, but they seem more compiler related than tag generation related.

@weavejester
Copy link
Owner

That's a reasonable point. My reasoning was that they don't add much additional code, but provide a common function for transforming Hiccup syntax into something else. I don't know of any Hiccup-inspired library that doesn't have some equivalent to html, whether it's a macro generating strings or a function generating DOM nodes. Alternatively, we could have hiccup-common, hiccup-core and hiccup-clojure-compiler.

That said, we don't need to do it all in one pull request. The current work can be merged in, or at least merged into a branch.

@asmala
Copy link
Collaborator

asmala commented Jan 29, 2014

I like the idea of separating the default compiler from some of the shared compiler functionality like tag normalization. Let me play with that a bit and see what makes sense.

Regarding the html macros, while most hiccup family compilers may provide those, I don't think it's safe to assume they have the same API. Sablono, for example, doesn't have SGML and XML modes since it emits Om elements, not HTML strings. Perhaps we can leave it under the compiler sub?

@weavejester
Copy link
Owner

I didn't mean separate out any of the internals of the compiler, just the public interface.

The html macro has an mechanism for setting the rendering mode, but this is entirely optional. Different platforms might have different sets of options, but the interface is broadly the same:

(html & hiccup-forms)
(html option-map & hiccup-forms)

To my mind that's more than generic enough to factor out as a common interface.

@asmala
Copy link
Collaborator

asmala commented Jan 29, 2014

Fair enough. The macros don't do much, though. They only wrap the fns defined under hiccup.page with a call to html and pass the appropriate compiler opts. I can move them to hiccup-common, if you'd prefer it that way.

@asmala
Copy link
Collaborator

asmala commented Jan 30, 2014

Oh, I think I misunderstood what you meant by the macro refactoring. Did you mean to rewrite the hiccup.core/html macro to call hiccup.core/*compiler-fn* instead of the current hiccup.compiler/compile-html? Then we could plug in any other hiccup structure compiler into the system by e.g. (set! hiccup.core/*compiler-fn* sablono/html). Did I get that right?

@weavejester
Copy link
Owner

Right, that's pretty much exactly what I meant.

@asmala
Copy link
Collaborator

asmala commented Jan 30, 2014

I've been playing with the compiler-fn redefinition and it feels rather brittle, since html is a macro and some of the calls to compiler-fn happen at compile time. For example, what would happen if two namespaces independently set *hiccup.core/compiler-fn* to two different fns at compile time?

For the sake of simplicity, I would opt for letting each compiler lib define their own html macro, particularly as not all libs have the same API footprint for html. sablono.compiler/compile-html, for example, does not take options.

@asmala
Copy link
Collaborator

asmala commented Apr 3, 2014

I managed to solve the defelem conundrum by checking for *clojure-version* at runtime to determine which branch to run. So we now have a cross-compiling tag generation lib called hiccup-common and separate Clojure only compiler library called hiccup-compiler, both within a parent project called hiccup. All tests pass and the code's reasonably tidy. Would you like to take a look before I send over a pull request?

@weavejester
Copy link
Owner

That works, then? Interesting; I had assumed the *clojure-version* would be whatever Clojure version was doing the compiling to ClojureScript.

Apologies for the delayed response. I've have a lot on my plate recently.

@asmala
Copy link
Collaborator

asmala commented Apr 7, 2014

Suggesting we close this thread in favor of pull request #99.

@asmala
Copy link
Collaborator

asmala commented May 1, 2014

Closing this since the upcoming 2.0 release will address the issue.

@asmala asmala closed this as completed May 1, 2014
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

No branches or pull requests

3 participants