From 5aa58752cc421420a93e42c583fb8260ad0e0648 Mon Sep 17 00:00:00 2001 From: Kris Selden Date: Tue, 30 Aug 2016 02:54:14 -0700 Subject: [PATCH 1/2] [BUGFIX beta] Fix cleanup --- .../components/curly-components-test.js | 70 ++++++++++ .../integration/components/life-cycle-test.js | 129 ++++++++++++++++-- .../lib/hooks/cleanup-render-node.js | 20 +++ .../lib/hooks/destroy-render-node.js | 6 +- .../lib/hooks/did-cleanup-tree.js | 4 + .../lib/hooks/will-cleanup-tree.js | 4 +- .../lib/keywords/element-component.js | 4 +- .../node-managers/component-node-manager.js | 5 +- packages/ember-htmlbars/lib/renderer.js | 28 +--- .../lib/views/states/has_element.js | 2 +- 10 files changed, 223 insertions(+), 49 deletions(-) diff --git a/packages/ember-glimmer/tests/integration/components/curly-components-test.js b/packages/ember-glimmer/tests/integration/components/curly-components-test.js index 79f1c382a68..d28d83313df 100644 --- a/packages/ember-glimmer/tests/integration/components/curly-components-test.js +++ b/packages/ember-glimmer/tests/integration/components/curly-components-test.js @@ -2458,4 +2458,74 @@ moduleFor('Components test: curly components', class extends RenderingTest { attrs: { id: 'foo-bar', style: styles('color: blue; display: none;') } }); } + + ['@test child triggers revalidate during parent destruction (GH#13846)']() { + let select; + + this.registerComponent('x-select', { + ComponentClass: Component.extend({ + tagName: 'select', + + init() { + this._super(); + this.options = emberA([]); + this.value = null; + + select = this; + }, + + updateValue() { + var newValue = this.get('options.lastObject.value'); + + this.set('value', newValue); + }, + + registerOption(option) { + this.get('options').addObject(option); + }, + + unregisterOption(option) { + this.get('options').removeObject(option); + + this.updateValue(); + } + }), + + template: '{{yield this}}' + }); + + this.registerComponent('x-option', { + ComponentClass: Component.extend({ + tagName: 'option', + attributeBindings: ['selected'], + + didInsertElement() { + this._super(...arguments); + + this.get('select').registerOption(this); + }, + + selected: computed('select.value', function() { + return this.get('value') === this.get('select.value'); + }), + + willDestroyElement() { + this._super(...arguments); + this.get('select').unregisterOption(this); + } + }) + }); + + this.render(strip` + {{#x-select value=value as |select|}} + {{#x-option value="1" select=select}}1{{/x-option}} + {{#x-option value="2" select=select}}2{{/x-option}} + {{/x-select}} + `); + + + this.teardown(); + + this.assert.ok(true, 'no errors during teardown'); + } }); diff --git a/packages/ember-glimmer/tests/integration/components/life-cycle-test.js b/packages/ember-glimmer/tests/integration/components/life-cycle-test.js index 088b2be5d05..72790425150 100644 --- a/packages/ember-glimmer/tests/integration/components/life-cycle-test.js +++ b/packages/ember-glimmer/tests/integration/components/life-cycle-test.js @@ -51,17 +51,19 @@ class LifeCycleHooksTest extends RenderingTest { }; let assertParentView = (hookName, instance) => { - if (!instance.parentView) { - this.assert.ok(false, `parentView should be present in ${hookName}`); + this.assert.ok(instance.parentView, `parentView should be present in ${hookName}`); + if (hookName === 'willDestroyElement') { + this.assert.ok(instance.parentView.childViews.indexOf(instance) !== -1, `view is still connected to parentView in ${hookName}`); + } + if (this.isHTMLBars) { + this.assert.ok(instance.ownerView, `ownerView should be present in ${hookName}`); } }; let assertElement = (hookName, instance) => { if (instance.tagName === '') { return; } - if (!instance.element) { - this.assert.ok(false, `element property should be present on ${instance} during ${hookName}`); - } + this.assert.ok(instance.element && document.contains(instance.element), `element property should be present on ${instance} during ${hookName}`); let inDOM = this.$(`#${instance.elementId}`)[0]; if (!inDOM) { @@ -134,6 +136,14 @@ class LifeCycleHooksTest extends RenderingTest { pushHook('willDestroyElement'); assertParentView('willDestroyElement', this); assertElement('willDestroyElement', this); + }, + + didDestroyElement() { + pushHook('didDestroyElement'); + }, + + willDestroy() { + pushHook('willDestroy'); } }); @@ -387,8 +397,26 @@ class LifeCycleHooksTest extends RenderingTest { 'destroy', ['the-top', 'willDestroyElement'], ['the-middle', 'willDestroyElement'], - ['the-bottom', 'willDestroyElement'] + ['the-bottom', 'willDestroyElement'], + ['the-top', 'didDestroyElement'], + ['the-middle', 'didDestroyElement'], + ['the-bottom', 'didDestroyElement'], + ['the-top', 'willDestroy'], + ['the-middle', 'willDestroy'], + ['the-bottom', 'willDestroy'] ); + + this.assert.equal(this.components['the-top']._state, 'preRender'); + this.assert.equal(this.components['the-top'].isDestroying, true); + this.assert.equal(this.components['the-top'].isDestroyed, true); + + this.assert.equal(this.components['the-middle']._state, 'preRender'); + this.assert.equal(this.components['the-middle'].isDestroying, true); + this.assert.equal(this.components['the-middle'].isDestroyed, true); + + this.assert.equal(this.components['the-bottom']._state, 'preRender'); + this.assert.equal(this.components['the-bottom'].isDestroying, true); + this.assert.equal(this.components['the-bottom'].isDestroyed, true); }); } @@ -559,20 +587,42 @@ class LifeCycleHooksTest extends RenderingTest { 'destroy', ['the-top', 'willDestroyElement'], ['the-middle', 'willDestroyElement'], - ['the-bottom', 'willDestroyElement'] + ['the-bottom', 'willDestroyElement'], + ['the-top', 'didDestroyElement'], + ['the-middle', 'didDestroyElement'], + ['the-bottom', 'didDestroyElement'], + ['the-top', 'willDestroy'], + ['the-middle', 'willDestroy'], + ['the-bottom', 'willDestroy'] ); + + this.assert.equal(this.components['the-top']._state, 'preRender'); + this.assert.equal(this.components['the-top'].isDestroying, true); + this.assert.equal(this.components['the-top'].isDestroyed, true); + + this.assert.equal(this.components['the-middle']._state, 'preRender'); + this.assert.equal(this.components['the-middle'].isDestroying, true); + this.assert.equal(this.components['the-middle'].isDestroyed, true); + + this.assert.equal(this.components['the-bottom']._state, 'preRender'); + this.assert.equal(this.components['the-bottom'].isDestroying, true); + this.assert.equal(this.components['the-bottom'].isDestroyed, true); }); } ['@test components rendered from `{{each}}` have correct life-cycle hooks to be called']() { let { invoke } = this.boundHelpers; + this.registerComponent('nested-item', { template: strip` + {{yield}} + ` }); + this.registerComponent('an-item', { template: strip` -
Item: {{count}}
+ {{#nested-item}}Item: {{count}}{{/nested-item}} ` }); this.registerComponent('no-items', { template: strip` -
Nothing to see here
+ {{#nested-item}}
Nothing to see here
{{/nested-item}} ` }); this.render(strip` @@ -592,12 +642,18 @@ class LifeCycleHooksTest extends RenderingTest { ['an-item', 'init'], ['an-item', 'didInitAttrs', { attrs: { count } }], ['an-item', 'didReceiveAttrs', { newAttrs: { count } }], - ['an-item', 'willRender'] + ['an-item', 'willRender'], + ['nested-item', 'init'], + ['nested-item', 'didInitAttrs', { attrs: { } }], + ['nested-item', 'didReceiveAttrs', { newAttrs: { } }], + ['nested-item', 'willRender'] ]; }; let initialAfterRenderHooks = (count) => { return [ + ['nested-item', 'didInsertElement'], + ['nested-item', 'didRender'], ['an-item', 'didInsertElement'], ['an-item', 'didRender'] ]; @@ -622,7 +678,9 @@ class LifeCycleHooksTest extends RenderingTest { ...initialAfterRenderHooks(1) ); + this.assert.equal(this.component.childViews.length, 5, 'childViews precond'); this.runTask(() => set(this.context, 'items', [])); + this.assert.equal(this.component.childViews.length, 1, 'childViews updated'); this.assertText('Nothing to see here'); @@ -630,26 +688,71 @@ class LifeCycleHooksTest extends RenderingTest { 'reset to empty array', ['an-item', 'willDestroyElement'], + ['nested-item', 'willDestroyElement'], + ['an-item', 'didDestroyElement'], + ['nested-item', 'didDestroyElement'], ['an-item', 'willDestroyElement'], + ['nested-item', 'willDestroyElement'], + ['an-item', 'didDestroyElement'], + ['nested-item', 'didDestroyElement'], ['an-item', 'willDestroyElement'], + ['nested-item', 'willDestroyElement'], + ['an-item', 'didDestroyElement'], + ['nested-item', 'didDestroyElement'], ['an-item', 'willDestroyElement'], + ['nested-item', 'willDestroyElement'], + ['an-item', 'didDestroyElement'], + ['nested-item', 'didDestroyElement'], ['an-item', 'willDestroyElement'], + ['nested-item', 'willDestroyElement'], + ['an-item', 'didDestroyElement'], + ['nested-item', 'didDestroyElement'], ['no-items', 'init'], ['no-items', 'didInitAttrs', { attrs: { } }], ['no-items', 'didReceiveAttrs', { newAttrs: { } }], ['no-items', 'willRender'], + ['nested-item', 'init'], + ['nested-item', 'didInitAttrs', { attrs: { } }], + ['nested-item', 'didReceiveAttrs', { newAttrs: { } }], + ['nested-item', 'willRender'], + + ['nested-item', 'didInsertElement'], + ['nested-item', 'didRender'], ['no-items', 'didInsertElement'], - ['no-items', 'didRender'] + ['no-items', 'didRender'], + + ['an-item', 'willDestroy'], + ['nested-item', 'willDestroy'], + ['an-item', 'willDestroy'], + ['nested-item', 'willDestroy'], + ['an-item', 'willDestroy'], + ['nested-item', 'willDestroy'], + ['an-item', 'willDestroy'], + ['nested-item', 'willDestroy'], + ['an-item', 'willDestroy'], + ['nested-item', 'willDestroy'] ); this.teardownAssertions.push(() => { this.assertHooks( 'destroy', - - ['no-items', 'willDestroyElement'] + ['no-items', 'willDestroyElement'], + ['nested-item', 'willDestroyElement'], + ['no-items', 'didDestroyElement'], + ['nested-item', 'didDestroyElement'], + ['no-items', 'willDestroy'], + ['nested-item', 'willDestroy'] ); + + this.assert.equal(this.components['no-items']._state, 'preRender'); + this.assert.equal(this.components['no-items'].isDestroying, true); + this.assert.equal(this.components['no-items'].isDestroyed, true); + + this.assert.equal(this.components['nested-item']._state, 'preRender'); + this.assert.equal(this.components['nested-item'].isDestroying, true); + this.assert.equal(this.components['nested-item'].isDestroyed, true); }); } } diff --git a/packages/ember-htmlbars/lib/hooks/cleanup-render-node.js b/packages/ember-htmlbars/lib/hooks/cleanup-render-node.js index e0cce31c781..92b262962a2 100644 --- a/packages/ember-htmlbars/lib/hooks/cleanup-render-node.js +++ b/packages/ember-htmlbars/lib/hooks/cleanup-render-node.js @@ -4,5 +4,25 @@ */ export default function cleanupRenderNode(renderNode) { + let view = renderNode.emberView; + if (view) { + view.renderer.willDestroyElement(view); + view.ownerView._destroyingSubtreeForView.push(env => { + view._transitionTo('destroying'); // unregisters view + // prevents rerender and scheduling + view._renderNode = null; + renderNode.emberView = null; + + view.renderer.didDestroyElement(view); + + if (view.parentView && view.parentView === env.view) { + view.parentView.removeChild(view); + } + view.parentView = null; + + view._transitionTo('preRender'); + }); + } + if (renderNode.cleanup) { renderNode.cleanup(); } } diff --git a/packages/ember-htmlbars/lib/hooks/destroy-render-node.js b/packages/ember-htmlbars/lib/hooks/destroy-render-node.js index 4be52cd4b06..3456e4bbb70 100644 --- a/packages/ember-htmlbars/lib/hooks/destroy-render-node.js +++ b/packages/ember-htmlbars/lib/hooks/destroy-render-node.js @@ -5,13 +5,15 @@ export default function destroyRenderNode(renderNode) { let view = renderNode.emberView; if (view) { - view.renderer.remove(view, true); + view.ownerView._destroyingSubtreeForView.push(() => { + view.destroy(); + }); } - let streamUnsubscribers = renderNode.streamUnsubscribers; if (streamUnsubscribers) { for (let i = 0; i < streamUnsubscribers.length; i++) { streamUnsubscribers[i](); } } + renderNode.streamUnsubscribers = null; } diff --git a/packages/ember-htmlbars/lib/hooks/did-cleanup-tree.js b/packages/ember-htmlbars/lib/hooks/did-cleanup-tree.js index b50b6c46d78..fdbcb4abc9c 100644 --- a/packages/ember-htmlbars/lib/hooks/did-cleanup-tree.js +++ b/packages/ember-htmlbars/lib/hooks/did-cleanup-tree.js @@ -1,5 +1,9 @@ export default function didCleanupTree(env) { // Once we have finsihed cleaning up the render node and sub-nodes, reset // state tracking which view those render nodes belonged to. + let queue = env.view.ownerView._destroyingSubtreeForView; + for (let i = 0; i < queue.length; i++) { + queue[i](env); + } env.view.ownerView._destroyingSubtreeForView = null; } diff --git a/packages/ember-htmlbars/lib/hooks/will-cleanup-tree.js b/packages/ember-htmlbars/lib/hooks/will-cleanup-tree.js index 81fe68fc7da..38c693be29f 100644 --- a/packages/ember-htmlbars/lib/hooks/will-cleanup-tree.js +++ b/packages/ember-htmlbars/lib/hooks/will-cleanup-tree.js @@ -1,5 +1,5 @@ export default function willCleanupTree(env) { - let view = env.view; + let { view } = env; // When we go to clean up the render node and all of its children, we may // encounter views/components associated with those nodes along the way. In @@ -28,5 +28,5 @@ export default function willCleanupTree(env) { // the `childViews` array. Other parent/child view relationships are // untouched. This view is then cleared once cleanup is complete in // `didCleanupTree`. - view.ownerView._destroyingSubtreeForView = view; + view.ownerView._destroyingSubtreeForView = []; } diff --git a/packages/ember-htmlbars/lib/keywords/element-component.js b/packages/ember-htmlbars/lib/keywords/element-component.js index fb51c92e468..fc0232624fa 100644 --- a/packages/ember-htmlbars/lib/keywords/element-component.js +++ b/packages/ember-htmlbars/lib/keywords/element-component.js @@ -19,7 +19,7 @@ export default { }); }, - render(morph, ...rest) { + render(morph) { let state = morph.getState(); if (state.manager) { @@ -31,7 +31,7 @@ export default { // but the `{{component}}` helper can. state.manager = null; - render(morph, ...rest); + render(...arguments); }, rerender: render 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 01f5380196c..592427f9a59 100644 --- a/packages/ember-htmlbars/lib/node-managers/component-node-manager.js +++ b/packages/ember-htmlbars/lib/node-managers/component-node-manager.js @@ -151,13 +151,10 @@ ComponentNodeManager.prototype.rerender = function ComponentNodeManager_rerender }; ComponentNodeManager.prototype.destroy = function ComponentNodeManager_destroy() { - 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(); + this.component.destroy(); }; export function createComponent(_component, props, renderNode, env, attrs = {}) { diff --git a/packages/ember-htmlbars/lib/renderer.js b/packages/ember-htmlbars/lib/renderer.js index e62c762ca47..83861d0596a 100755 --- a/packages/ember-htmlbars/lib/renderer.js +++ b/packages/ember-htmlbars/lib/renderer.js @@ -228,34 +228,12 @@ Renderer.prototype.rerender = function (view) { renderNode.ownerNode.emberView.scheduleRevalidate(renderNode, view.toString(), 'rerendering'); }; -Renderer.prototype.remove = function (view, shouldDestroy) { - let renderNode = view._renderNode; - view._renderNode = null; - if (renderNode) { - renderNode.emberView = null; - this.willDestroyElement(view); - view._transitionTo('destroying'); - - view._renderNode = null; - let lastResult = renderNode.lastResult; - if (lastResult) { - internal.clearMorph(renderNode, lastResult.env, shouldDestroy !== false); - } - - if (!shouldDestroy) { - view._transitionTo('preRender'); - } - this.didDestroyElement(view); - } - - // toplevel view removed, remove insertion point +Renderer.prototype.remove = function (view) { let lastResult = view.lastResult; - if (lastResult) { + if (lastResult) { // toplevel only. view.lastResult = null; lastResult.destroy(); - } - - if (shouldDestroy && !view.isDestroying) { + } else { view.destroy(); } }; diff --git a/packages/ember-views/lib/views/states/has_element.js b/packages/ember-views/lib/views/states/has_element.js index 82ea3c932e8..b87f12289cb 100644 --- a/packages/ember-views/lib/views/states/has_element.js +++ b/packages/ember-views/lib/views/states/has_element.js @@ -34,7 +34,7 @@ assign(hasElement, { }, destroy(view) { - view.renderer.remove(view, true); + view.renderer.remove(view); }, // Handle events from `Ember.EventDispatcher` From 08803e324521aba5ca0bd22e1da8e45d4364df0b Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Tue, 30 Aug 2016 10:26:21 -0400 Subject: [PATCH 2/2] Fix issue with document.contains on IE. `document.contains` does not exist in Internet Explorer (tested in IE9, IE10, and IE11), but `document.body.contains` does. Since `document.body.contains` works just fine for our purposes (we don't need to check if `document.head` contains the element), we can just use that. --- .../tests/integration/components/life-cycle-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ember-glimmer/tests/integration/components/life-cycle-test.js b/packages/ember-glimmer/tests/integration/components/life-cycle-test.js index 72790425150..7ac3ba91aaa 100644 --- a/packages/ember-glimmer/tests/integration/components/life-cycle-test.js +++ b/packages/ember-glimmer/tests/integration/components/life-cycle-test.js @@ -63,7 +63,7 @@ class LifeCycleHooksTest extends RenderingTest { let assertElement = (hookName, instance) => { if (instance.tagName === '') { return; } - this.assert.ok(instance.element && document.contains(instance.element), `element property should be present on ${instance} during ${hookName}`); + this.assert.ok(instance.element && document.body.contains(instance.element), `element property should be present on ${instance} during ${hookName}`); let inDOM = this.$(`#${instance.elementId}`)[0]; if (!inDOM) {