From 78f4df640955915e0b30c6ad174d60771d5ca624 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Wed, 3 Jun 2015 07:22:05 -0400 Subject: [PATCH 1/2] Failing test for {{component}} keyword teardown. Currently, the `{{component}}` keyword does not teardown previously created components when swapping. This test demonstrates that. --- .../node-managers/component-node-manager.js | 3 ++ .../lib/node-managers/view-node-manager.js | 4 ++ .../tests/helpers/component_test.js | 48 +++++++++++++++++++ 3 files changed, 55 insertions(+) diff --git a/packages/ember-htmlbars/lib/node-managers/component-node-manager.js b/packages/ember-htmlbars/lib/node-managers/component-node-manager.js index cf3c0e2df5e..cc966552130 100644 --- a/packages/ember-htmlbars/lib/node-managers/component-node-manager.js +++ b/packages/ember-htmlbars/lib/node-managers/component-node-manager.js @@ -269,6 +269,9 @@ export function createComponent(_component, isAngleBracket, _props, renderNode, } component._renderNode = renderNode; + if (renderNode.emberView && renderNode.emberView !== component) { + throw new Error('Need to clean up this view before blindly reassigning.'); + } renderNode.emberView = component; return component; } diff --git a/packages/ember-htmlbars/lib/node-managers/view-node-manager.js b/packages/ember-htmlbars/lib/node-managers/view-node-manager.js index 6bb438aa510..ef34cd8f6a8 100644 --- a/packages/ember-htmlbars/lib/node-managers/view-node-manager.js +++ b/packages/ember-htmlbars/lib/node-managers/view-node-manager.js @@ -188,6 +188,10 @@ export function createOrUpdateComponent(component, options, createOptions, rende } component._renderNode = renderNode; + + if (renderNode.emberView && renderNode.emberView !== component) { + throw new Error('Need to clean up this view before blindly reassigning.'); + } renderNode.emberView = component; return component; } diff --git a/packages/ember-htmlbars/tests/helpers/component_test.js b/packages/ember-htmlbars/tests/helpers/component_test.js index fd9364281b9..453524ad5fe 100644 --- a/packages/ember-htmlbars/tests/helpers/component_test.js +++ b/packages/ember-htmlbars/tests/helpers/component_test.js @@ -48,6 +48,54 @@ if (Ember.FEATURES.isEnabled('ember-htmlbars-component-helper')) { equal(view.$().text(), 'yummy Loisaida arepas!', 'component was updated and re-rendered'); }); + QUnit.test("component helper destroys underlying component when it is swapped out", function() { + var currentComponent; + var destroyCalls = 0; + registry.register('component:foo-bar', Component.extend({ + init() { + this._super(...arguments); + currentComponent = 'foo-bar'; + }, + willDestroy() { + destroyCalls++; + } + })); + registry.register('component:baz-qux', Component.extend({ + init() { + this._super(...arguments); + currentComponent = 'baz-qux'; + }, + willDestroy() { + destroyCalls++; + } + })); + + view = EmberView.create({ + container: container, + dynamicComponent: 'foo-bar', + template: compile('{{component view.dynamicComponent}}') + }); + + runAppend(view); + + equal(currentComponent, 'foo-bar', 'precond - instantiates the proper component'); + equal(destroyCalls, 0, 'precond - nothing destroyed yet'); + + Ember.run(function() { + set(view, "dynamicComponent", 'baz-qux'); + }); + + equal(currentComponent, 'baz-qux', 'changing bound value instantiates the proper component'); + equal(destroyCalls, 1, 'prior component should be destroyed'); + + Ember.run(function() { + set(view, "dynamicComponent", 'foo-bar'); + }); + + equal(currentComponent, 'foo-bar', 'changing bound value instantiates the proper component'); + equal(destroyCalls, 2, 'prior components destroyed'); + }); + QUnit.test("component helper with actions", function() { registry.register('template:components/foo-bar', compile('yippie! {{yield}}')); registry.register('component:foo-bar', Ember.Component.extend({ From ac846c375b44089c6e0d9d6c3cb953ced5c30d7d Mon Sep 17 00:00:00 2001 From: Tom Dale Date: Thu, 11 Jun 2015 16:39:29 -0700 Subject: [PATCH 2/2] Destroy components when changed on re-render Most of the infrastructure around components assumes they are static (e.g. cannot become a after it is rendered). As such, there were some holes in destruction for the one dynamic case (the {{component}} helper). This commit detects when the underlying component has changed and ensures that the component is scheduled for destruction before the new component is swapped in. It also fixes a similar issue with the view helper. --- packages/ember-htmlbars/lib/keywords/component.js | 4 ++++ packages/ember-htmlbars/lib/keywords/view.js | 5 +++++ .../lib/node-managers/component-node-manager.js | 12 +++++++++--- .../lib/node-managers/view-node-manager.js | 7 ++++--- 4 files changed, 22 insertions(+), 6 deletions(-) diff --git a/packages/ember-htmlbars/lib/keywords/component.js b/packages/ember-htmlbars/lib/keywords/component.js index 6bc4bffaa22..b3843ff4a9c 100644 --- a/packages/ember-htmlbars/lib/keywords/component.js +++ b/packages/ember-htmlbars/lib/keywords/component.js @@ -7,6 +7,10 @@ export default { }, render(morph, ...rest) { + if (morph.state.manager) { + morph.state.manager.destroy(); + } + // Force the component hook to treat this as a first-time render, // because normal components (``) cannot change at runtime, // but the `{{component}}` helper can. diff --git a/packages/ember-htmlbars/lib/keywords/view.js b/packages/ember-htmlbars/lib/keywords/view.js index 2d14b5e7c13..dc61c15f68f 100644 --- a/packages/ember-htmlbars/lib/keywords/view.js +++ b/packages/ember-htmlbars/lib/keywords/view.js @@ -70,6 +70,11 @@ export default { options.createOptions._targetObject = node.state.targetObject; } + if (state.manager) { + state.manager.destroy(); + state.manager = null; + } + var nodeManager = ViewNodeManager.create(node, env, hash, options, parentView, null, scope, template); state.manager = nodeManager; diff --git a/packages/ember-htmlbars/lib/node-managers/component-node-manager.js b/packages/ember-htmlbars/lib/node-managers/component-node-manager.js index 7b851fb705f..0bfade72df8 100644 --- a/packages/ember-htmlbars/lib/node-managers/component-node-manager.js +++ b/packages/ember-htmlbars/lib/node-managers/component-node-manager.js @@ -264,6 +264,15 @@ ComponentNodeManager.prototype.rerender = function(_env, attrs, visitor) { }, this); }; +ComponentNodeManager.prototype.destroy = function() { + let component = this.component; + + // Clear component's render node. Normally this gets cleared + // during view destruction, but in this case we're re-assigning the + // node to a different view and it will get cleaned up automatically. + component._renderNode = null; + component.destroy(); +}; export function createComponent(_component, isAngleBracket, _props, renderNode, env, attrs = {}) { let props = assign({}, _props); @@ -298,9 +307,6 @@ export function createComponent(_component, isAngleBracket, _props, renderNode, } component._renderNode = renderNode; - if (renderNode.emberView && renderNode.emberView !== component) { - throw new Error('Need to clean up this view before blindly reassigning.'); - } renderNode.emberView = component; return component; } diff --git a/packages/ember-htmlbars/lib/node-managers/view-node-manager.js b/packages/ember-htmlbars/lib/node-managers/view-node-manager.js index 33b6373a290..55bc10a3499 100644 --- a/packages/ember-htmlbars/lib/node-managers/view-node-manager.js +++ b/packages/ember-htmlbars/lib/node-managers/view-node-manager.js @@ -149,6 +149,10 @@ ViewNodeManager.prototype.rerender = function(env, attrs, visitor) { }, this); }; +ViewNodeManager.prototype.destroy = function() { + this.component.destroy(); +}; + function getTemplate(componentOrView) { return componentOrView.isComponent ? get(componentOrView, '_template') : get(componentOrView, 'template'); } @@ -192,9 +196,6 @@ export function createOrUpdateComponent(component, options, createOptions, rende component._renderNode = renderNode; - if (renderNode.emberView && renderNode.emberView !== component) { - throw new Error('Need to clean up this view before blindly reassigning.'); - } renderNode.emberView = component; return component; }