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 tags lib with ClojureScript support #98

Closed asmala closed 10 years ago

asmala commented 10 years ago

As per the discussion on #75, I'm submitting a pull request that splits hiccup into a tag generation library and a compiler library under one repo, and adds ClojureScript support for the former.

The API should be identical to v1.0.5, with a couple of moves and additions resulting from the split:

I did not change the version number yet, but I did otherwise update the project.clj files, the README, and the Travis config.

asmala commented 10 years ago

The failing Travis build is due to the two sub projects not being published on Clojars, it seems.

weavejester commented 10 years ago

Thanks for the PR!

The incompatibilities you've introduced will need to be reverted before this PR can be merged, I'm afraid. Maintaining backward compatibility of APIs is important, and none of the changes you've introduced seem major enough to warrant breaking that. I'd rather release these changes as Hiccup 1.1 than jump all the way to a 2.0 release for no real reason.

The commits will also need to be tidied up before this can be merged. Commits like 184d467 and 03339e8 can be squashed, and bd20445 could be better accomplished through a rebase. Also, try to avoid ending periods in the commit summary messages, and fix the spelling error in 2d4807c.

Regarding the code, remove the .gitignore files in the subdirectories, instead use the one at the top level to add any additional rules. Add a :scm {:dir ".."} option in the subprojects so that SCM information is added to the pom.xml files they generate. Prefer using lein sub for travis.

asmala commented 10 years ago

I hear ya. I'm not quite sure how to cleanly avoid the namespace reorganization, though. Looking at each of the changes in turn:

  1. hiccup.core/h does seem to fit more neatly in hiccup.util, but given the requirement for backwards compatibility, how about we have the alias in both hiccup.core and hiccup.util?
  2. I moved hiccup.def/defhtml to hiccup.core since it depends on the compiler, whereas hiccup.def/defelem does not. You suggested earlier having an unbound dynamic var called e.g. *compiler-fn*, that would get called by defelem, but it seems a bit unclean given potentially different compiler-fn footprints and potential issues in multi-threaded applications. Do you still want to go with the dynamic var approach and then leave it up to the user to evaluate the downsides accordingly?
  3. I see a need for both hiccup emitting and string emitting versions of html4, xhtml, etc., so how about I leave the string emitting versions in the hiccup.page namespace and the hiccup emitting ones in e.g. hiccup.html? That way we'll have both backwards compatibility and added flexibility of using the fns in another context (e.g. sablono and React).

I had some issues getting lein sub working with the tests so I did the bash version as a temporary workaround, but let me see if I could get that sorted before a merge. I'll also try to clean up the commit tree; should be good practice for my git fu. But let me know what you think about the items above.

weavejester commented 10 years ago

Regarding hiccup.core/h, that was initially just an alias to make it easier to use hiccup.util/escape-html. I'd rather not have it in two namespaces, so let's keep it in hiccup.core for now.

With respect to defhtml and html, I'd prefer them to be placed in hiccup-common rather than hiccup-compiler. I was thinking to make html macro behave something like:

;; Old behavior. Tries to resolve hiccup.compiler/compile-html.
;; Throws an error if missing.
(hiccup/html [:span "hello world"])

;; Explicitly set the compiler globally (uses alter-var-root behind the scenes)
(hiccup/set-compiler! hiccup.compiler/compile-html)
(hiccup/html [:span "hello world"])

;; Set the compiler via an option:
(hiccup/html {:compiler hiccup.compiler/compile-html} [:span "hello world"]))

;; Return the current compiler:
hiccup/*compiler*

We might as well make it dynamic, though because html is a macro, it won't be hugely useful to place it in a binding.

I don't hugely like the idea of a global setting, but it's probably going to be a pretty common use-case.

I'd like to revisit some of these decisions for Hiccup 2.0, but for now we should maintain backward compatibility.

Regarding html4, html5 and xhtml, let's keep them how they are for now, and deal with them in another pull request.

asmala commented 10 years ago

Roger that. I'll clean up the branch and ping you. To keep the changes easier to track, I might end up submitting two separate pull requests; one to split the lib in two and another to add ClojureScript support.

asmala commented 10 years ago

I'm closing this in favor of #99.