From 5e5fb79d45bc7fce77af7e915bc8a2713f1c9786 Mon Sep 17 00:00:00 2001 From: Maira Bello Date: Thu, 4 Aug 2016 13:47:34 -0300 Subject: [PATCH] Fixes disposed components with ref not being removed from owners --- .../src/IncrementalDomRenderer.js | 9 ++++ .../cleanup/IncrementalDomUnusedComponents.js | 33 +++++++------- .../test/IncrementalDomRenderer.js | 44 +++++++++++++++++++ .../cleanup/IncrementalDomUnusedComponents.js | 31 ++++++++----- 4 files changed, 89 insertions(+), 28 deletions(-) diff --git a/packages/metal-incremental-dom/src/IncrementalDomRenderer.js b/packages/metal-incremental-dom/src/IncrementalDomRenderer.js index aed37a0c..60a09190 100644 --- a/packages/metal-incremental-dom/src/IncrementalDomRenderer.js +++ b/packages/metal-incremental-dom/src/IncrementalDomRenderer.js @@ -163,6 +163,14 @@ class IncrementalDomRenderer extends ComponentRenderer { */ disposeInternal() { super.disposeInternal(); + + var comp = this.component_; + var ref = this.config_.ref; + var owner = this.getOwner(); + if (owner && owner.components && owner.components[ref] === comp) { + delete owner.components[ref]; + } + for (var i = 0; i < this.childComponents_.length; i++) { const child = this.childComponents_[i]; if (!child.isDisposed()) { @@ -170,6 +178,7 @@ class IncrementalDomRenderer extends ComponentRenderer { child.dispose(); } } + this.childComponents_ = null; } /** diff --git a/packages/metal-incremental-dom/src/cleanup/IncrementalDomUnusedComponents.js b/packages/metal-incremental-dom/src/cleanup/IncrementalDomUnusedComponents.js index 40c9b451..d9319a0e 100644 --- a/packages/metal-incremental-dom/src/cleanup/IncrementalDomUnusedComponents.js +++ b/packages/metal-incremental-dom/src/cleanup/IncrementalDomUnusedComponents.js @@ -1,6 +1,7 @@ 'use strict'; var comps_ = []; +var disposing_ = false; class IncrementalDomUnusedComponents { /** @@ -8,26 +9,22 @@ class IncrementalDomUnusedComponents { * time this function was scheduled. */ static disposeUnused() { + if (disposing_) { + return; + } + disposing_ = true; + for (var i = 0; i < comps_.length; i++) { var comp = comps_[i]; - if (!comp.isDisposed()) { - var renderer = comp.getRenderer(); - if (!renderer.getParent()) { - // Don't let disposing cause the element to be removed, since it may - // be currently being reused by another component. - comp.element = null; - - var ref = renderer.config_.ref; - var owner = renderer.getOwner(); - if (owner && !owner.isDisposed() && owner.components[ref] === comp) { - owner.disposeSubComponents([ref]); - } else { - comp.dispose(); - } - } + if (!comp.isDisposed() && !comp.getRenderer().getParent()) { + // Don't let disposing cause the element to be removed, since it may + // be currently being reused by another component. + comp.element = null; + comp.dispose(); } } comps_ = []; + disposing_ = false; } /** @@ -37,8 +34,10 @@ class IncrementalDomUnusedComponents { */ static schedule(comps) { for (var i = 0; i < comps.length; i++) { - comps[i].getRenderer().parent_ = null; - comps_.push(comps[i]); + if (!comps[i].isDisposed()) { + comps[i].getRenderer().parent_ = null; + comps_.push(comps[i]); + } } } } diff --git a/packages/metal-incremental-dom/test/IncrementalDomRenderer.js b/packages/metal-incremental-dom/test/IncrementalDomRenderer.js index b5a8b9a6..f364708f 100644 --- a/packages/metal-incremental-dom/test/IncrementalDomRenderer.js +++ b/packages/metal-incremental-dom/test/IncrementalDomRenderer.js @@ -2575,6 +2575,50 @@ describe('IncrementalDomRenderer', function() { }); }); + describe('Dispose', function() { + it('should remove component with ref from owner', function() { + class Child extends Component { + } + Child.RENDERER = IncrementalDomRenderer; + + class TestComponent extends Component { + render() { + IncrementalDOM.elementVoid(Child, null, null, 'ref', 'child'); + } + } + TestComponent.RENDERER = IncrementalDomRenderer; + + component = new TestComponent(); + var child = component.components.child; + + child.dispose(); + assert.ok(child.isDisposed()); + assert.ok(!component.components.child); + }); + + it('should not remove different component with same ref from owner', function() { + class Child extends Component { + } + Child.RENDERER = IncrementalDomRenderer; + + class TestComponent extends Component { + render() { + IncrementalDOM.elementVoid(Child, null, null, 'ref', 'child'); + } + } + TestComponent.RENDERER = IncrementalDomRenderer; + + component = new TestComponent(); + var child = component.components.child; + component.components.child = new Child(); + + child.dispose(); + assert.ok(child.isDisposed()); + assert.ok(component.components.child); + assert.ok(!component.components.child.isDisposed()); + }); + }); + describe('IncrementalDomRenderer.isIncDomNode', function() { it('should check if given data is an incremental dom node', function() { assert.ok(!IncrementalDomRenderer.isIncDomNode({})); diff --git a/packages/metal-incremental-dom/test/cleanup/IncrementalDomUnusedComponents.js b/packages/metal-incremental-dom/test/cleanup/IncrementalDomUnusedComponents.js index 187d577a..2f51bdeb 100644 --- a/packages/metal-incremental-dom/test/cleanup/IncrementalDomUnusedComponents.js +++ b/packages/metal-incremental-dom/test/cleanup/IncrementalDomUnusedComponents.js @@ -50,7 +50,7 @@ describe('IncrementalDomUnusedComponents', function() { assert.ok(comps[1].isDisposed()); }); - it('should not dispose scheduled components that have parent again', function() { + it('should not dispose scheduled components that have received a new parent', function() { var comps = [comp.components.child1, comp.components.child2]; IncrementalDomUnusedComponents.schedule(comps); comps[0].getRenderer().parent_ = comp; @@ -60,10 +60,10 @@ describe('IncrementalDomUnusedComponents', function() { assert.ok(comps[1].isDisposed()); }); - it('should not dispose scheduled components that have already been disposed', function() { + it('should not dispose scheduled components that have been disposed before scheduled', function() { var comps = [comp.components.child1, comp.components.child2]; - IncrementalDomUnusedComponents.schedule(comps); comps[0].dispose(); + IncrementalDomUnusedComponents.schedule(comps); sinon.spy(comps[0], 'dispose'); sinon.spy(comps[1], 'dispose'); IncrementalDomUnusedComponents.disposeUnused(); @@ -72,22 +72,31 @@ describe('IncrementalDomUnusedComponents', function() { assert.strictEqual(1, comps[1].dispose.callCount); }); - it('should not dispose different component with same ref as a scheduled component', function() { + it('should not dispose scheduled components that have been disposed after scheduled', function() { var comps = [comp.components.child1, comp.components.child2]; IncrementalDomUnusedComponents.schedule(comps); + comps[0].dispose(); + sinon.spy(comps[0], 'dispose'); + sinon.spy(comps[1], 'dispose'); + IncrementalDomUnusedComponents.disposeUnused(); + + assert.strictEqual(0, comps[0].dispose.callCount); + assert.strictEqual(1, comps[1].dispose.callCount); + }); - comp.addSubComponent('child1', new Component()); - var newChild1 = comp.components.child1; - assert.notStrictEqual(comps[0], newChild1); + it('should not throw error when `disposeUnused` is called during another `disposeUnused` call', function() { + sinon.stub(comp.components.child1, 'disposed', () => { + IncrementalDomUnusedComponents.disposeUnused(); + }); - IncrementalDomUnusedComponents.disposeUnused(); + var comps = [comp.components.child1]; + IncrementalDomUnusedComponents.schedule(comps); + assert.doesNotThrow(() => IncrementalDomUnusedComponents.disposeUnused()); assert.ok(comps[0].isDisposed()); - assert.ok(comps[1].isDisposed()); - assert.ok(!newChild1.isDisposed()); }); - it('should not throw error when disposing component that has previously disposed owner', function() { + it('should not throw error when disposing component that has an owner that was previously disposed', function() { var comps = [comp.components.child2, grandchild]; IncrementalDomUnusedComponents.schedule(comps); assert.doesNotThrow(() => IncrementalDomUnusedComponents.disposeUnused());