ckirkendall / enfocus

DOM manipulation and templating library for ClojureScript inspired by Enlive.
http://ckirkendall.github.com/enfocus-site/
371 stars 41 forks source link

replace-vars broken in clojurescript 1.7.145 #104

Closed nickweeds closed 8 years ago

nickweeds commented 9 years ago

On updating my project to use enfocus with clojurecript 1.7.145 I found that all variables were replaced with the text "null".

I believe that this is because clojurescript 1.7.145 has a non-back-compatible change to the string/replace function. This change is described by clojurescript issue CLJS-1304.

See http://dev.clojure.org/jira/browse/CLJS-1304 for the issue description.

See https://github.com/clojure/clojurescript/blob/master/changes.md for the clojurescript change history (including CLJS-1304).

See https://github.com/clojure/clojurescript/commit/3e146905999f9b9ffb249e139e266f845c4af34c for the string/replace change.

The replace-vars function uses string/replace with a replacement function. The problem can probably be fixed by changing the "%2" in the replacement function to "(%1 1)".

Users will need to be warned which enfocus versions work with which clojurescript versions.

ckirkendall commented 9 years ago

Thanks for figuring this out. I think with a little magic around clojurescript-version we might be able to fix the issue in a way that works for everyone.

nickweeds commented 9 years ago

Something like the following might also work for all ClojureScript versions:

  (defn replace-vars
    "replaces entries like the following ${var1} in attributes and text"
    [vars]
    (letfn [(match-group [args n]
              (if (vector? (args 0))
                  ((args 0) n) ; clojurescript >= 1.7.145
                  (args n)))   ; clojurescript < 1.7.145
            (rep-str [text]
              (string/replace text #"\$\{\s*(\S+)\s*}"
                (fn [& args] (vars (keyword (match-group args 1))))))]
  ...
ckirkendall commented 9 years ago

@nickweeds do you mind pulling down the latest and testing it out by doing a 'lein install'. I pushed a fix.

nickweeds commented 9 years ago

Many thanks for applying a fix so quickly. I am keen to try it out, but I'm afraid I won't get an opportunity to do so for 1-2 weeks (my clojurescript project is at work and I won't be back in the office until then).

nickweeds commented 9 years ago

Thanks for making the fix. I've tested enfocus 2.1.2-SNAPSHOT (containing the fix) with both old and new versions of clojurescript, and it works with both.

Specifically, I tested enfocus 2.1.2-SNAPSHOT with clojurescript versions 1.7.107, 1.7.122, and 1.7.145, and all worked OK. By comparison enfocus 2.1.1 worked with clojurescript 1.7.107 but not with clojursecript 1.7.122 or 1.7.145. Based on those results I think the non-back-compatible change to string/replace must have been introduced in clojurescript 1.7.122 (not mentioned in the change history).

nickweeds commented 8 years ago

I think this issue can be closed now.

Would you push a fix release to clojars so that developers can easily use Enfocus replace-vars with ClojuresScript 1.7.145 and above ?