Changing children types will cause React Error

[xeqi]

Summary

When changing children of a PIXI.Stage into different types, React will attempt to remove a DOM node, causing an error.

Reproduction

Minimal example:

(ns example.core
  (:require [om.core :as om :include-macros true]
            [omreactpixi.abbrev :as pixi]))

(defmulti game :state :default :loading)

(defmethod game :loading [app owner]
  (reify
    om/IRender
    (render [_]
      (pixi/displayobjectcontainer
       {:interactive true
        :buttonMode true
        :click #(om/transact! app (fn [c] (assoc c :state :loaded)))}
       (pixi/text {:text "Click here to load"
                   :x 30
                   :y 30})))))

(defmethod game :loaded [app owner]
  (reify
    om/IRender
    (render [_]
      (pixi/text {:text "Loaded!"}))))

(defn main []
  (om/root
   (fn [app owner]
     (om/component
      (pixi/stage {:width 640
                   :height 480
                   :backgroundcolor 0x888888}
                  (om/build game app))))
   {}
   {:target (. js/document (getElementById "app"))}))

Expected

When the text is clicked the PIXI.DisplayObjectContainer should be removed and a PIXI.Text should be added.

Current Output

Console Error:

Uncaught Error: Invariant Violation: findComponentRoot(..., .0.0): Unable to find element. This probably means the DOM was unexpectedly mutated (e.g., by the browser), usually due to forgetting a <tbody> when using tables, nesting tags like <form>, <p>, or <a>, or using non-SVG elements in an <svg> parent. Try inspecting the child nodes of the element with React ID ``.

Additional notes

This will happen when changing the type of the children. I originally hit this going from PIXI.Text to PIXI.Sprite, but figured this example was more minimal. reactpixi.js#L6630 calls shouldUpdateReactComponent to check if it should attempt DOM node removal, and this functions seems to allow inplace changes when they are the same type.

The error happens when reactpixi.js#L6646 calls ReactComponent.BackendIDOperations.dangerouslyReplaceNodeWithMarkupByID. Since this attempts to search the DOM for the nodes I have a feeling this shouldn't be happening. I don't understand React's internal mixins enough to propose a solution.

This might have been better placed in react-pixi, but I don't have the build tools to create a reproduction case.

[Izzimach]

Wow, that is an awesome bug report!

I've not tried to generate components via multimethods which is why I haven't this before.

The presence of dangerouslyReplaceNodeWithMarkupByID makes me think this is probably something similar to the behavior referred to in one of the react-pixi tests.

Hopefully this is the same kind of bug. Fixing it without modifying Om means I'll probably have to monkey-patch React though.

[xeqi]

It is unrelated to the multimethod. The error happens when that code is replaced with:

(defn game [app owner]
  (if (:loaded (:state app))
    (reify
      om/IRender
      (render [_]
        (pixi/text {:text "Winner!"})))
    (reify
      om/IRender
      (render [_]
        (pixi/displayobjectcontainer
         {:interactive true
          :buttonMode true
          :click #(om/transact! app (fn [c] (assoc c :state :loaded)))}
         (pixi/text {:text "Click here to win"
                     :x 30
                     :y 30}))))))

However, removing the om/build and reifys and instead using a straight conditional and pixi/... seems to work. Example:

(defn main []
  (om/root
   (fn [app owner]
     (om/component
      (pixi/stage {:width 640
                   :height 480
                   :backgroundcolor 0x888888}
                  (if (:winner app)
                    (pixi/text {:text "Winner!"})
                    (pixi/displayobjectcontainer
                     {:interactive true
                      :buttonMode true
                      :click #(om/transact! app (fn [c] (assoc c :winner true)))}
                     (pixi/text {:text "Click here to win"
                                 :x 30
                                 :y 30}))))))
   {}
   {:target (. js/document (getElementById "app"))}))

This leads me to believe it is Om's wrapping components getting in the way, though I haven't had time to trace through om/build* without getting lost.

[xeqi]

updated copy/paste errors above.

[xeqi]

It looks like this is the same issue the react-pixi test was setting up and checking. Messing with the "metadata" Om uses to make it use ReactPixi/createClass will work correctly. Example:

(defn game [app owner]
  (if (:winner app)
    (reify
      om/IRender
      (render [_]
        (pixi/text {:text "Winner!"})))
    (reify
      om/IRender
      (render [_]
        (pixi/displayobjectcontainer
         {:interactive true
          :buttonMode true
          :click #(om/transact! app (fn [c] (assoc c :winner true)))}
         (pixi/text {:text "Click here to win"
                     :x 30
                     :y 30}))))))

(aset game "om$descriptor"
       (js/React.createFactory
        (js/ReactPIXI.createClass om/pure-descriptor)))

(defn main []
  (om/root
   (fn [app owner]
     (om/component
      (pixi/stage {:width 640
                   :height 480
                   :backgroundcolor 0x888888}
                  (om/build game app))))
   {}
   {:target (. js/document (getElementById "app"))}))

This is good enough for a work around on my end. Hopefully this disappears in the future, as it looks like the next version of React has removed the DOM manipulation from ReactCompositeComponent. https://github.com/facebook/react/commit/5951a131db0d4c56cb435f78d3ae53dda0834441

[Izzimach]

Thanks for the info. Hopefully that commit to remove DOM-specifics from ReactCompositeComponent will get into 0.13.

Until then it looks like react-pixi should just replace React.createClass with a patched version that handles both DOM and pixi nodes. I'll add an issue in react-pixi.

[Izzimach]

@xeqi can I use some of this code as an automated test? Once I've navigated the lein-cljsbuild docs that is.

[xeqi]

Sure, though I'd suggest adapting it to use an atom, render once, use `swap!, and then render + diff/test.

Is there particular formal language you would like me state? Something like: I authorize this code to be licensed under the same license as the project, currently Apache v2; as would be normal under a pull request.

[Izzimach]

Nah no formal language, just thought I should check with you before I dumped that code into a test file.

[xeqi]

Unfortunately I've run into further issues around om/build, and setting the descriptor is not sufficient. I'm going to add on to this issue as it's related to the underlying cause.

Summary

Removed PIXI display objects may return on screen

Reproduction

(ns example.core
  (:require [om.core :as om :include-macros true]
            [omreactpixi.abbrev :as pixi]))

(defonce app-state (atom {:hello "hello"}))

(defn game [app owner]
  (om/component
   (if (:loaded app)
     (pixi/text {:text (:hello app)})
     (pixi/displayobjectcontainer
      {}
      (pixi/text {:text "loading"})))))

(aset game "om$descriptor"
      (js/React.createFactory
       (js/ReactPIXI.createClass om/pure-descriptor)))

(defn main []
  (om/root
   (fn [app owner]
     (reify
       om/IRender
       (render [_]
         (pixi/stage {:width 640
                      :height 480
                      :backgroundcolor 0x888888}
                     (om/build game app)))))
   app-state
   {:target (. js/document (getElementById "app"))}))

(comment
  ;; Run these at a repl, possibly in a chestnut setup
  (swap! app-state assoc :loaded true)
  (swap! app-state assoc :hello "huh?"))

Expected

A PIXI.Stage with a PIXI.text saying "huh?"

Output

A PIXI.Stage with a PIXI.text saying "huh?", but on top of a PIXI.DisplayObjectContainer holding a PIXI.Text saying "loading".

Additional Notes

My tracing seems to indicate that the ReactPixiComponent for the Stage is holding onto a reference to the ReactCompositeComponent wrapped around the ReactPixiComponent for the DisplayObjectContainer under _renderedChildren. Then on react-pixi.js#L12072 it uses this as the basis for previous components during _updateChildren. Then react-pixi#L12089 calls moveChild on this old component, which results in re-adding the DisplayObjectContainer to the Stage.

[Izzimach]

I can't get the first example to raise an exception. Maybe you're using a different version of Om or React-Pixi? the first example in your second post does error out though, so I'll work from that for now.

Also it's evident that I didn't mention in the README that you can use :include-macros but that doesn't appear to effect this bug (and it shouldn't)

            [omreactpixi.abbrev :as pixi :include-macros true]

[Izzimach]

So react-pixi 0.2.2 patches React.createClass which fixes the invariant violation but not the element duplication.

[Izzimach]

I'm going to (try and) reproduce that second example in javascript since it'll be easier to debug/test.

[Izzimach]

Looks like de3db64c2bc6a8e74f5289cd19ac3cd6c8e57e40 fixes this problem. So upgrading to react-pixi 0.2.3 should fix all these issues.