Skip to content

Commit

Permalink
[BUGFIX beta] WIP Cleanup view teardown.
Browse files Browse the repository at this point in the history
- [ ] add tests that assure counts of hooks in all dynamic scenarios
  • Loading branch information
krisselden committed Jun 30, 2016
1 parent b83b166 commit 44e7ecd
Show file tree
Hide file tree
Showing 12 changed files with 78 additions and 94 deletions.
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) {
view.destroy();
}
}

componentInitAttrs() {
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: []
});

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
2 changes: 1 addition & 1 deletion packages/ember-htmlbars/lib/morphs/morph.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ 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);
};
Expand All @@ -38,7 +39,6 @@ proto.cleanup = function() {

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

if (parentView && view.ownerView._destroyingSubtreeForView === parentView) {
parentView.removeChild(view);
}
Expand Down
65 changes: 26 additions & 39 deletions packages/ember-htmlbars/lib/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,6 @@ Renderer.prototype.willInsertElement = function (view) {
if (view.trigger) { view.trigger('willInsertElement'); }
}; // Will place into DOM.

Renderer.prototype.setAttrs = function (view, attrs) {
set(view, 'attrs', attrs);
}; // Set attrs the first time.

Renderer.prototype.componentInitAttrs = function (component, attrs) {
component.trigger('didInitAttrs', { attrs });
component.trigger('didReceiveAttrs', { newAttrs: attrs });
Expand All @@ -191,10 +187,6 @@ Renderer.prototype.didRender = function (view) {
if (view.trigger) { view.trigger('didRender'); }
};

Renderer.prototype.updateAttrs = function (view, attrs) {
this.setAttrs(view, attrs);
}; // Setting new attrs.

Renderer.prototype.componentUpdateAttrs = function (component, newAttrs) {
let oldAttrs = null;

Expand Down Expand Up @@ -244,51 +236,46 @@ Renderer.prototype.rerender = function (view) {
};

Renderer.prototype.remove = function (view, shouldDestroy) {
this.willDestroyElement(view);

view._willRemoveElement = true;
run.schedule('render', this, this.renderElementRemoval, view);
};

Renderer.prototype.renderElementRemoval =
function Renderer_renderElementRemoval(view) {
// Use the _willRemoveElement flag to avoid mulitple removal attempts in
// case many have been scheduled. This should be more performant than using
// `scheduleOnce`.
if (view._willRemoveElement) {
view._willRemoveElement = false;
let renderNode = view._renderNode;
view._renderNode = null;
if (renderNode) {
renderNode.emberView = null;
this.willDestroyElement(view);
view._transitionTo('destroying');

if (view._renderNode && view.element && view.element.parentNode) {
view._renderNode.clear();
}
this.didDestroyElement(view);
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
let lastResult = view.lastResult;
if (lastResult) {
view.lastResult = null;
lastResult.destroy();
}

Renderer.prototype.willRemoveElement = function (/*view*/) {};
if (shouldDestroy && !view.isDestroying) {
view.destroy();
}
};

Renderer.prototype.willDestroyElement = function (view) {
if (view.trigger) {
view.trigger('willDestroyElement');
view.trigger('willClearRender');
}

if (view._transitionTo) {
view._transitionTo('destroying');
}
};

Renderer.prototype.didDestroyElement = function (view) {
view.element = null;

// Views that are being destroyed should never go back to the preRender state.
// However if we're just destroying an element on a view (as is the case when
// using View#remove) then the view should go to a preRender state so that
// it can be rendered again later.
if (view._state !== 'destroying' && view._transitionTo) {
view._transitionTo('preRender');
}

if (view.trigger) {
view.trigger('didDestroyElement');
}
Expand Down
37 changes: 16 additions & 21 deletions packages/ember-views/lib/mixins/view_support.js
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,22 @@ export default Mixin.create({
@private
*/
destroyElement() {
return this._currentState.destroyElement(this);
this._currentState.destroyElement(this);
return this;
},

/**
You must call `destroy` on a view to destroy the view (and all of its
child views). This will remove the view from any parent node, then make
sure that the DOM element managed by the view can be released by the
memory manager.
@method destroy
@private
*/
destroy() {
this._super(...arguments);
this._currentState.destroy(this);
},

/**
Expand Down Expand Up @@ -524,26 +539,6 @@ export default Mixin.create({
}
},

/**
You must call `destroy` on a view to destroy the view (and all of its
child views). This will remove the view from any parent node, then make
sure that the DOM element managed by the view can be released by the
memory manager.
@method destroy
@private
*/
destroy() {
if (!this._super(...arguments)) { return; }

// Destroy HTMLbars template
if (this.lastResult) {
this.lastResult.destroy();
}

return this;
},

// .......................................................
// EVENT HANDLING
//
Expand Down
20 changes: 9 additions & 11 deletions packages/ember-views/lib/views/core_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,15 @@ const CoreView = EmberObject.extend(Evented, ActionHandler, {
this._super(...arguments);
this._state = 'preRender';
this._currentState = this._states.preRender;
this._willInsert = false;
this._renderNode = null;
this.lastResult = null;
this._dispatching = null;
this._destroyingSubtreeForView = null;
this._isDispatchingAttrs = false;
this._isVisible = false;
this.element = null;
this.env = null;
this._isVisible = get(this, 'isVisible');

// Fallback for legacy cases where the view was created directly
Expand All @@ -50,9 +59,6 @@ const CoreView = EmberObject.extend(Evented, ActionHandler, {
renderer = renderer || InteractiveRenderer.create({ dom: new DOMHelper() });
this.renderer = renderer;
}

this._destroyingSubtreeForView = null;
this._dispatching = null;
},

/**
Expand Down Expand Up @@ -97,14 +103,6 @@ const CoreView = EmberObject.extend(Evented, ActionHandler, {

has(name) {
return typeOf(this[name]) === 'function' || this._super(name);
},

destroy() {
if (!this._super(...arguments)) { return; }

this._currentState.cleanup(this);

return this;
}
});

Expand Down
3 changes: 2 additions & 1 deletion packages/ember-views/lib/views/states/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,10 @@ export default {
return true; // continue event propagation
},

cleanup() { } ,
destroyElement() { },

destroy() { },

rerender(view) {
view.renderer.ensureViewNotRendering(view);
}
Expand Down
1 change: 0 additions & 1 deletion packages/ember-views/lib/views/states/destroying.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,3 @@ assign(destroying, {
});

export default destroying;

13 changes: 4 additions & 9 deletions packages/ember-views/lib/views/states/has_element.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,12 @@ assign(hasElement, {
view.renderer.rerender(view);
},

cleanup(view) {
view._currentState.destroyElement(view);
},

// once the view is already in the DOM, destroying it removes it
// from the DOM, nukes its element, and puts it back into the
// preRender state if inDOM.

destroyElement(view) {
view.renderer.remove(view, false);
return view;
},

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

// Handle events from `Ember.EventDispatcher`
Expand Down
4 changes: 3 additions & 1 deletion packages/ember-views/lib/views/states/in_dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ assign(inDOM, {
},

exit(view) {
view._unregister();
if (view.tagName !== '') {
view._unregister();
}
}
});

Expand Down
4 changes: 1 addition & 3 deletions packages/ember-views/tests/system/event_dispatcher_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,7 @@ QUnit.test('should not dispatch events to views not inDOM', function() {
let $element = view.$();

run(() => {
// TODO change this test not to use private API
// Force into preRender
view.renderer.remove(view, false, true);
view.destroyElement();
});

$element.trigger('mousedown');
Expand Down

0 comments on commit 44e7ecd

Please sign in to comment.