Skip to content

Commit

Permalink
Merge pull request #14227 from emberjs/lifecycle-hook-teardown
Browse files Browse the repository at this point in the history 
Implement correctly ordered destruction hooks.
  • Loading branch information
@rwjblue
rwjblue authored Sep 7, 2016
2 parents c5cab7f + 3dcde0f commit 945d81a
Show file tree
Hide file tree
Showing 5 changed files with 178 additions and 26 deletions.
24 changes: 24 additions & 0 deletions packages/ember-glimmer/lib/environment.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ export default class Environment extends GlimmerEnvironment {
super(...arguments);
this.owner = owner;

// can be removed once https://github.com/tildeio/glimmer/pull/305 lands
this.destroyedComponents = undefined;

installPlatformSpecificProtocolForURL(this);

this._definitionCache = new Cache(2000, ({ name, source, owner }) => {
Expand Down Expand Up @@ -341,6 +344,27 @@ export default class Environment extends GlimmerEnvironment {
didDestroy(destroyable) {
destroyable.destroy();
}

begin() {
this.inTransaction = true;

super.begin();

this.destroyedComponents = [];
}

commit() {
// components queued for destruction must be destroyed before firing
// `didCreate` to prevent errors when removing and adding a component
// with the same name (would throw an error when added to view registry)
for (let i = 0; i < this.destroyedComponents.length; i++) {
this.destroyedComponents[i].destroy();
}

super.commit();

this.inTransaction = false;
}
}

runInDebug(() => {
Expand Down
45 changes: 38 additions & 7 deletions packages/ember-glimmer/lib/renderer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class RootState {
assert(`You cannot render \`${self.value()}\` without a template.`, template);

this.id = getViewId(root);
this.env = env;
this.root = root;
this.result = undefined;
this.shouldReflush = false;
Expand All @@ -83,14 +84,35 @@ class RootState {
}

destroy() {
let { result } = this;
let { result, env } = this;

this.env = null;
this.root = null;
this.result = null;
this.render = null;

if (result) {
/*
Handles these scenarios:
* When roots are removed during standard rendering process, a transaction exists already
`.begin()` / `.commit()` are not needed.
* When roots are being destroyed manually (`component.append(); component.destroy() case), no
transaction exists already.
* When roots are being destroyed during `Renderer#destroy`, no transaction exists
*/
let needsTransaction = !env.inTransaction;

if (needsTransaction) {
env.begin();
}

result.destroy();

if (needsTransaction) {
env.commit();
}
}
}
}
Expand Down Expand Up @@ -194,10 +216,22 @@ export class Renderer {
}

remove(view) {
view.trigger('willDestroyElement');
view.trigger('willClearRender');
view._transitionTo('destroying');

view.element = null;
view.trigger('didDestroyElement');

this.cleanupRootFor(view);

if (!view.isDestroying) {
view.destroy();
}
}

cleanupRootFor(view) {
// no need to cleanup roots if we have already been destroyed
if (this._destroyed) { return; }

let roots = this._roots;

// traverse in reverse so we can remove items
Expand All @@ -215,10 +249,6 @@ export class Renderer {
if (this._roots.length === 0) {
deregister(this);
}

if (!view.isDestroying) {
view.destroy();
}
}

destroy() {
Expand Down Expand Up @@ -308,6 +338,7 @@ export class Renderer {
let root = roots[i];
root.destroy();
}

this._roots = null;

if (roots.length) {
Expand Down
13 changes: 11 additions & 2 deletions packages/ember-glimmer/lib/syntax/curly-component.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,15 @@ class ComponentStateBucket {
this.finalizer = finalizer;
}

destroy() {
let { component, environment } = this;

component.trigger('willDestroyElement');
component.trigger('willClearRender');

environment.destroyedComponents.push(component);
}

finalize() {
let { finalizer } = this;
finalizer();
Expand Down Expand Up @@ -345,8 +354,8 @@ class CurlyComponentManager {
component.trigger('didRender');
}

getDestructor({ component }) {
return component;
getDestructor(stateBucket) {
return stateBucket;
}
}

Expand Down
120 changes: 104 additions & 16 deletions packages/ember-glimmer/tests/integration/components/life-cycle-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,19 +71,17 @@ class LifeCycleHooksTest extends RenderingTest {

let assertParentView = (hookName, instance) => {
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}`);
}
};

let assertElement = (hookName, instance) => {
if (instance.tagName === '') { return; }

if (!instance.element) {
this.assert.ok(false, `element property should be present on ${instance} during ${hookName}`);
}

let inDOM = this.$(`#${instance.elementId}`)[0];
if (!inDOM) {
this.assert.ok(false, `element for ${instance} should be in the DOM during ${hookName}`);
}
this.assert.ok(instance.element, `element property should be present on ${instance} during ${hookName}`);
this.assert.ok(document.body.contains(instance.element), `element for ${instance} should be in the DOM during ${hookName}`);
};

let assertNoElement = (hookName, instance) => {
Expand All @@ -97,6 +95,7 @@ class LifeCycleHooksTest extends RenderingTest {
expectDeprecation(() => { this._super(...arguments); },
/didInitAttrs called/);

this.componentName = name;
pushHook('init');
pushComponent(this);
assertParentView('init', this);
Expand Down Expand Up @@ -159,8 +158,15 @@ class LifeCycleHooksTest extends RenderingTest {
assertElement('willClearRender', this);
},

didDestroyElement() {
pushHook('didDestroyElement');
assertNoElement('didDestroyElement', this);
},

willDestroy() {
pushHook('willDestroy');
removeComponent(this);

this._super(...arguments);
}
});
Expand Down Expand Up @@ -354,7 +360,13 @@ class LifeCycleHooksTest extends RenderingTest {
['the-middle', 'willDestroyElement'],
['the-middle', 'willClearRender'],
['the-bottom', 'willDestroyElement'],
['the-bottom', 'willClearRender']
['the-bottom', 'willClearRender'],
['the-top', 'didDestroyElement'],
['the-middle', 'didDestroyElement'],
['the-bottom', 'didDestroyElement'],
['the-top', 'willDestroy'],
['the-middle', 'willDestroy'],
['the-bottom', 'willDestroy']
);

this.assertRegisteredViews('after destroy');
Expand Down Expand Up @@ -608,7 +620,17 @@ class LifeCycleHooksTest extends RenderingTest {
['the-second-child', 'willDestroyElement'],
['the-second-child', 'willClearRender'],
['the-last-child', 'willDestroyElement'],
['the-last-child', 'willClearRender']
['the-last-child', 'willClearRender'],

['the-parent', 'didDestroyElement'],
['the-first-child', 'didDestroyElement'],
['the-second-child', 'didDestroyElement'],
['the-last-child', 'didDestroyElement'],

['the-parent', 'willDestroy'],
['the-first-child', 'willDestroy'],
['the-second-child', 'willDestroy'],
['the-last-child', 'willDestroy']
);

this.assertRegisteredViews('after destroy');
Expand Down Expand Up @@ -747,7 +769,13 @@ class LifeCycleHooksTest extends RenderingTest {
['the-middle', 'willDestroyElement'],
['the-middle', 'willClearRender'],
['the-bottom', 'willDestroyElement'],
['the-bottom', 'willClearRender']
['the-bottom', 'willClearRender'],
['the-top', 'didDestroyElement'],
['the-middle', 'didDestroyElement'],
['the-bottom', 'didDestroyElement'],
['the-top', 'willDestroy'],
['the-middle', 'willDestroy'],
['the-bottom', 'willDestroy']
);

this.assertRegisteredViews('after destroy');
Expand All @@ -757,12 +785,14 @@ class LifeCycleHooksTest extends RenderingTest {
['@test components rendered from `{{each}}` have correct life-cycle hooks to be called']() {
let { invoke } = this.boundHelpers;

this.registerComponent('nested-item', { template: `{{yield}}` });

this.registerComponent('an-item', { template: strip`
<div>Item: {{count}}</div>
{{#nested-item}}Item: {{count}}{{/nested-item}}
` });

this.registerComponent('no-items', { template: strip`
<div>Nothing to see here</div>
{{#nested-item}}Nothing to see here{{/nested-item}}
` });

this.render(strip`
Expand All @@ -783,12 +813,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']
];
Expand All @@ -813,39 +849,91 @@ 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');

this.assertHooks(
'reset to empty array',

['an-item', 'willDestroyElement'],
['an-item', 'willClearRender'],
['nested-item', 'willDestroyElement'],
['nested-item', 'willClearRender'],
['an-item', 'willDestroyElement'],
['an-item', 'willClearRender'],
['nested-item', 'willDestroyElement'],
['nested-item', 'willClearRender'],
['an-item', 'willDestroyElement'],
['an-item', 'willClearRender'],
['nested-item', 'willDestroyElement'],
['nested-item', 'willClearRender'],
['an-item', 'willDestroyElement'],
['an-item', 'willClearRender'],
['nested-item', 'willDestroyElement'],
['nested-item', 'willClearRender'],
['an-item', 'willDestroyElement'],
['an-item', 'willClearRender'],
['nested-item', 'willDestroyElement'],
['nested-item', 'willClearRender'],

['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'],

['an-item', 'didDestroyElement'],
['nested-item', 'didDestroyElement'],
['an-item', 'didDestroyElement'],
['nested-item', 'didDestroyElement'],
['an-item', 'didDestroyElement'],
['nested-item', 'didDestroyElement'],
['an-item', 'didDestroyElement'],
['nested-item', 'didDestroyElement'],
['an-item', 'didDestroyElement'],
['nested-item', 'didDestroyElement'],

['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', 'willClearRender']
['no-items', 'willClearRender'],
['nested-item', 'willDestroyElement'],
['nested-item', 'willClearRender'],

['no-items', 'didDestroyElement'],
['nested-item', 'didDestroyElement'],

['no-items', 'willDestroy'],
['nested-item', 'willDestroy']
);

this.assertRegisteredViews('after destroy');
Expand Down
2 changes: 1 addition & 1 deletion packages/ember-views/lib/views/states/has_element.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ assign(hasElement, {
},

destroy(view) {
view.renderer.remove(view, true);
view.renderer.remove(view);
},

// Handle events from `Ember.EventDispatcher`
Expand Down

0 comments on commit 945d81a

Please sign in to comment.