plumatic / dommy

A tiny ClojureScript DOM manipulation and event library
758 stars 74 forks source link

doesn't recognize identifiers with slashes #62

Closed trevor closed 10 years ago

trevor commented 10 years ago

id's or classes that have a / character aren't processed. (This is usually caused in clojure by accidentally using name as a generic keyword-to-string function.)

Note this is a legal character in html5: http://www.w3.org/html/wg/drafts/html/master/dom.html#the-id-attribute

I'm assuming not all characters can easily be supported, but perhaps this is an easy fix.

A solution would be something a function like (subs (str :abc) 1), but there may be something better.

cpetzold commented 10 years ago

Thanks for the report :)

rm-hull commented 10 years ago

@cpetzold - the https://github.com/Prismatic/dommy/commit/1a8445f946886d0097ffe39d299cce041187ebf6 fix you put in on Monday, changing name to string-or-keyword breaks things badly when I use the node macro, even though all the tests pass.

I'm getting stack traces like:

clojure.lang.ExceptionInfo: failed compiling file:
-----8<-----
blah blah, snipped
-----8<----- 
Caused by: java.lang.ClassCastException: clojure.lang.Symbol cannot be cast to java.lang.CharSequence
                core.clj:4386 clojure.core/re-matcher
                core.clj:4438 clojure.core/re-find
               macros.clj:122 dommy.macros/parse-keyword
               macros.clj:134 dommy.macros/compile-compound
               ...

Aliasing

(def string-or-keyword name)

On my local install, and it works, ... why the change?

trevor commented 10 years ago

@rm-hull I can't speak to the stack trace issue, but as for "why the change?" see this ticket. name isn't a suitable general function for stringification.

rm-hull commented 10 years ago

Hmmm... Well this solves it, I suppose:

(defn string-or-keyword [s]
  (if (keyword? s)
    (-> (str s) (subs 1))
    (str s))) 

by wrapping the s when it's not a keyword (it is a symbol, after all); I guess it should be called string-or-symbol-or-keyword or something to that effect!

trevor commented 10 years ago

@rm-hull I think that would work.

It's a shame this isn't standardized in core somewhere to be idiomatic.

rm-hull commented 10 years ago

Quick update on this: I didn't really fully consider the consequence of the exception message: Caused by: java.lang.ClassCastException: clojure.lang.Symbol cannot be cast to java.lang.CharSequence uintil just now...

Hunting through the code there was a single snippet as follows:

[div#id ...]

i.e. it was missing the starting colon, and was being treated as a symbol - this had worked previously when the code was using name as that would happily consume symbols. With the code change in https://github.com/Prismatic/dommy/commit/1a8445f946886d0097ffe39d299cce041187ebf6 this no longer worked.

Clearly my code was wrong, but is it worth fixing this up as per my previous comment? Happy to raise a PR for it.

cpetzold commented 10 years ago

Ah funny, didn't realize the template macros worked with symbol tags. I think it's a bit dangerous for this to work, because switching from the macro to run-time template fns would break in this case.

Maybe an assertion that verifies that the tag is a keyword? I'm unsure about allowing string tags, as we may want to treat string vectors as text node groupings instead.