Merge pull request #1244 from pygy/robust-onbeforeremove

Robust onbeforeremove

render/render.js

@@ -203,7 +203,7 @@ module.exports = function($window) {
 			else updateComponent(parent, old, vnode, hooks, nextSibling, recycling, ns)
 		}
 		else {
-			removeNode(parent, old, null, false)
+			removeNode(parent, old, null)
 			insertNode(parent, createNode(vnode, hooks, undefined), nextSibling)
 		}
 	}
@@ -265,7 +265,7 @@ module.exports = function($window) {
 			vnode.domSize = vnode.instance.domSize
 		}
 		else if (old.instance != null) {
-			removeNode(parent, old.instance, null, false)
+			removeNode(parent, old.instance, null)
 			vnode.dom = undefined
 			vnode.domSize = 0
 		}
@@ -327,41 +327,48 @@ module.exports = function($window) {
 			var vnode = vnodes[i]
 			if (vnode != null) {
 				if (vnode.skip) vnode.skip = false
-				else removeNode(parent, vnode, context, false)
+				else removeNode(parent, vnode, context)
 			}
 		}
 	}
-	function removeNode(parent, vnode, context, deferred) {
+	function once(f) {
-		if (deferred === false) {
+		var called = false
-			var expected = 0, called = 0
+		return function() {
-			var callback = function() {
+			if (!called) {
-				if (++called === expected) removeNode(parent, vnode, context, true)
+				called = true
+				f()
 			}
-			if (vnode.attrs && vnode.attrs.onbeforeremove) {
-				expected++
-				vnode.attrs.onbeforeremove.call(vnode.state, vnode, callback)
-			}
-			if (typeof vnode.tag !== "string" && vnode.tag.onbeforeremove) {
-				expected++
-				vnode.tag.onbeforeremove.call(vnode.state, vnode, callback)
-			}
-			if (expected > 0) return
 		}
-
+	}
-		onremove(vnode)
+	function removeNode(parent, vnode, context) {
-		if (vnode.dom) {
+		var expected = 1, called = 0
-			var count = vnode.domSize || 1
+		if (vnode.attrs && vnode.attrs.onbeforeremove) {
-			if (count > 1) {
+			expected++
-				var dom = vnode.dom
+			vnode.attrs.onbeforeremove.call(vnode.state, vnode, once(continuation))
-				while (--count) {
+		}
-					parent.removeChild(dom.nextSibling)
+		if (typeof vnode.tag !== "string" && vnode.tag.onbeforeremove) {
+			expected++
+			vnode.tag.onbeforeremove.call(vnode.state, vnode, once(continuation))
+		}
+		continuation()
+		function continuation() {
+			if (++called === expected) {
+				onremove(vnode)
+				if (vnode.dom) {
+					var count = vnode.domSize || 1
+					if (count > 1) {
+						var dom = vnode.dom
+						while (--count) {
+							parent.removeChild(dom.nextSibling)
+						}
+					}
+					if (vnode.dom.parentNode != null) parent.removeChild(vnode.dom)
+					if (context != null && vnode.domSize == null && !hasIntegrationMethods(vnode.attrs) && typeof vnode.tag === "string") { //TODO test custom elements
+						if (!context.pool) context.pool = [vnode]
+						else context.pool.push(vnode)
+					}
 				}
 			}
-			if (vnode.dom.parentNode != null) parent.removeChild(vnode.dom)
-			if (context != null && vnode.domSize == null && !hasIntegrationMethods(vnode.attrs) && typeof vnode.tag === "string") { //TODO test custom elements
-				if (!context.pool) context.pool = [vnode]
-				else context.pool.push(vnode)
-			}
 		}
 	}
 	function onremove(vnode) {

render/tests/test-onbeforeremove.js

@@ -188,4 +188,34 @@ o.spec("onbeforeremove", function() {
 		o(root.childNodes.length).equals(1)
 		o(root.firstChild.firstChild.nodeValue).equals("2")
 	})
+	o("finalizes the remove phase only once when `done()` is called synchronously from both attrs- and tag.onbeforeremove", function() {
+		var onremove = o.spy()
+		var component = {
+			view: function(){return {tag:'br'}},
+			onbeforeremove: function(vnode, done){done()},
+			onremove: onremove
+		}
+		render(root, [{tag: component, attrs: component}])
+		render(root, [])
+		o(onremove.callCount).equals(2) // once for `tag`, once for `attrs`
+	})
+	o("doesn't finalize prematurely if `done` is called twice in the `tag` hook", function(done) {
+		var async = false
+		var component = {
+			view: function(){return {tag:'br'}},
+			onbeforeremove: function(vnode, doneRemoving){
+				doneRemoving()
+				doneRemoving()
+			},
+			onremove: function(){
+				o(async).equals(true)("onremove should be called asynchronously")
+				done()
+			}
+		}
+		render(root, [{tag:component, attrs: {onbeforeremove: function(vnode, doneRemoving){
+			callAsync(doneRemoving)
+		}}}])
+		render(root, [])
+		async = true
+	})
 })

render/tests/test-onremove.js

@@ -145,4 +145,15 @@ o.spec("onremove", function() {
 
 		o(vnode.dom).notEquals(updated.dom)
 	})
+	o("The remove phase is finalized only once when `done()` is called synchronously from both attrs- and tag.onbeforeremove", function () {
+		var onremove = o.spy()
+		var component = {
+			view: function(){return m('br')},
+			onbeforeremove: function(vnode, done){done()},
+			onremove: onremove
+		}
+		render(root, [{tag: component, attrs: component}])
+		render(root, [])
+		o(onremove.callCount).equals(2)
+	})
 })