Fixes unused children components not being disposed - Fixes #13

src/IncrementalDomRenderer.js

@@ -6,6 +6,7 @@ import dom from 'metal-dom';
 import { ComponentRenderer, EventsCollector } from 'metal-component';
 import IncrementalDomAop from './IncrementalDomAop';
 import IncrementalDomChildren from './children/IncrementalDomChildren';
+import IncrementalDomUnusedComponents from './cleanup/IncrementalDomUnusedComponents';
 import IncrementalDomUtils from './utils/IncrementalDomUtils';
 
 /**
@@ -88,21 +89,6 @@ class IncrementalDomRenderer extends ComponentRenderer {
 		return this.currentPrefix_ + 'sub' + count;
 	}
 
-	/**
-	 * Disposes all sub components that were not found after an update anymore.
-	 * @protected
-	 */
-	disposeUnusedSubComponents_() {
-		var keys = Object.keys(this.component_.components);
-		var unused = [];
-		for (var i = 0; i < keys.length; i++) {
-			if (!this.subComponentsFound_[keys[i]]) {
-				unused.push(keys[i]);
-			}
-		}
-		this.component_.disposeSubComponents(unused);
-	}
-
 	/**
 	 * Gets the component being currently rendered via `IncrementalDomRenderer`.
 	 * @return {Component}
@@ -365,6 +351,24 @@ class IncrementalDomRenderer extends ComponentRenderer {
 		return attr.substr(0, 7) === 'data-on';
 	}
 
+	/**
+	 * Gets the component that is this component's parent (that is, the one that
+	 * actually rendered it), or null if there's no parent.
+	 * @return {Component}
+	 */
+	getParent() {
+		return this.parent_;
+	}
+
+	/**
+	 * Gets the component that is this component's owner (that is, the one that
+	 * passed its config properties and holds its ref), or null if there's none.
+	 * @return {Component}
+	 */
+	getOwner() {
+		return this.owner_;
+	}
+
 	/**
 	 * Renders the renderer's component for the first time, patching its element
 	 * through the incremental dom function calls done by `renderIncDom`.
@@ -446,7 +450,8 @@ class IncrementalDomRenderer extends ComponentRenderer {
 		IncrementalDomRenderer.startedRenderingComponent(this.component_);
 		this.changes_ = {};
 		this.rootElementReached_ = false;
-		this.subComponentsFound_ = {};
+		IncrementalDomUnusedComponents.schedule(this.childComponents_ || []);
+		this.childComponents_ = [];
 		this.generatedKeyCount_ = {};
 		this.listenersToAttach_ = [];
 		this.currentPrefix_ = '';
@@ -478,7 +483,10 @@ class IncrementalDomRenderer extends ComponentRenderer {
 		this.updateContext_(comp);
 		var renderer = comp.getRenderer();
 		if (renderer instanceof IncrementalDomRenderer) {
-			renderer.lastParentComponent_ = IncrementalDomRenderer.getComponentBeingRendered();
+			var parentComp = IncrementalDomRenderer.getComponentBeingRendered();
+			parentComp.getRenderer().childComponents_.push(comp);
+			renderer.parent_ = parentComp;
+			renderer.owner_ = this.component_;
 			renderer.renderInsidePatch();
 		} else {
 			console.warn(
@@ -490,7 +498,6 @@ class IncrementalDomRenderer extends ComponentRenderer {
 		if (!comp.wasRendered) {
 			comp.renderAsSubComponent();
 		}
-		this.subComponentsFound_[config.key] = true;
 		return comp;
 	}
 
@@ -522,11 +529,11 @@ class IncrementalDomRenderer extends ComponentRenderer {
 	 * done by `renderIncDom`.
 	 */
 	patch() {
-		if (!this.component_.element && this.lastParentComponent_) {
+		if (!this.component_.element && this.parent_) {
 			// If the component has no content but was rendered from another component,
 			// we'll need to patch this parent to make sure that any new content will
 			// be added in the right place.
-			this.lastParentComponent_.getRenderer().patch();
+			this.parent_.getRenderer().patch();
 			return;
 		}
 
@@ -559,7 +566,6 @@ class IncrementalDomRenderer extends ComponentRenderer {
 		if (this.hasChangedBesidesElement_() && this.shouldUpdate(this.changes_)) {
 			this.patch();
 			this.eventsCollector_.detachUnusedListeners();
-			this.disposeUnusedSubComponents_();
 		}
 	}
 

src/cleanup/IncrementalDomUnusedComponents.js

@@ -0,0 +1,43 @@
+'use strict';
+
+import { async } from 'metal';
+
+class IncrementalDomUnusedComponents {
+	/**
+	 * Schedules a cleanup of unused components to happen in the next tick.
+	 * @param {!Array<!Component} comps
+	 */
+	static schedule(comps) {
+		for (var i = 0; i < comps.length; i++) {
+			comps[i].getRenderer().parent_ = null;
+			comps_.push(comps[i]);
+		}
+		if (!scheduled_) {
+			scheduled_ = true;
+			async.nextTick(disposeUnused_);
+		}
+	}
+}
+
+var comps_ = [];
+var scheduled_ = false;
+
+/**
+ * Disposes all sub components that were not rerendered since the last
+ * time this function was scheduled.
+ * @protected
+ */
+function disposeUnused_() {
+	for (var i = 0; i < comps_.length; i++) {
+		if (!comps_[i].isDisposed()) {
+			var renderer = comps_[i].getRenderer();
+			if (!renderer.getParent()) {
+				renderer.getOwner().disposeSubComponents([comps_[i].config.key]);
+			}
+		}
+	}
+	scheduled_ = false;
+	comps_ = [];
+}
+
+export default IncrementalDomUnusedComponents;

test/IncrementalDomRenderer.js

@@ -1,6 +1,6 @@
 'use strict';
 
-import { object } from 'metal';
+import { async, object } from 'metal';
 import dom from 'metal-dom';
 import { Component, ComponentRegistry } from 'metal-component';
 import IncrementalDomChildren from '../src/children/IncrementalDomChildren';
@@ -1009,7 +1009,7 @@ describe('IncrementalDomRenderer', function() {
 			});
 		});
 
