Skip to content

Commit e81233a

Browse files
krisseldenkrisselden
authored
Merge pull request #13775 from emberjs/view-destroy-cleanup
[BUGFIX beta] Cleanup view teardown.
2 parents 2c63b75 + 9228bb8 commit e81233a

File tree

15 files changed

+237
-110
lines changed

15 files changed

+237
-110
lines changed

‎packages/ember-glimmer/lib/renderer.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,9 @@ class Renderer {
192192
_renderResult.destroy();
193193
}
194194

195-
view.destroy();
195+
if (!view.isDestroying) {
196+
view.destroy();
197+
}
196198
}
197199

198200
componentInitAttrs() {

‎packages/ember-glimmer/lib/syntax/curly-component.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ class CurlyComponentManager {
7272

7373
aliasIdToElementId(args, props);
7474

75+
props.parentView = parentView;
7576
props.renderer = parentView.renderer;
7677
props[HAS_BLOCK] = hasBlock;
7778

‎packages/ember-glimmer/tests/integration/components/dynamic-components-test.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,10 +310,15 @@ moduleFor('Components test: dynamic components', class extends RenderingTest {
310310

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

314315
this.registerComponent('foo-bar', {
315316
template: 'hello from foo-bar',
316317
ComponentClass: Component.extend({
318+
willDestroyElement() {
319+
assert.equal(testContext.$(`#${this.elementId}`).length, 1, 'element is still attached to the document');
320+
},
321+
317322
willDestroy() {
318323
this._super();
319324
destroyed['foo-bar']++;

‎packages/ember-glimmer/tests/integration/components/life-cycle-test.js

Lines changed: 153 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,15 @@ class LifeCycleHooksTest extends RenderingTest {
88
super();
99
this.hooks = [];
1010
this.components = {};
11+
this.teardownAssertions = [];
1112
}
1213

1314
teardown() {
14-
super();
15-
this.assertHooks(
16-
'destroy',
17-
['the-top', 'willDestroyElement'],
18-
['the-middle', 'willDestroyElement'],
19-
['the-bottom', 'willDestroyElement']
20-
);
15+
super.teardown();
16+
17+
for (let i = 0; i < this.teardownAssertions.length; i++) {
18+
this.teardownAssertions[i]();
19+
}
2120
}
2221

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

53+
let assertParentView = (hookName, instance) => {
54+
if (!instance.parentView) {
55+
this.assert.ok(false, `parentView should be present in ${hookName}`);
56+
}
57+
};
58+
59+
let assertElement = (hookName, instance) => {
60+
if (instance.tagName === '') { return; }
61+
62+
if (!instance.element) {
63+
this.assert.ok(false, `element property should be present on ${instance} during ${hookName}`);
64+
}
65+
66+
let inDOM = this.$(`#${instance.elementId}`)[0];
67+
if (!inDOM) {
68+
this.assert.ok(false, `element for ${instance} should be in the DOM during ${hookName}`);
69+
}
70+
};
71+
72+
let assertNoElement = (hookName, instance) => {
73+
if (instance.element) {
74+
this.assert.ok(false, `element should not be present in ${hookName}`);
75+
}
76+
};
77+
5478
let ComponentClass = this.ComponentClass.extend({
5579
init() {
5680
expectDeprecation(() => { this._super(...arguments); },
5781
/didInitAttrs called/);
5882

5983
pushHook('init');
6084
pushComponent(this);
85+
assertParentView('init', this);
86+
assertNoElement('init', this);
6187
},
6288

6389
didInitAttrs(options) {
6490
pushHook('didInitAttrs', options);
91+
assertParentView('didInitAttrs', this);
92+
assertNoElement('didInitAttrs', this);
6593
},
6694

6795
didUpdateAttrs(options) {
6896
pushHook('didUpdateAttrs', options);
97+
assertParentView('didUpdateAttrs', this);
6998
},
7099

71100
willUpdate(options) {
72101
pushHook('willUpdate', options);
102+
assertParentView('willUpdate', this);
73103
},
74104

75105
didReceiveAttrs(options) {
76106
pushHook('didReceiveAttrs', options);
107+
assertParentView('didReceiveAttrs', this);
77108
},
78109

79110
willRender() {
80111
pushHook('willRender');
112+
assertParentView('willRender', this);
81113
},
82114

83115
didRender() {
84116
pushHook('didRender');
117+
assertParentView('didRender', this);
118+
assertElement('didRender', this);
85119
},
86120

87121
didInsertElement() {
88122
pushHook('didInsertElement');
123+
assertParentView('didInsertElement', this);
124+
assertElement('didInsertElement', this);
89125
},
90126

91127
didUpdate(options) {
92128
pushHook('didUpdate', options);
129+
assertParentView('didUpdate', this);
130+
assertElement('didUpdate', this);
93131
},
94132

95133
willDestroyElement() {
96134
pushHook('willDestroyElement');
135+
assertParentView('willDestroyElement', this);
136+
assertElement('willDestroyElement', this);
97137
}
98138
});
99139

@@ -341,6 +381,15 @@ class LifeCycleHooksTest extends RenderingTest {
341381
['the-top', 'didRender']
342382

343383
);
384+
385+
this.teardownAssertions.push(() => {
386+
this.assertHooks(
387+
'destroy',
388+
['the-top', 'willDestroyElement'],
389+
['the-middle', 'willDestroyElement'],
390+
['the-bottom', 'willDestroyElement']
391+
);
392+
});
344393
}
345394

346395
['@test passing values through attrs causes lifecycle hooks to fire if the attribute values have changed']() {
@@ -504,8 +553,105 @@ class LifeCycleHooksTest extends RenderingTest {
504553
} else {
505554
this.assertHooks('after no-op rernder (root)');
506555
}
556+
557+
this.teardownAssertions.push(() => {
558+
this.assertHooks(
559+
'destroy',
560+
['the-top', 'willDestroyElement'],
561+
['the-middle', 'willDestroyElement'],
562+
['the-bottom', 'willDestroyElement']
563+
);
564+
});
507565
}
508566

567+
['@test components rendered from `{{each}}` have correct life-cycle hooks to be called']() {
568+
let { invoke } = this.boundHelpers;
569+
570+
this.registerComponent('an-item', { template: strip`
571+
<div>Item: {{count}}</div>
572+
` });
573+
574+
this.registerComponent('no-items', { template: strip`
575+
<div>Nothing to see here</div>
576+
` });
577+
578+
this.render(strip`
579+
{{#each items as |item|}}
580+
${invoke('an-item', { count: expr('item') })}
581+
{{else}}
582+
${invoke('no-items')}
583+
{{/each}}
584+
`, {
585+
items: [1, 2, 3, 4, 5]
586+
});
587+
588+
this.assertText('Item: 1Item: 2Item: 3Item: 4Item: 5');
589+
590+
let initialHooks = (count) => {
591+
return [
592+
['an-item', 'init'],
593+
['an-item', 'didInitAttrs', { attrs: { count } }],
594+
['an-item', 'didReceiveAttrs', { newAttrs: { count } }],
595+
['an-item', 'willRender']
596+
];
597+
};
598+
599+
let initialAfterRenderHooks = (count) => {
600+
return [
601+
['an-item', 'didInsertElement'],
602+
['an-item', 'didRender']
603+
];
604+
};
605+
606+
this.assertHooks(
607+
608+
'after initial render',
609+
610+
// Sync hooks
611+
...initialHooks(1),
612+
...initialHooks(2),
613+
...initialHooks(3),
614+
...initialHooks(4),
615+
...initialHooks(5),
616+
617+
// Async hooks
618+
...initialAfterRenderHooks(5),
619+
...initialAfterRenderHooks(4),
620+
...initialAfterRenderHooks(3),
621+
...initialAfterRenderHooks(2),
622+
...initialAfterRenderHooks(1)
623+
);
624+
625+
this.runTask(() => set(this.context, 'items', []));
626+
627+
this.assertText('Nothing to see here');
628+
629+
this.assertHooks(
630+
'reset to empty array',
631+
632+
['an-item', 'willDestroyElement'],
633+
['an-item', 'willDestroyElement'],
634+
['an-item', 'willDestroyElement'],
635+
['an-item', 'willDestroyElement'],
636+
['an-item', 'willDestroyElement'],
637+
638+
['no-items', 'init'],
639+
['no-items', 'didInitAttrs', { attrs: { } }],
640+
['no-items', 'didReceiveAttrs', { newAttrs: { } }],
641+
['no-items', 'willRender'],
642+
643+
['no-items', 'didInsertElement'],
644+
['no-items', 'didRender']
645+
);
646+
647+
this.teardownAssertions.push(() => {
648+
this.assertHooks(
649+
'destroy',
650+
651+
['no-items', 'willDestroyElement']
652+
);
653+
});
654+
}
509655
}
510656

511657
moduleFor('Components test: lifecycle hooks (curly components)', class extends LifeCycleHooksTest {

‎packages/ember-htmlbars/lib/component.js

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ const Component = View.extend(TargetActionSupport, ActionSupport, {
193193
let attr = this._renderNode.childNodes.filter(node => node.attrName === name)[0];
194194
if (!attr) { return null; }
195195
return attr.getContent();
196-
}
196+
},
197197

198198
/**
199199
Returns true when the component was invoked with a block template.
@@ -359,6 +359,7 @@ const Component = View.extend(TargetActionSupport, ActionSupport, {
359359
@public
360360
@since 1.13.0
361361
*/
362+
didReceiveAttrs() {},
362363

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

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

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

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

437442
/**
438443
Called when the component is about to update and rerender itself.
@@ -447,10 +452,11 @@ const Component = View.extend(TargetActionSupport, ActionSupport, {
447452
Called when the component has updated and rerendered itself.
448453
Called only during a rerender, not during an initial render.
449454
450-
@event didUpdate
455+
@method didUpdate
451456
@public
452457
@since 1.13.0
453458
*/
459+
didUpdate() {}
454460

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

467473
Component.reopenClass({
468-
isComponentFactory: true
474+
isComponentFactory: true,
475+
positionalParams: []
469476
});
470477

471478
export default Component;

‎packages/ember-htmlbars/lib/hooks/destroy-render-node.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,10 @@
22
@module ember
33
@submodule ember-htmlbars
44
*/
5-
65
export default function destroyRenderNode(renderNode) {
7-
if (renderNode.emberView) {
8-
renderNode.emberView.destroy();
6+
let view = renderNode.emberView;
7+
if (view) {
8+
view.renderer.remove(view, true);
99
}
1010

1111
let streamUnsubscribers = renderNode.streamUnsubscribers;

‎packages/ember-htmlbars/lib/morphs/morph.js

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,21 +29,12 @@ proto.HTMLBarsMorph$constructor = HTMLBarsMorph;
2929
proto.HTMLBarsMorph$clear = HTMLBarsMorph.prototype.clear;
3030

3131
proto.addDestruction = function(toDestroy) {
32+
// called from Router.prototype._connectActiveComponentNode for {{render}}
3233
this.emberToDestroy = this.emberToDestroy || [];
3334
this.emberToDestroy.push(toDestroy);
3435
};
3536

3637
proto.cleanup = function() {
37-
let view = this.emberView;
38-
39-
if (view) {
40-
let parentView = view.parentView;
41-
42-
if (parentView && view.ownerView._destroyingSubtreeForView === parentView) {
43-
parentView.removeChild(view);
44-
}
45-
}
46-
4738
let toDestroy = this.emberToDestroy;
4839

4940
if (toDestroy) {

0 commit comments

Comments
 (0)