From 3d4cae954beaa421cda1ac10f10d0be5a6e1a226 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Gro=C3=9Fe?= Date: Tue, 20 Jun 2017 10:50:50 +0200 Subject: [PATCH 1/3] Removes Node after invoking componentWillUnmount on all children - changes the order of the unmounting steps, so the containing node is removed at last - replaces the children only when no unmounting is needed - accidentally discovered, that replaceChild is only needed when no unmounting is needed, all test pass and the behavior is as expected. But someone with more internal knowledge of preact should look over it. --- src/vdom/component.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/vdom/component.js b/src/vdom/component.js index b64ae40087b..a78db40e9f5 100644 --- a/src/vdom/component.js +++ b/src/vdom/component.js @@ -144,12 +144,18 @@ export function renderComponent(component, opts, mountAll, isChild) { if (initialBase && base!==initialBase && inst!==initialChildComponent) { let baseParent = initialBase.parentNode; if (baseParent && base!==baseParent) { - baseParent.replaceChild(base, initialBase); if (!toUnmount) { + baseParent.replaceChild(base, initialBase); initialBase._component = null; recollectNodeTree(initialBase, false); } + // @TODO Formerly the initialBase was always replaced by the base + // but somehow all tests passes also if its only called at not unmounting. + // These else condition was the original fix. + // else { + // baseParent.insertBefore(base, initialBase); + // } } } @@ -264,10 +270,11 @@ export function unmountComponent(component) { component.nextBase = base; - removeNode(base); collectComponent(component); - removeChildren(base); + + // Remove Node as last, so all childs exists when componentWillUnmount is called + removeNode(base); } if (component.__ref) component.__ref(null); From bc6d2051881ae1a19bbc08c3a855b426ba8c7c1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Gro=C3=9Fe?= Date: Tue, 20 Jun 2017 10:52:17 +0200 Subject: [PATCH 2/3] Removes Node after recuring through all children --- src/vdom/diff.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/vdom/diff.js b/src/vdom/diff.js index 44aebc12c66..1409ab79045 100644 --- a/src/vdom/diff.js +++ b/src/vdom/diff.js @@ -262,11 +262,12 @@ export function recollectNodeTree(node, unmountOnly) { // (this is part of the React spec, and smart for unsetting references) if (node[ATTR_KEY]!=null && node[ATTR_KEY].ref) node[ATTR_KEY].ref(null); + removeChildren(node); + + // Remove Node as last, so all childs exists when componentWillUnmount is called if (unmountOnly===false || node[ATTR_KEY]==null) { removeNode(node); } - - removeChildren(node); } } From 479a0480c9bce8366753b1fa77694c51841019a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Gro=C3=9Fe?= Date: Tue, 20 Jun 2017 10:52:53 +0200 Subject: [PATCH 3/3] Extends the test to a more nested element --- test/browser/lifecycle.js | 56 ++++++++++++++++++++++++++++++--------- 1 file changed, 44 insertions(+), 12 deletions(-) diff --git a/test/browser/lifecycle.js b/test/browser/lifecycle.js index 6493473c3e2..84c46df40b2 100644 --- a/test/browser/lifecycle.js +++ b/test/browser/lifecycle.js @@ -456,42 +456,74 @@ describe('Lifecycle methods', () => { expect(document.getElementById('InnerDiv'), 'Inner componentDidMount').to.exist; } componentWillUnmount() { - // @TODO Component mounted into elements (non-components) - // are currently unmounted after those elements, so their - // DOM is unmounted prior to the method being called. - //expect(document.getElementById('InnerDiv'), 'Inner componentWillUnmount').to.exist; + expect(document.getElementById('InnerDiv'), 'Inner componentWillUnmount').to.exist; setTimeout( () => { expect(document.getElementById('InnerDiv'), 'Inner after componentWillUnmount').to.not.exist; }, 0); } render() { - return
; + return
+ +
; + } + } + + class Innerest extends Component { + componentWillMount() { + expect(document.getElementById('InnerestDiv'), 'Innerest componentWillMount').to.not.exist; + } + componentDidMount() { + expect(document.getElementById('InnerestDiv'), 'Innerest componentDidMount').to.exist; + } + componentWillUnmount() { + expect(document.getElementById('InnerestDiv'), 'Innerest componentWillUnmount').to.exist; + setTimeout( () => { + expect(document.getElementById('InnerestDiv'), 'Innerest after componentWillUnmount').to.not.exist; + }, 0); + } + + render() { + return
; } } let proto = Inner.prototype; + let proto_innerest = Innerest.prototype; let spies = ['componentWillMount', 'componentDidMount', 'componentWillUnmount']; spies.forEach( s => sinon.spy(proto, s) ); + spies.forEach( s => sinon.spy(proto_innerest, s) ); let reset = () => spies.forEach( s => proto[s].reset() ); + let reset_innerest = () => spies.forEach( s => proto_innerest[s].reset() ); render(, scratch); - expect(proto.componentWillMount).to.have.been.called; - expect(proto.componentWillMount).to.have.been.calledBefore(proto.componentDidMount); - expect(proto.componentDidMount).to.have.been.called; + expect(proto.componentWillMount, 'Inner componentWillMount').to.have.been.called; + expect(proto.componentWillMount, 'Inner componentWillMount').to.have.been.calledBefore(proto.componentDidMount); + expect(proto.componentDidMount, 'Inner componentDidMount').to.have.been.called; + + expect(proto_innerest.componentWillMount, 'Innerest componentWillMount').to.have.been.called; + expect(proto_innerest.componentWillMount, 'Innerest componentWillMount').to.have.been.calledBefore(proto_innerest.componentDidMount); + expect(proto_innerest.componentDidMount, 'Innerest componentDidMount').to.have.been.called; reset(); + reset_innerest(); setState({ show:false }); - expect(proto.componentWillUnmount).to.have.been.called; + expect(proto.componentWillUnmount, 'Inner componentWillUnmount').to.have.been.called; + expect(proto_innerest.componentWillUnmount, 'Innerest componentWillUnmount').to.have.been.called; reset(); + reset_innerest(); setState({ show:true }); - expect(proto.componentWillMount).to.have.been.called; - expect(proto.componentWillMount).to.have.been.calledBefore(proto.componentDidMount); - expect(proto.componentDidMount).to.have.been.called; + expect(proto.componentWillMount, 'Inner componentWillMount').to.have.been.called; + expect(proto.componentWillMount, 'Inner componentWillMount').to.have.been.calledBefore(proto.componentDidMount); + expect(proto.componentDidMount, 'Inner componentDidMount').to.have.been.called; + + expect(proto_innerest.componentWillMount, 'Innerest componentWillMount').to.have.been.called; + expect(proto_innerest.componentWillMount, 'Innerest componentWillMount').to.have.been.calledBefore(proto_innerest.componentDidMount); + expect(proto_innerest.componentDidMount, 'Innerest componentDidMount').to.have.been.called; }); it('should remove this.base for HOC', () => {