Skip to content

[BUGFIX beta] Cleanup view teardown. #13775

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 1, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/ember-glimmer/lib/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,9 @@ class Renderer {
_renderResult.destroy();
}

view.destroy();
if (!view.isDestroying) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we guard against both isDestroying and isDestroyed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't unset isDestroying when we set isDestroyed but I can add that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, then its not needed. Seems good

view.destroy();
}
}

componentInitAttrs() {
Expand Down
1 change: 1 addition & 0 deletions packages/ember-glimmer/lib/syntax/curly-component.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ class CurlyComponentManager {

aliasIdToElementId(args, props);

props.parentView = parentView;
props.renderer = parentView.renderer;
props[HAS_BLOCK] = hasBlock;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -310,10 +310,15 @@ moduleFor('Components test: dynamic components', class extends RenderingTest {

['@test component helper destroys underlying component when it is swapped out'](assert) {
let destroyed = { 'foo-bar': 0, 'foo-bar-baz': 0 };
let testContext = this;

this.registerComponent('foo-bar', {
template: 'hello from foo-bar',
ComponentClass: Component.extend({
willDestroyElement() {
assert.equal(testContext.$(`#${this.elementId}`).length, 1, 'element is still attached to the document');
},

willDestroy() {
this._super();
destroyed['foo-bar']++;
Expand Down
160 changes: 153 additions & 7 deletions packages/ember-glimmer/tests/integration/components/life-cycle-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,15 @@ class LifeCycleHooksTest extends RenderingTest {
super();
this.hooks = [];
this.components = {};
this.teardownAssertions = [];
}

teardown() {
super();
this.assertHooks(
'destroy',
['the-top', 'willDestroyElement'],
['the-middle', 'willDestroyElement'],
['the-bottom', 'willDestroyElement']
);
super.teardown();

for (let i = 0; i < this.teardownAssertions.length; i++) {
this.teardownAssertions[i]();
}
}

/* abstract */
Expand Down Expand Up @@ -51,49 +50,90 @@ class LifeCycleHooksTest extends RenderingTest {
this.hooks.push(hook(name, hookName, args));
};

let assertParentView = (hookName, instance) => {
if (!instance.parentView) {
this.assert.ok(false, `parentView 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}`);
}

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

let assertNoElement = (hookName, instance) => {
if (instance.element) {
this.assert.ok(false, `element should not be present in ${hookName}`);
}
};

let ComponentClass = this.ComponentClass.extend({
init() {
expectDeprecation(() => { this._super(...arguments); },
/didInitAttrs called/);

pushHook('init');
pushComponent(this);
assertParentView('init', this);
assertNoElement('init', this);
},

didInitAttrs(options) {
pushHook('didInitAttrs', options);
assertParentView('didInitAttrs', this);
assertNoElement('didInitAttrs', this);
},

didUpdateAttrs(options) {
pushHook('didUpdateAttrs', options);
assertParentView('didUpdateAttrs', this);
},

willUpdate(options) {
pushHook('willUpdate', options);
assertParentView('willUpdate', this);
},

didReceiveAttrs(options) {
pushHook('didReceiveAttrs', options);
assertParentView('didReceiveAttrs', this);
},

willRender() {
pushHook('willRender');
assertParentView('willRender', this);
},

didRender() {
pushHook('didRender');
assertParentView('didRender', this);
assertElement('didRender', this);
},

didInsertElement() {
pushHook('didInsertElement');
assertParentView('didInsertElement', this);
assertElement('didInsertElement', this);
},

didUpdate(options) {
pushHook('didUpdate', options);
assertParentView('didUpdate', this);
assertElement('didUpdate', this);
},

willDestroyElement() {
pushHook('willDestroyElement');
assertParentView('willDestroyElement', this);
assertElement('willDestroyElement', this);
}
});

Expand Down Expand Up @@ -341,6 +381,15 @@ class LifeCycleHooksTest extends RenderingTest {
['the-top', 'didRender']

);

this.teardownAssertions.push(() => {
this.assertHooks(
'destroy',
['the-top', 'willDestroyElement'],
['the-middle', 'willDestroyElement'],
['the-bottom', 'willDestroyElement']
);
});
}

['@test passing values through attrs causes lifecycle hooks to fire if the attribute values have changed']() {
Expand Down Expand Up @@ -504,8 +553,105 @@ class LifeCycleHooksTest extends RenderingTest {
} else {
this.assertHooks('after no-op rernder (root)');
}

this.teardownAssertions.push(() => {
this.assertHooks(
'destroy',
['the-top', 'willDestroyElement'],
['the-middle', 'willDestroyElement'],
['the-bottom', 'willDestroyElement']
);
});
}

['@test components rendered from `{{each}}` have correct life-cycle hooks to be called']() {
let { invoke } = this.boundHelpers;

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

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

this.render(strip`
{{#each items as |item|}}
${invoke('an-item', { count: expr('item') })}
{{else}}
${invoke('no-items')}
{{/each}}
`, {
items: [1, 2, 3, 4, 5]
});

this.assertText('Item: 1Item: 2Item: 3Item: 4Item: 5');

let initialHooks = (count) => {
return [
['an-item', 'init'],
['an-item', 'didInitAttrs', { attrs: { count } }],
['an-item', 'didReceiveAttrs', { newAttrs: { count } }],
['an-item', 'willRender']
];
};

let initialAfterRenderHooks = (count) => {
return [
['an-item', 'didInsertElement'],
['an-item', 'didRender']
];
};

this.assertHooks(

'after initial render',

// Sync hooks
...initialHooks(1),
...initialHooks(2),
...initialHooks(3),
...initialHooks(4),
...initialHooks(5),

// Async hooks
...initialAfterRenderHooks(5),
...initialAfterRenderHooks(4),
...initialAfterRenderHooks(3),
...initialAfterRenderHooks(2),
...initialAfterRenderHooks(1)
);

this.runTask(() => set(this.context, 'items', []));

this.assertText('Nothing to see here');

this.assertHooks(
'reset to empty array',

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

['no-items', 'init'],
['no-items', 'didInitAttrs', { attrs: { } }],
['no-items', 'didReceiveAttrs', { newAttrs: { } }],
['no-items', 'willRender'],

['no-items', 'didInsertElement'],
['no-items', 'didRender']
);

this.teardownAssertions.push(() => {
this.assertHooks(
'destroy',

['no-items', 'willDestroyElement']
);
});
}
}

moduleFor('Components test: lifecycle hooks (curly components)', class extends LifeCycleHooksTest {
Expand Down
13 changes: 10 additions & 3 deletions packages/ember-htmlbars/lib/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ const Component = View.extend(TargetActionSupport, ActionSupport, {
let attr = this._renderNode.childNodes.filter(node => node.attrName === name)[0];
if (!attr) { return null; }
return attr.getContent();
}
},

/**
Returns true when the component was invoked with a block template.
Expand Down Expand Up @@ -359,6 +359,7 @@ const Component = View.extend(TargetActionSupport, ActionSupport, {
@public
@since 1.13.0
*/
didReceiveAttrs() {},

/**
Called when the attributes passed into the component have been updated.
Expand All @@ -379,6 +380,7 @@ const Component = View.extend(TargetActionSupport, ActionSupport, {
@public
@since 1.13.0
*/
didRender() {},

/**
Called after a component has been rendered, both on initial render and
Expand All @@ -397,6 +399,7 @@ const Component = View.extend(TargetActionSupport, ActionSupport, {
@public
@since 1.13.0
*/
willRender() {},

/**
Called before a component has been rendered, both on initial render and
Expand All @@ -415,6 +418,7 @@ const Component = View.extend(TargetActionSupport, ActionSupport, {
@public
@since 1.13.0
*/
didUpdateAttrs() {},

/**
Called when the attributes passed into the component have been changed.
Expand All @@ -433,6 +437,7 @@ const Component = View.extend(TargetActionSupport, ActionSupport, {
@public
@since 1.13.0
*/
willUpdate() {},

/**
Called when the component is about to update and rerender itself.
Expand All @@ -447,10 +452,11 @@ const Component = View.extend(TargetActionSupport, ActionSupport, {
Called when the component has updated and rerendered itself.
Called only during a rerender, not during an initial render.

@event didUpdate
@method didUpdate
@public
@since 1.13.0
*/
didUpdate() {}

/**
Called when the component has updated and rerendered itself.
Expand All @@ -465,7 +471,8 @@ const Component = View.extend(TargetActionSupport, ActionSupport, {
Component[NAME_KEY] = 'Ember.Component';

Component.reopenClass({
isComponentFactory: true
isComponentFactory: true,
positionalParams: []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tell me more

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I mean to move the doc here too, but the issue is if (positionalParams) { will optimize to false because this is uncommon but not exceptional and the branch deopts, I was doing a lot of clean as I go, I can remove that stuff and make it a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the same with all the hooks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, and this is already prototype only type thing. Seems good thanks

});

export default Component;
6 changes: 3 additions & 3 deletions packages/ember-htmlbars/lib/hooks/destroy-render-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
@module ember
@submodule ember-htmlbars
*/

export default function destroyRenderNode(renderNode) {
if (renderNode.emberView) {
renderNode.emberView.destroy();
let view = renderNode.emberView;
if (view) {
view.renderer.remove(view, true);
}

let streamUnsubscribers = renderNode.streamUnsubscribers;
Expand Down
11 changes: 1 addition & 10 deletions packages/ember-htmlbars/lib/morphs/morph.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,21 +29,12 @@ proto.HTMLBarsMorph$constructor = HTMLBarsMorph;
proto.HTMLBarsMorph$clear = HTMLBarsMorph.prototype.clear;

proto.addDestruction = function(toDestroy) {
// called from Router.prototype._connectActiveComponentNode for {{render}}
this.emberToDestroy = this.emberToDestroy || [];
this.emberToDestroy.push(toDestroy);
};

proto.cleanup = function() {
let view = this.emberView;

if (view) {
let parentView = view.parentView;

if (parentView && view.ownerView._destroyingSubtreeForView === parentView) {
parentView.removeChild(view);
}
}

let toDestroy = this.emberToDestroy;

if (toDestroy) {
Expand Down
Loading