Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build-all breaks in simple/advanced compilation #147

Closed
totakke opened this issue Feb 1, 2017 · 3 comments · Fixed by #149
Closed

build-all breaks in simple/advanced compilation #147

totakke opened this issue Feb 1, 2017 · 3 comments · Fixed by #149

Comments

@totakke
Copy link
Contributor

totakke commented Feb 1, 2017

A combination of sablono and om/build-all is not working in simple/advanced compilation.

Example code:

(def app-state (atom {:list [1 2 3]}))

(defn row [n owner]
  (reify
    om/IRender
    (render [this]
      (html [:li (str n ": " (om/get-shared owner :message))]))))

(defn component1 [data owner]
  (reify
    om/IRender
    (render [this]
      (dom/div nil
        (apply dom/ul nil
          (om/build-all row (:list data)))))))

(defn component2 [data owner]
  (reify
    om/IRender
    (render [this]
      (html [:div
             [:ul (om/build-all row (:list data))]]))))

(om/root component1 ; component2
         app-state
         {:target (. js/document (getElementById "app"))
          :shared {:message "Hello, world!"}})

component1 using om.dom is working. However, component2 using sablono doesn't display "Hello, world" message in the :shared map.

This problem seems to be similar in #119. Putting doall in the front of om/build-all is a workaround. I think sablono should realize components correctly because it is hard to notice this problem. This doesn't occur in :optimizations :none...

@r0man
Copy link
Owner

r0man commented Feb 1, 2017

@totakke Patch with tests welcome! ;)

@totakke
Copy link
Contributor Author

totakke commented Feb 2, 2017

@r0man Thank you for comment!

This problem caused by lazy seq and binding was once solved by 426ef6c.

(defn- interpret-seq
  "Interpret the seq `x` as HTML elements."
  [x]
  (into [] (map interpret) x))

However, It recurred by 17ec99d, making interpret-seq lazy. (for performance?)

 (defn- interpret-seq
   "Interpret the seq `x` as HTML elements."
   [x]
-  (into [] (map interpret) x))
+  (map interpret x))

Fixing this by reverting interpret-seq to non-lazy is easy (testing is a little difficult). Is lazy interpret-seq important?

@r0man
Copy link
Owner

r0man commented Feb 2, 2017

@totakke No, it's not important. Go for it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants