Some diff glitches

Question

Some diff glitches

ul opened this issue 10 years ago · 19 comments

Consider this snippet:

(let [structures (fr/cursor state #(-> % :deck-structures keys))]
    [:div
     [:div {:horizontal 1 :layout 1 :center-justified 1}
      [:paper-fab {:icon     "fa:plus"
                   :on-click (fn [_] (swap! state assoc :page [:deck-structures (encore/uuid-str)]))}]]

     (rx (into [:div] (mapv (fn [k] (deck-structure (fr/cursor state [:deck-structures k]))) @structures)))
     ])

It throws

 Uncaught TypeError: Cannot read property 'tag' of undefined
dom.cljs:591try_diff
dom.cljs:638replace_child
dom.cljs:733(anonymous function)
dom.cljs:769animate
dom.cljs:233render
tick.js:30(anonymous function)
tick.js:30processRafCallbacks

When I modify try_diff that way:

(defn- try-diff [parent spec vdom cur-dom-node top-level]
  (if-let [cur-state (get-element-state cur-dom-node)]
    (let [cur-tag (.-tag cur-state)
          new-tag (first vdom)]
      (if (identical? spec (.-element-spec cur-state))
        cur-dom-node
        (if (keyword-identical? new-tag cur-tag)
          (do
            ;(println "diff hit" (first vdom))
            (let [old-attrs (.-attrs cur-state)
                  new-attrs? (second vdom)
                  new-attrs (when (map? new-attrs?) new-attrs?)]
              (reset-element-spec! cur-state vdom new-tag new-attrs)
              (replace-attrs! cur-dom-node
                              cur-state
                              old-attrs
                              new-attrs)
              (let [new-children (if new-attrs (nnext vdom) (next vdom))
                    dangling-child (try-diff-subseq cur-dom-node (.-firstChild cur-dom-node) new-children)]
                (loop [cur-child dangling-child]
                  (when cur-child
                    (let [next-sib (.-nextSibling cur-child)]
                      (remove-dom-node cur-child)
                      (recur next-sib)))))
              (on-attached cur-state cur-dom-node)
              cur-dom-node))
          (do
            ;(println "build hit" (first vdom) (first cur-vdom))
            (replace-node-completely parent vdom cur-dom-node top-level)))))
    (do
      (js/console.log "Problems:"  parent cur-dom-node)
      (replace-node-completely parent vdom cur-dom-node top-level))))

I got:

Uncaught NotFoundError: Failed to execute 'replaceChild' on 'Node': The node to be replaced is not a child of this node.
dom.cljs:564replace_node_completely
dom.cljs:621try_diff
dom.cljs:642replace_child
dom.cljs:742(anonymous function)
dom.cljs:773animate
dom.cljs:233render
tick.js:30(anonymous function)
tick.js:30processRafCallbacks

and parent is top :div of snippet, cur-dom-node is what rendered by (rx (into [:div] (mapv (fn [k] (deck-structure (fr/cursor state [:deck-structures k]))) @structures)))

Answer 1 · 2014-11-27T14:17:11.000Z

Not surprised you're having problems with diffing. You can disable it for
now if you like by setting enable-diffing to false. It is important to
debug the issues with it though thanks for the report.

On Thursday, November 27, 2014, Ruslan Prokopchuk notifications@github.com
wrote:

Consider this snippet:

(let [structures (fr/cursor state #(-> % :deck-structures keys))]
[:div
[:div {:horizontal 1 :layout 1 :center-justified 1}
[:paper-fab {:icon "fa:plus"
:on-click (fn [_](swap! state assoc :page [:deck-structures %28encore/uuid-str%29]))}]]
 (rx (into [:div] (mapv (fn [k] (deck-structure (fr/cursor state [:deck-structures k]))) @structures)))
 ])
It throws

Uncaught TypeError: Cannot read property 'tag' of undefined
dom.cljs:591try_diff
dom.cljs:638replace_child
dom.cljs:733(anonymous function)
dom.cljs:769animate
dom.cljs:233render
tick.js:30(anonymous function)
tick.js:30processRafCallbacks

When I modify try_diff that way:

(defn- try-diff [parent spec vdom cur-dom-node top-level](if-let [cur-state %28get-element-state cur-dom-node%29]
%28let [cur-tag %28.-tag cur-state%29
new-tag %28first vdom%29]
%28if %28identical? spec %28.-element-spec cur-state%29%29
cur-dom-node
%28if %28keyword-identical? new-tag cur-tag%29
%28do
;%28println "diff hit" %28first vdom))
(let [old-attrs (.-attrs cur-state)
new-attrs? (second vdom)
new-attrs (when (map? new-attrs?) new-attrs?)](reset-element-spec! cur-state vdom new-tag new-attrs)
(replace-attrs! cur-dom-node
cur-state
old-attrs
new-attrs)
(let [new-children (if new-attrs (nnext vdom) (next vdom))
dangling-child (try-diff-subseq cur-dom-node (.-firstChild cur-dom-node) new-children)](loop [cur-child dangling-child]
%28when cur-child
%28let [next-sib %28.-nextSibling cur-child%29]
%28remove-dom-node cur-child%29
%28recur next-sib%29%29%29))
(on-attached cur-state cur-dom-node)
cur-dom-node))
(do
;(println "build hit" (first vdom) (first cur-vdom))
(replace-node-completely parent vdom cur-dom-node top-level)))))
(do
(js/console.log "Problems:" parent cur-dom-node)
(replace-node-completely parent vdom cur-dom-node top-level))))

I got:

Uncaught NotFoundError: Failed to execute 'replaceChild' on 'Node': The node to be replaced is not a child of this node.
dom.cljs:564replace_node_completely
dom.cljs:621try_diff
dom.cljs:642replace_child
dom.cljs:742(anonymous function)
dom.cljs:773animate
dom.cljs:233render
tick.js:30(anonymous function)
tick.js:30processRafCallbacks

and parent is top :div of snippet, cur-dom-node is what rendered by (rx
(into [:div](mapv %28fn [k] %28deck-structure %28fr/cursor state
[:deck-structures k]%29%29%29 @Structures)))

—
Reply to this email directly or view it on GitHub
#22.

Answer 2 · 2014-11-27T14:18:37.000Z

I'm happy if this report will help!

Answer 3 · 2014-11-27T17:44:39.000Z

I just pushed fda4d33. Can you see if this helps at all?

It's possible there might be another issue with this new fix, but I'm not sure yet...

Answer 4 · 2014-11-27T18:18:29.000Z

Nope, the same exception is thrown.

Answer 5 · 2014-11-27T18:48:33.000Z

Can you paste the stack trace (it may have changed a little)?

On Thursday, November 27, 2014, Ruslan Prokopchuk notifications@github.com
wrote:

Nope, the same exception is thrown.

—
Reply to this email directly or view it on GitHub
#22 (comment).

Answer 6 · 2014-11-27T19:01:36.000Z

Uncaught TypeError: Cannot read property 'tag' of undefined dom.cljs:591
try_diff dom.cljs:638
replace_child dom.cljs:736
show_new_elem_fn dom.cljs:768 
animate dom.cljs:233 
render tick.js:30 
(anonymous function) tick.js:30 
processRafCallbacks tick.js:30

Answer 7 · 2014-11-27T19:05:51.000Z

Oops! I didn't actually include the intended fix. Try this: bb8de6d

Answer 8 · 2014-11-27T19:13:21.000Z

The same error, but with one difference:
If I reload page at route that render this code — I get no error, but (rx (into [:div... is not rendered at all.
If I load page with another route and then update state to render page with problematic code — I get exactly the same error as before.

Answer 9 · 2014-11-27T19:21:30.000Z

Okay, just to make sure - if you disable diffing, does it work as expected?

On Thu, Nov 27, 2014 at 2:13 PM, Ruslan Prokopchuk <notifications@github.com

wrote:

The same error, but with one difference:
If I reload page at route that render this code — I get no error, but (rx
(into [:div... is not rendered at all.
If I load page with another route and then update state to render page
with problematic code — I get exactly the same error as before.

—
Reply to this email directly or view it on GitHub
#22 (comment).

Answer 10 · 2014-11-27T19:22:01.000Z

Yes, it does.

Answer 11 · 2014-11-27T19:24:42.000Z

I will make repo with minimal case tomorrow if you don't come to solution earlier, too tired today to mess with that now, sorry.

Answer 12 · 2014-11-27T19:26:25.000Z

Yes, that would be helpful. Thanks.

On Thu, Nov 27, 2014 at 2:24 PM, Ruslan Prokopchuk <notifications@github.com

wrote:

I will make repo with minimal case tomorrow if you don't come to solution
earlier, too tired today to mess with that now, sorry.

—
Reply to this email directly or view it on GitHub
#22 (comment).

Answer 13 · 2014-11-27T19:47:54.000Z

I did just post a bug fix for some issues with diffing child nodes: 03cf8e9. This may or may not help your issue.

Answer 14 · 2014-11-29T06:54:59.000Z

Sorry for delay, this is repo with minimal case https://github.com/ul/freactive-issue-22 based on latest master of freactive.

But it have some differences in behaviour comparing my initial code: in minimal case I don't get exception "Cannot read property 'tag' of undefined", but code rendered in mapv dissapears in the same way. If fix of minimal case will not lead to fix of exception I will investigate how to reproduce exception then.

Answer 15 · 2014-12-22T04:31:35.000Z

Can you verify that this commit (on the develop branch) resolves your diff issues? It was an error in trying to have disposed bindings remove their own children.

In this develop branch, I've also made some progress with some other things:

Overall binding performance is improved - almost 2x improvement in fps in the animation benchmark
Automatic memory management logic was added to deal with other memory leaks I found - memory cleanup should be pretty good now

I also benchmarked (in Chrome) both memory consumption and pure diffing fps performance against the React/Reagent comparison example. See: https://github.com/aaronc/freactive/blob/develop/test/freactive/diff_perf.cljs. Memory usage appears on par or slightly higher in freactive (5-10%) in the dom-perf example and almost the same in the diff-perf example. Pure diffing (diff-perf example) performance actually appears to be slightly higher than the React/Reagent example.

If you get a chance to try this branch and see if you have any issues, it would be useful. The main other thing I want to address for 0.2.0 is the transitions API if you have any input: #8. I'll probably push items-view, etc. to 0.3.0.

Answer 16 · 2014-12-23T08:40:59.000Z

Minimal case I've posted seems to be resolved, but I still have errors in large project. Need time to figure out what the difference with prev min case and produce a new one.

Answer 17 · 2015-06-10T05:50:12.000Z

Closing this, because project which produced error is abandoned and used old version of freactive. I'm sure that this specific bug is fixed in develop.

Answer 18 · 2015-06-10T14:51:57.000Z

Well in develop, there is no diffing at all because I decided that it was more important to support reactive element sequences which if used correctly (together with direct binding) eliminates a lot of the need for diffing. Still diffing could be added as some later point if desired and I think it would be easier to get it correct with the new architecture.

Answer 19 · 2015-06-10T15:20:36.000Z

Glad to see that Freactive is alive again! I will be happy to try it in my projects when it will be possible (for current project I can't change framework)