From 0d38735749245b400b21c20cd197e37f2a4afa1f Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Tue, 12 May 2015 17:35:11 -0400 Subject: [PATCH 1/3] [BUGFIX beta] Prevent `willDestroyElement` from being called twice. The node-managers ensure that `willDestroyElement` (and other hooks) are called properly (by walking the much more precise `view._renderNode` tree), having the renderer's `willDestroyElement` also traverse `childViews` leads to `willDestroyElement` getting called twice in cases other than those for `ContainerView`. --- .../will-destroy-element-hook-test.js | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 packages/ember-htmlbars/tests/integration/will-destroy-element-hook-test.js diff --git a/packages/ember-htmlbars/tests/integration/will-destroy-element-hook-test.js b/packages/ember-htmlbars/tests/integration/will-destroy-element-hook-test.js new file mode 100644 index 00000000000..b7095955c94 --- /dev/null +++ b/packages/ember-htmlbars/tests/integration/will-destroy-element-hook-test.js @@ -0,0 +1,60 @@ +import run from 'ember-metal/run_loop'; +import Component from 'ember-views/views/component'; +import compile from 'ember-template-compiler/system/compile'; +import { runAppend, runDestroy } from "ember-runtime/tests/utils"; + +import { set } from 'ember-metal/property_set'; + +var component; + +QUnit.module('ember-htmlbars: destroy-element-hook tests', { + teardown() { + runDestroy(component); + } +}); + +QUnit.test('willDestroyElement is only called once when a component leaves scope', function(assert) { + var done = assert.async(); + var innerChild, innerChildDestroyed; + + component = Component.create({ + switch: true, + + layout: compile(` + {{~#if switch~}} + {{~#view innerChild}}Truthy{{/view~}} + {{~/if~}} + `), + + innerChild: Component.extend({ + init() { + this._super(...arguments); + innerChild = this; + }, + + willDestroyElement() { + if (innerChildDestroyed) { + throw new Error('willDestroyElement has already been called!!'); + } else { + innerChildDestroyed = true; + } + } + }) + }); + + runAppend(component); + + assert.equal(component.$().text(), 'Truthy', 'precond - truthy template is displayed'); + assert.equal(component.get('childViews.length'), 1); + + run.later(function() { + set(component, 'switch', false); + + run.later(function() { + assert.equal(innerChild.get('isDestroyed'), true, 'the innerChild has been destroyed'); + assert.equal(component.$().text(), '', 'truthy template is removed'); + + done(); + }); + }); +}); From 6e8e8ac87da7676c332d2281b5c15366e8410f96 Mon Sep 17 00:00:00 2001 From: Kris Selden Date: Thu, 11 Jun 2015 23:50:39 -0700 Subject: [PATCH 2/3] move recursion from inside of renderer into ContainerView which is the only thing that seems to need this --- packages/ember-metal-views/lib/renderer.js | 16 +++---------- .../lib/mixins/view_state_support.js | 6 ++--- .../ember-views/lib/views/container_view.js | 23 +++++++++++++++++-- 3 files changed, 27 insertions(+), 18 deletions(-) diff --git a/packages/ember-metal-views/lib/renderer.js b/packages/ember-metal-views/lib/renderer.js index bfa37b29b3a..c29a7976e07 100755 --- a/packages/ember-metal-views/lib/renderer.js +++ b/packages/ember-metal-views/lib/renderer.js @@ -231,14 +231,7 @@ Renderer.prototype.willDestroyElement = function (view) { } if (view._transitionTo) { - view._transitionTo('destroying', false); - } - - var childViews = view.childViews; - if (childViews) { - for (var i = 0; i < childViews.length; i++) { - this.willDestroyElement(childViews[i]); - } + view._transitionTo('destroying'); } }; @@ -253,11 +246,8 @@ Renderer.prototype.didDestroyElement = function (view) { view._transitionTo('preRender'); } - var childViews = view.childViews; - if (childViews) { - for (var i = 0; i < childViews.length; i++) { - this.didDestroyElement(childViews[i]); - } + if (view.trigger) { + view.trigger('didDestroyElement'); } }; // element destroyed so view.destroy shouldn't try to remove it removedFromDOM diff --git a/packages/ember-views/lib/mixins/view_state_support.js b/packages/ember-views/lib/mixins/view_state_support.js index 1f49573e983..bd7e894624e 100644 --- a/packages/ember-views/lib/mixins/view_state_support.js +++ b/packages/ember-views/lib/mixins/view_state_support.js @@ -2,12 +2,12 @@ import Ember from 'ember-metal/core'; import { Mixin } from "ember-metal/mixin"; var ViewStateSupport = Mixin.create({ - transitionTo(state, children) { + transitionTo(state) { Ember.deprecate("Ember.View#transitionTo has been deprecated, it is for internal use only"); - this._transitionTo(state, children); + this._transitionTo(state); }, - _transitionTo(state, children) { + _transitionTo(state) { var priorState = this.currentState; var currentState = this.currentState = this._states[state]; this._state = state; diff --git a/packages/ember-views/lib/views/container_view.js b/packages/ember-views/lib/views/container_view.js index 41df4f0e89c..a2d376091ae 100644 --- a/packages/ember-views/lib/views/container_view.js +++ b/packages/ember-views/lib/views/container_view.js @@ -9,6 +9,7 @@ import { observer, beforeObserver } from "ember-metal/mixin"; +import { on } from "ember-metal/events"; import containerViewTemplate from "ember-htmlbars/templates/container-view"; containerViewTemplate.meta.revision = 'Ember@VERSION_STRING_PLACEHOLDER'; @@ -280,8 +281,26 @@ var ContainerView = View.extend(MutableArray, { }, objectAt(idx) { - return get(this, 'childViews')[idx]; - } + return this.childViews[idx]; + }, + + _triggerChildWillDestroyElement: on('willDestroyElement', function () { + var childViews = this.childViews; + if (childViews) { + for (var i = 0; i < childViews.length; i++) { + this.renderer.willDestroyElement(childViews[i]); + } + } + }), + + _triggerChildDidDestroyElement: on('didDestroyElement', function () { + var childViews = this.childViews; + if (childViews) { + for (var i = 0; i < childViews.length; i++) { + this.renderer.didDestroyElement(childViews[i]); + } + } + }) }); export default ContainerView; From 177733b42d7aef11a7b63f684d3194ddeb6e1447 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Fri, 12 Jun 2015 17:16:53 -0500 Subject: [PATCH 3/3] Remove invalid usage of run.later. --- .../integration/will-destroy-element-hook-test.js | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/packages/ember-htmlbars/tests/integration/will-destroy-element-hook-test.js b/packages/ember-htmlbars/tests/integration/will-destroy-element-hook-test.js index b7095955c94..987f17fd0f3 100644 --- a/packages/ember-htmlbars/tests/integration/will-destroy-element-hook-test.js +++ b/packages/ember-htmlbars/tests/integration/will-destroy-element-hook-test.js @@ -14,7 +14,6 @@ QUnit.module('ember-htmlbars: destroy-element-hook tests', { }); QUnit.test('willDestroyElement is only called once when a component leaves scope', function(assert) { - var done = assert.async(); var innerChild, innerChildDestroyed; component = Component.create({ @@ -47,14 +46,12 @@ QUnit.test('willDestroyElement is only called once when a component leaves scope assert.equal(component.$().text(), 'Truthy', 'precond - truthy template is displayed'); assert.equal(component.get('childViews.length'), 1); - run.later(function() { + run(function() { set(component, 'switch', false); + }); - run.later(function() { - assert.equal(innerChild.get('isDestroyed'), true, 'the innerChild has been destroyed'); - assert.equal(component.$().text(), '', 'truthy template is removed'); - - done(); - }); + run(function() { + assert.equal(innerChild.get('isDestroyed'), true, 'the innerChild has been destroyed'); + assert.equal(component.$().text(), '', 'truthy template is removed'); }); });