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

Separate tag and compiler libs #99

Closed asmala closed 10 years ago

asmala commented 10 years ago

This pull request separates hiccup into two libs, using lein sub. hiccup-common will contain all tag generation functionality, while hiccup-compiler contains the hiccup Clojure compiler. This should be fully backwards compatible since the compiler fn in hiccup.core can be redefined but defaults to hiccup.compiler/compile-html.

I'll be submitting a separate pull request for ClojureScript support once we've cleared this merge. Should make the change more manageable.

asmala commented 10 years ago

The Travis build error is a result of Clojars not having the hiccup/hiccup-common JAR installed.

weavejester commented 10 years ago

Rather than resolving the compiler at definition, it might be better to leave it as nil, and then have a resolve-compiler function like:

(defn resolve-compiler []
  (or *compiler* (resolve 'hiccup.compiler/compile-html)))

This would allow users to more accurately determine the current compiler, and shouldn't affect performance as html is a macro, and therefore the compiler is always resolved at compile-time.

asmala commented 10 years ago

Can do. Here you go. I also added some tests to cover the *compiler* use.

I do think the setup is a bit brittle but works well enough for backwards compatibility. To illustrate potential issues, the following for example would not work:

;; Since (html …) is executed at macro expansion time, the binding has no effect
(binding [*compiler* my-compiler-fn]
  (html [:h1] [:p]))
weavejester commented 10 years ago

Yep, that's why I'm somewhat hesitant about making the compiler a dynamic var. On the other hand, we have vars like *clojure-version* that are set at compile time, and obviously not meant to be used in a binding.

weavejester commented 10 years ago

A couple more points:

  1. I think we should use a base class of Error, rather than Exception, when the compiler cannot be found.
  2. The html macro should take a :compiler option to allow the compiler to be set locally.
asmala commented 10 years ago

Agreed. Here's an idea; why don't we do away with the dynamic var altogether and only use the :compiler option? That would be a robust solution, with a negligible impact on the portability of code involving calls to html and defhtml.

weavejester commented 10 years ago

I've just thought of another problem: ClojureScript cannot require dynamically, to my knowledge, so I don't know if we can have a pointer to a default compiler without adding the compiler as an explicit dependency.

Really, this whole problem seems to be getting more and more complicated. I'm beginning to think that this cannot be done in a clean way without breaking the Hiccup API. If we need to hop up to Hiccup 2.0, I'd rather go in feet first and make some substantial improvements.

My current inclination is to forgo the compiler entirely. It's a huge pain to deal with, and there's no real evidence it's actually useful. Is template compilation ever the bottleneck in a web app? If needs be, we could add pre-compilation macros later, but I don't think the core of the library should revolve around such an inflexible basis.

If the html macro were actually a function, we could develop a cross-platform Hiccup, with functionality like automatically escaping strings. That seems far more valuable to me than the compiler system we have currently.

Would you be interested in collaborating on such a design? I'd be interested in hearing your thoughts.

asmala commented 10 years ago

I think we can get around issues with ClojureScript by some creative use of cljx, without too much complexity. That being said, I agree with the sentiment that a clean solution might require breaking the API.

I'd be happy to collaborate on a 2.0 design. My only concern is that an actual release might be some ways off and would hence delay development of cross-platform libraries built on top of hiccup. With this concern in mind, I suggest the following:

  1. Create a new project for tag generation only, e.g. hiccup-tags, as follows:
    • Move hiccup.util, hiccup.element, and hiccup.form there
    • Create a new namespace with a copy of defelem there (this way we can still maintain defhtml and defelem in the same namespace in the original hiccup)
    • Create a new version of hiccup.page (with a new name) that spits out hiccup data structures instead of strings
  2. Include this project in the current hiccup, in a backwards-compatible 1.1 release
  3. Port the new project to ClojureScript
  4. Proceed with 2.0 design

This way we could push out the breaking changes to a later point in time and still get out something reusable and cross-platform.

What do you think?

weavejester commented 10 years ago

My feeling is that we're jumping through a lot of hoops to maintain a measure of backward compatibility, when we're 90% of the way toward a 2.0 release anyway. Although a 2.0 release seems like a big deal, it's actually not very different from the code you have already.

My thought was that a 2.0 release would contain:

  1. Replace the html macro with a function
  2. Make the library cross-platform between Clojure and ClojureScript
  3. Automatically escape raw strings

You're already completed most of point 2, correct? And 1 and 3 don't seem particularly complex. Compared to rearranging the current library, doesn't a 2.0 release sound like less work overall?

asmala commented 10 years ago

Gotcha. Yes, that does seem quite doable.

I would still vote for splitting the library, since we're dealing with two very distinct things; the tag generation fns and the compiler, that only need to share a well-defined data structure format. If I look at projects like crate and sablono, they use practically the same tag generation code as hiccup but completely different compilers. I don't think there's even a need to port the hiccup compiler to ClojureScript, except perhaps for use in Node.js.

With this in mind, I suggest the following amended agenda for 2.0:

  1. Split library into a tag generation library and a hiccup-to-string compiler
    • All compiler dependent code, including html, defhtml, and hiccup.page, to be moved to the compiler library, with possible changes in namespaces
  2. Replace the html macro with a function
    • Consider adding a compile-time version of html later
  3. Make tag generation library cross-platform between Clojure and ClojureScript
  4. Automatically escape raw strings in the compiler

If this makes sense to you, we could create a hiccup organization on GitHub with two repos under it; one for the tags and another for the compiler. This way we could split the work more readily between the two of us and create space for future hiccup-based projects (e.g. compiler-utils or hiccup-bootstrap). What do you think?

weavejester commented 10 years ago

I think I agree, but give me a day or so to think it over.

I'd also like to change hiccup.page to have functions that return data structures, rather than strings, so we can move that out of the compiler.

We also need to figure out a good syntax for raw strings, perhaps using a data reader.

asmala commented 10 years ago

Cool, keep me posted.

I'm with you on the hiccup.page change. That's not to say we couldn't do both; have a version that returns data structures in the tag generation lib and a version that returns string in the compiler lib.

I've been thinking about the raw string conundrum as well. If you mean using reader literals, e.g. #hiccup/raw "Comment with <strong>HTML</strong>", that would only work if we use a custom data reader and read the contents e.g. from an EDN file. Not a use case I had in mind, but let me know if I'm missing something?

Another alternative could be using meta, e.g. ^:raw-html "Comment with <strong>HTML</strong>" looks fairly readable too. Not sure if there's a risk of metadata getting lost as data gets passed around, though.

weavejester commented 10 years ago

You can actually provide data readers direct to the Clojure compiler by supplying a data_readers.clj file in the root of your classpath. There are a few libraries that use this, notably the Datomic peer library, ordered, and two of my libraries, euclidean and crumpets.

Using metadata, on the other hand, won't work because you can only add metadata to Clojure data structures (or anything that implements IMeta). Unfortunately Strings aren't included, and because the String class is final, Clojure can't extend the String class with a more Clojure-friendly version.

asmala commented 10 years ago

Neat! Live and learn. In that case, reader literals are the way to go.

asmala commented 10 years ago

I've been thinking about the design further and it seems like there's actually three distinct phases to the hiccup pipeline; generate, preprocess, and emit. Thinking about the library in these three phases might be helpful as we think about how to organize it.

hiccup flow

  1. Generate creates hiccup compatible data structures
    • [:div#main.content [:h1 {:style {:font-weight "bold"}} "Welcome!"]]
    • Currently: Functions in hiccup.element and hiccup.form
    • In the future: A whole ecosystem of tag generation libraries, like giddyup
  2. Preprocess applies various pre-compilation transformations and outputs uniform hiccup data
    • [:div {:id "main" :class "content"} [:h1 {:style "font-weight: bold;"} "Welcome!"]]
    • Currently: hiccup.compiler/normalize-element
    • In the future: A toolbox of libraries for various pre-compilation needs, such as string escaping, style attribute compilation, or HTML-to-DOM attribute conversion
  3. Emit converts the standardized hiccup data to the target format
    • "<div id=\"main\" class=\"content\"><h1 style=\"font-weight: bold;\">Welcome!</h1></div>"
    • Currently: hiccup.compiler, sablono.compiler, crate.compiler (not integrated)
    • In the future: Targeted compilers all sharing the same generation and preprocessing libraries

It may be that for performance reasons phase 2 will happen together with phase 3, but from a library architecture perspective, it's probably a good idea to keep them separate to help with reusability.

asmala commented 10 years ago

I've got a functional version of the tag generation library waiting on my local. Let me know what you think about the hiccup GitHub org idea.

weavejester commented 10 years ago

Apologies for the delayed response; I was finishing up some contracting work.

A GitHub org seems like a reasonable idea. It would provide a place to collect various tag-generating libraries. I'll create a suitable skeleton, and pull you in as a contributor.

weavejester commented 10 years ago

On the other hand, if we place the hiccup/hiccup-core and hiccup/hiccup-common subprojects in the same repository, we can have a top-level hiccup project that depends on both.

That seems like the best approach for Hiccup 2.0, as people can still include a top level [hiccup "2.0.0"] dependency and expect to get a similar set of functionality to Hiccup 1.0.

This is the approach taken by Ring, and works pretty well.

weavejester commented 10 years ago

I've added you as a collaborator to the project, and I've created a clean 2.0 branch.

asmala commented 10 years ago

Okay, cool. I'll move the tag generation functions to a hiccup-common directory, set that directory up as a project, and push the results to the 2.0 branch. I'll leave the organization of the top level project and the compiler sub to you. Sound good?

Here's what I'm thinking for each of the namespaces:

Let me know if you have any concerns with the above, otherwise I'll get to work on this tomorrow.

weavejester commented 10 years ago

That seems okay for now. Perhaps defelem would be better suited for hiccup-core, but I'm undecided.

asmala commented 10 years ago

I'd definitely stick defelm in hiccup-common, since it's hugely useful for (compiler-independent) tag generation libraries. defhtml, on the other hand, is closely intertwined with the compiler, and would thus be a better fit for hiccup-compiler.

Anyway, I'll get to work on the tag generation sub tomorrow along the lines of our conversation above.

asmala commented 10 years ago

I moved the tag generation related namespaces to hiccup-common, as discussed above, and pushed the results to the 2.0 branch. I only did minimal updates to the top-level project to accommodate for the move (updated .gitignore for hiccup-common/target and project.clj for lein-sub config).

I'll leave the compiler rehash to you. That still leaves us with hiccup.middleware, though. We could leave it within the top-level project, move it to hiccup-common, or create a third sub for it.

I'm closing this ticket for now, since we've strayed quite far from the original topic. :-)