From e71a5115eeafb137ef977f5720174e6dd18de466 Mon Sep 17 00:00:00 2001
From: Michael Aufreiter <michael@substance.io>
Date: Tue, 8 Sep 2015 12:25:32 +0200
Subject: [PATCH] Fix double didMount bug.

---
 test/unit/ui/component.test.js | 18 ++++++++++++++----
 ui/component.js                | 20 +++++++++++++-------
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/test/unit/ui/component.test.js b/test/unit/ui/component.test.js
index 94fcd0821..cf79e1a64 100644
--- a/test/unit/ui/component.test.js
+++ b/test/unit/ui/component.test.js
@@ -10,6 +10,7 @@ var TestComponent = Component.extend({
   didInitialize: function() {
     // make some methods inspectable
     this.didMount = sinon.spy(this, 'didMount');
+    this.willUnmount = sinon.spy(this, 'willUnmount');
     this.shouldRerender = sinon.spy(this, 'shouldRerender');
     this.render = sinon.spy(this, 'render');
   }
@@ -157,9 +158,9 @@ QUnit.test("Do deep rerender when state has changed.", function(assert) {
   comp.setState({ foo: 'baz' });
   assert.equal(comp.shouldRerender.callCount, 1, "Component should have been asked whether to rerender.");
   assert.equal(comp.render.callCount, 2, "Component should have been rerendered.");
- });
+});
 
-QUnit.test("Only call didMount once.", function(assert) {
+QUnit.test("Only call didMount once for childs and grandchilds when setProps is called during mounting process.", function(assert) {
   var Child = TestComponent.extend({
     render: function() {
       if (this.props.loading) {
@@ -180,7 +181,16 @@ QUnit.test("Only call didMount once.", function(assert) {
       this.refs.child.setProps({ loading: false });
     }
   });
+  
   var comp = Component.mount($$(Parent), $('#qunit-fixture'));
-  assert.equal(comp.refs.child.didMount.callCount, 1, "Child's didMount should have been called only once.");
-  assert.equal(comp.refs.child.refs.child.didMount.callCount, 1, "Grandchild's didMount should have been called only once.");
+  var childComp = comp.refs.child;
+  var grandChildComp = childComp.refs.child;
+  assert.equal(childComp.didMount.callCount, 1, "Child's didMount should have been called only once.");
+  assert.equal(grandChildComp.didMount.callCount, 1, "Grandchild's didMount should have been called only once.");
+  
+  comp.empty();
+  assert.equal(childComp.willUnmount.callCount, 1, "Child's willUnmount should have been called once.");
+  assert.equal(grandChildComp.willUnmount.callCount, 1, "Grandchild's willUnmount should have been called once.");
+
+  // TODO: can/should we somehow dispose the references to childComp and grandChildComp now?
 });
diff --git a/ui/component.js b/ui/component.js
index a14764ce6..564e63a5a 100644
--- a/ui/component.js
+++ b/ui/component.js
@@ -4,7 +4,6 @@ var OO = require('../basics/oo');
 var _ = require('../basics/helpers');
 
 var __id__ = 0;
-var _isDocumentElement;
 var _isInDocument;
 var VirtualTextNode;
 var RawHtml;
@@ -209,7 +208,10 @@ Component.Prototype = function ComponentPrototype() {
    * ```
    */
   this.triggerDidMount = function() {
-    this.didMount();
+    if (!this.__mounted__) {
+      this.__mounted__ = true;
+      this.didMount();
+    }
     _.each(this.children, function(child) {
       child.triggerDidMount();
     });
@@ -237,6 +239,7 @@ Component.Prototype = function ComponentPrototype() {
   this.unmount = function() {
     this.triggerWillUnmount();
     this.$el.remove();
+    this.__mounted__ = false;
     // TODO: do we need to remove this from parents children
     // right now it feels like that it doesn't make a great difference
     // because most often this method is called by the parent during rerendering
@@ -1123,14 +1126,17 @@ Component.$$.prepareChildren = function(children) {
   }
 };
 
-_isDocumentElement = function(el) {
-  // Node.DOCUMENT_NODE = 9
-  return (el.nodeType === 9);
-};
+/**
+ * Checks whether a given element has been injected in the document already
+ *
+ * We traverse up the DOM until we find the document root element. We return true
+ * if we can find it.
+ */
 
 _isInDocument = function(el) {
   while(el) {
-    if (_isDocumentElement(el)) {
+    // Node.DOCUMENT_NODE = 9;
+    if (el.nodeType === 9) {
       return true;
     }
     el = el.parentNode;