-		it('should call "detached" lifecycle function when sub component is removed', function(done) {
+		it('should call "detached" lifecycle function and event when sub component is removed', function(done) {
 			class TestComponent extends Component {
 				render() {
 					IncDom.elementOpen('div');
@@ -1031,11 +1031,13 @@ describe('IncrementalDomRenderer', function() {
 			sinon.spy(child, 'detached');
 
 			component.remove = true;
-			component.once('stateSynced', function() {
-				assert.ok(!component.components.child);
-				assert.ok(child.isDisposed());
-				assert.strictEqual(1, child.detached.callCount);
-				done();
+			child.once('detached', function() {
+				async.nextTick(function() {
+					assert.ok(!component.components.child);
+					assert.ok(child.isDisposed());
+					assert.strictEqual(1, child.detached.callCount);
+					done();
+				});
 			});
 		});
 
@@ -1481,40 +1483,130 @@ describe('IncrementalDomRenderer', function() {
 			});
 		});
 
-		it('should dispose sub components that are unused after an update', function(done) {
-			class TestComponent extends Component {
-				render() {
-					IncDom.elementOpen('div');
-					for (var i = 1; i <= this.count; i++) {
-						IncDom.elementVoid('ChildComponent', null, ['key', 'child' + i]);
+		describe('Dispose Unused Sub Components', function() {
+			it('should dispose sub components that are unused after an update', function(done) {
+				class TestComponent extends Component {
+					render() {
+						IncDom.elementOpen('div');
+						for (var i = 1; i <= this.count; i++) {
+							IncDom.elementVoid('ChildComponent', null, ['key', 'child' + i]);
+						}
+						IncDom.elementClose('div');
 					}
-					IncDom.elementClose('div');
 				}
-			}
-			TestComponent.RENDERER = IncrementalDomRenderer;
-			TestComponent.STATE = {
-				count: {
-					value: 3
+				TestComponent.RENDERER = IncrementalDomRenderer;
+				TestComponent.STATE = {
+					count: {
+						value: 3
+					}
+				};
+				component = new TestComponent();
+				var subComps = object.mixin({}, component.components);
+				assert.strictEqual(3, Object.keys(subComps).length);
+				assert.ok(subComps.child1);
+				assert.ok(subComps.child2);
+				assert.ok(subComps.child3);
+
+				component.count = 2;
+				component.once('stateSynced', function() {
+					async.nextTick(function() {
+						assert.strictEqual(2, Object.keys(component.components).length);
+						assert.ok(component.components.child1);
+						assert.ok(component.components.child2);
+						assert.ok(!component.components.child3);
+
+						assert.ok(!subComps.child1.isDisposed());
+						assert.ok(!subComps.child2.isDisposed());
+						assert.ok(subComps.child3.isDisposed());
+						done();
+					});
+				});
+			});
+
+			it('should dispose sub components from "children" that are unused after an update', function(done) {
+				class TestChildComponent extends Component {
+					render() {
+						IncDom.elementOpen('div');
+						IncrementalDomRenderer.renderChild(
+							this.config.children[this.index]
+						);
+						IncDom.elementClose('div');
+					}
 				}
-			};
-			component = new TestComponent();
-			var subComps = object.mixin({}, component.components);
-			assert.strictEqual(3, Object.keys(subComps).length);
-			assert.ok(subComps.child1);
-			assert.ok(subComps.child2);
-			assert.ok(subComps.child3);
+				TestChildComponent.RENDERER = IncrementalDomRenderer;
+				TestChildComponent.STATE = {
+					index: {
+						value: 0
+					}
+				};
 
-			component.count = 2;
-			component.once('stateSynced', function() {
-				assert.strictEqual(2, Object.keys(component.components).length);
-				assert.ok(component.components.child1);
-				assert.ok(component.components.child2);
-				assert.ok(!component.components.child3);
+				class TestComponent extends Component {
+					render() {
+						IncDom.elementOpen(TestChildComponent, 'child');
+						IncDom.elementVoid(ChildComponent, 'item1');
+						IncDom.elementVoid(ChildComponent, 'item2');
+						IncDom.elementClose(TestChildComponent);
+					}
+				}
+				TestComponent.RENDERER = IncrementalDomRenderer;
 
-				assert.ok(!subComps.child1.isDisposed());
-				assert.ok(!subComps.child2.isDisposed());
-				assert.ok(subComps.child3.isDisposed());
-				done();
+				component = new TestComponent();
+				var child = component.components.child;
+				var item1 = component.components.item1;
+				assert.ok(item1);
+				assert.ok(!component.components.item2);
+				assert.ok(!item1.isDisposed());
+
+				child.index = 1;
+				component.once('stateSynced', function() {
+					async.nextTick(function() {
+						assert.strictEqual(child, component.components.child);
+						assert.ok(!component.components.item1);
+						assert.ok(component.components.item2);
+						assert.ok(item1.isDisposed());
+						done();
+					});
+				});
+			});
+
+			it('should dispose sub components from sub components that are unused after a parent update', function(done) {
+				class TestChildComponent extends Component {
+					render() {
+						IncDom.elementOpen('div');
+						if (!this.config.remove) {
+							IncDom.elementOpen(ChildComponent, 'innerChild');
+						}
+						IncDom.elementClose('div');
+					}
+				}
+				TestChildComponent.RENDERER = IncrementalDomRenderer;
+
+				class TestComponent extends Component {
+					render() {
+						IncDom.elementVoid(TestChildComponent, 'child', [], 'remove', this.remove);
+					}
+				}
+				TestComponent.RENDERER = IncrementalDomRenderer;
+				TestComponent.STATE = {
+					remove: {
+					}
+				};
+
+				component = new TestComponent();
+				var child = component.components.child;
+				var innerChild = child.components.innerChild;
+				assert.ok(innerChild);
+
+				component.remove = true;
+				component.once('stateSynced', function() {
+					async.nextTick(function() {
+						assert.strictEqual(child, component.components.child);
+						assert.ok(!child.isDisposed());
+						assert.ok(!child.components.innerChild);
+						assert.ok(innerChild.isDisposed());
+						done();
+					});
+				});
 			});
 		});
 	});