weavejester / hiccup

Fast library for rendering HTML in Clojure
http://weavejester.github.io/hiccup
Eclipse Public License 1.0
2.68k stars 174 forks source link

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

Closed davesann closed 10 years ago

davesann commented 11 years ago

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 commented 10 years ago

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

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

@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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

@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 commented 10 years ago

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

weavejester commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

asmala commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

asmala commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

asmala commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

asmala commented 10 years ago

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