Why identical? instead of = for value comparisons?
Opened this issue · 7 comments
I've run into times when an rx expression is updated even though it's value (and the value of it's dependencies) are the same, resulting in brief flashes because the corresponding piece of DOM re-renders. Looking at the code, I'm guessing this is because identical? is used instead of = when determining whether to notify watches. Is there a reason for this? If it's for performance, could it be made configurable?
Hi Dave, so yes, identical? is used for performance reasons as it maps to Javascript's ===. = in Clojurescript could invoke the -equiv protocol method as I recall and would affect performance, possibly substantially in many cases. Note that Clojure(script)'s atom does no equality check at all. Ideally, the system would be tolerant to occasionally getting spurious change notifications - the DOM actions should be idempotent. Maybe the reason why you're seeing flashing is because I have diffing turned off in the develop branch? We could make it configurable but that might make the library less simple to use. An acceptable solution might be to change this line to use =: https://github.com/aaronc/freactive/blob/develop/src-cljs/freactive/dom.cljs#L670. This would prevent spurious DOM node changes/diffing and allow attribute bindings (what's actually important for fast animation) to use the fast path of the identical? check. Your thoughts?
As often happens, filing an issue got me thinking more deeply about my problem. We're making HTML custom elements based on freactive and Datascript. I had built some lenses to abstract Datascript and provide an entity map based on entity IDs. When the Datascript instance changed due to user input, the entity cursor sent a notification even though the entity itself hadn't changed, and that propagated to cause the spurious re-render. By restructuring how the model data propagates to the custom elements, I was able to solve the problem. And the code is clearer now anyway, has better decoupling, etc.
The other case I'm using is lenses based on Datascript queries, where it would be nice to do a set equality to see if the result actually changed. But really, it's a special case, and probably deserves a custom implementation of the relevant protocols rather than expecting the generic implementation to handle it. So I'm going to agree that the default implementation uses identical? for fast attribute binding updates, and will start investigating custom implementations for more complex scenarios.
I do think the DOM equality check you suggested might be handy. I have run into cases where I use something like condp inside of rx to choose between different DOM subtrees, and sometimes it takes extra work to avoid the re-evaluation of the rx causing a re-render.
I changed the DOM equality check to use = here: 65d26c1
Let me know how this affects things on your end.
Also, FYI in the HEAD of the develop branch there is a completely new cursor (and atom) implementation - should be much more powerful and works in my test, but if you get a chance to try please let me know if you run into any issues.
Not sure this is working as you planned. Here's the code I used to test:
(ns freactive-sandbox.rx-test
(:require-macros [freactive.macros :refer [rx debug-rx]])
(:require [freactive.core :refer [atom cursor]]
[freactive.dom :as dom]))
(enable-console-print!)
(def state (atom {:a 1 :b {:c 2 :d 3}}))
(def b (cursor state :b))
(defn view
[]
(let [a (cursor state :a)
cd (cursor b #(* (:c %) (:d %)) #())]
(rx
(println "A" @a)
[:div (str @a)
[:div
#_(println "BBBBB" @b)
[:div (rx (str @b))]
(rx (println "B" @b)
[:div (str "c*d " @cd)])]])))
(dom/mount! (.getElementById js/document "root") (view))
(reset! b {:c 4 :d 5})
(js/setTimeout #(reset! b {:c 5 :d 4}) 100)I added some println's in animate to output the results of the equality test and it's inputs, here's what came out of the code above:
A 1
B {:c 2, :d 3}
EQUAL? false
CUR #<[object Text]>
NEW_ELEM {:c 4, :d 5}
B {:c 4, :d 5}
EQUAL? false
CUR #<[object HTMLDivElement]>
NEW_ELEM [:div c*d 20]
EQUAL? false
CUR #<[object Text]>
NEW_ELEM {:c 5, :d 4}
B {:c 5, :d 4}
EQUAL? false
CUR #<[object HTMLDivElement]>
NEW_ELEM [:div c*d 20]
I fiddled around with show-new-elem and animate to remember not just new-node, but also new-elem, and use the latter for comparisons. That change results in this output:
A 1
B {:c 2, :d 3}
EQUAL? false
CUR {:c 2, :d 3}
NEW_ELEM {:c 4, :d 5}
B {:c 4, :d 5}
EQUAL? false
CUR [:div c*d 6]
NEW_ELEM [:div c*d 20]
EQUAL? false
CUR {:c 4, :d 5}
NEW_ELEM {:c 5, :d 4}
B {:c 5, :d 4}
EQUAL? true
CUR [:div c*d 20]
NEW_ELEM [:div c*d 20]
Compare last four lines: is this more what we should expect?
Haven't played a great deal with the new atom/cursor implementations. I'm about to start converting another big chunk of our application from a custom virtual-DOM-based implementation to cereus and freactive, so that should be a good test. Also, the code above used to recompute the top-level rx when the children changed, as I guess dependencies from nested rx-expressions previously registered all the way up the tree. I don't see that happeining now, which is nice, because I used to have to be pretty diligent about wrapping nested rx's in non-reactively all over the place.
Can you post your before and after code for show-new-elem and animate? Are
you using develop HEAD? If so you are using the new atom/cursor
implementation.
On Fri, May 29, 2015 at 1:42 PM Dave Dixon notifications@github.com wrote:
Not sure this is working as you planned. Here's the code I used to test:
(ns freactive-sandbox.rx-test
(:require-macros [freactive.macros :refer [rx debug-rx]])
(:require [freactive.core :refer [atom cursor]]
[freactive.dom :as dom]))(enable-console-print!)
(def state (atom {:a 1 :b {:c 2 :d 3}}))
(def b (cursor state :b))
(defn view
[](let [a %28cursor state :a%29
cd %28cursor b #%28* %28:c %%29 %28:d %%29%29 #%28%29%29]
%28rx
%28println "A" @A)
[:div (str @A)
[:div
#_(println "BBBBB" @b)
[:div (rx (str @b))](rx %28println "B" @b)
[:div (str "c*d " @cd)])]])))(dom/mount! (.getElementById js/document "root") (view))
(reset! b {:c 4 :d 5})
(js/setTimeout #(reset! b {:c 5 :d 4}) 100)I added some println's in animate to output the results of the equality
test and it's inputs, here's what came out of the code above:A 1
B {:c 2, :d 3}
EQUAL? false
CUR #<[object Text]>
NEW_ELEM {:c 4, :d 5}
B {:c 4, :d 5}
EQUAL? false
CUR #<[object HTMLDivElement]>
NEW_ELEM [:div c_d 20]
EQUAL? false
CUR #<[object Text]>
NEW_ELEM {:c 5, :d 4}
B {:c 5, :d 4}
EQUAL? false
CUR #<[object HTMLDivElement]>
NEW_ELEM [:div c_d 20]I fiddled around with show-new-elem and animate to remember not just
new-node, but also new-elem, and use the latter for comparisons. That
change results in this output:A 1
B {:c 2, :d 3}
EQUAL? false
CUR {:c 2, :d 3}
NEW_ELEM {:c 4, :d 5}
B {:c 4, :d 5}
EQUAL? false
CUR [:div c_d 6]
NEW_ELEM [:div c_d 20]
EQUAL? false
CUR {:c 4, :d 5}
NEW_ELEM {:c 5, :d 4}
B {:c 5, :d 4}
EQUAL? true
CUR [:div c_d 20]
NEW_ELEM [:div c_d 20]Compare last four lines: is this more what we should expect?
Haven't played a great deal with the new atom/cursor implementations. I'm
about to start converting another big chunk of our application from a
custom virtual-DOM-based implementation to cereus and freactive, so that
should be a good test. Also, the code above used to recompute the top-level
rx when the children changed, as I guess dependencies from nested rx-expressions
previously registered all the way up the tree. I don't see that happeining
now, which is nice, because I used to have to be pretty diligent about
wrapping nested rx's in non-reactively all over the place.—
Reply to this email directly or view it on GitHub
#43 (comment).
You're right - the way I had it I was comparing the DOM node itself to the
virtual DOM. It should be something like (= cur-vdom new-vdom). I don't
even think the call the get-dom-image is necessary at all.