Add tests for the hooks for a delayed removal in a fragment, address other review concerns

package-lock.json

@@ -8,9 +8,6 @@
       "name": "mithril",
       "version": "2.2.2",
       "license": "MIT",
-      "bin": {
-        "ospec": "ospec/bin/ospec"
-      },
       "devDependencies": {
         "@alrra/travis-scripts": "^3.0.1",
         "@babel/parser": "^7.7.5",

render/render.js

@@ -14,7 +14,6 @@ module.exports = function($window) {
 	}
 
 	var currentRedraw
-	var currentDOM
 	var currentRender
 
 	function getNameSpace(vnode) {
@@ -323,9 +322,9 @@ module.exports = function($window) {
 					if (start === end) break
 					if (o.key !== ve.key || oe.key !== v.key) break
 					topSibling = getNextSibling(old, oldStart, nextSibling)
-					moveNode(parent, oe, topSibling)
+					moveDOM(parent, oe, topSibling)
 					if (oe !== v) updateNode(parent, oe, v, hooks, topSibling, ns)
-					if (++start <= --end) moveNode(parent, o, nextSibling)
+					if (++start <= --end) moveDOM(parent, o, nextSibling)
 					if (o !== ve) updateNode(parent, o, ve, hooks, nextSibling, ns)
 					if (ve.dom != null) nextSibling = ve.dom
 					oldStart++; oldEnd--
@@ -377,7 +376,7 @@ module.exports = function($window) {
 								if (oldIndices[i-start] === -1) createNode(parent, v, hooks, ns, nextSibling)
 								else {
 									if (lisIndices[li] === i - start) li--
-									else moveNode(parent, v, nextSibling)
+									else moveDOM(parent, v, nextSibling)
 								}
 								if (v.dom != null) nextSibling = vnodes[i].dom
 							}
@@ -547,7 +546,7 @@ module.exports = function($window) {
 	}
 
 	// This handles fragments with zombie children (removed from vdom, but persisted in DOM through onbeforeremove)
-	function moveNode(parent, vnode, nextSibling) {
+	function moveDOM(parent, vnode, nextSibling) {
 		if (vnode.dom != null) {
 			var target
 			if (vnode.domSize == null) {
@@ -611,25 +610,37 @@ module.exports = function($window) {
 		// If we can, try to fast-path it and avoid all the overhead of awaiting
 		if (!mask) {
 			onremove(vnode)
-			removeDOM(parent, vnode, undefined)
+			removeDOM(parent, vnode, generation)
 		} else {
 			generation = currentRender
 			for (var dom of domFor(vnode)) delayedRemoval.set(dom, generation)
-			var finalizer = function(a, b) {
+			if (stateResult != null) {
-				return function () {
+				stateResult.finally(function () {
 					// eslint-disable-next-line no-bitwise
-					if (mask & a) { mask &= b; if (!mask) {
+					if (mask & 1) {
+						// eslint-disable-next-line no-bitwise
+						mask &= 2
+						if (!mask) {
 							checkState(vnode, original)
 							onremove(vnode)
 							removeDOM(parent, vnode, generation)
-					} }
 						}
 					}
-			if (stateResult != null) {
+				})
-				stateResult.finally(finalizer(1, 2))
 			}
 			if (attrsResult != null) {
-				attrsResult.finally(finalizer(2, 1))
+				attrsResult.finally(function () {
+					// eslint-disable-next-line no-bitwise
+					if (mask & 2) {
+						// eslint-disable-next-line no-bitwise
+						mask &= 1
+						if (!mask) {
+							checkState(vnode, original)
+							onremove(vnode)
+							removeDOM(parent, vnode, generation)
+						}
+					}
+				})
 			}
 		}
 	}
@@ -902,6 +913,8 @@ module.exports = function($window) {
 		return true
 	}
 
+	var currentDOM
+
 	return function(dom, vnodes, redraw) {
 		if (!dom) throw new TypeError("DOM element being rendered to does not exist.")
 		if (currentDOM != null && dom.contains(currentDOM)) {

render/tests/test-onbeforeremove.js

@@ -74,17 +74,18 @@ o.spec("onbeforeremove", function() {
 			})
 		}
 	})
-	o("calls remove after onbeforeremove resolves", function(done) {
+	o("calls onremove after onbeforeremove resolves", function(done) {
 		var spy = o.spy()
-		var vnode = fragment({onbeforeremove: remove, onremove: spy}, "a")
+		var vnode = fragment({onbeforeremove: onbeforeremove, onremove: spy}, "a")
 
 		render(root, vnode)
 		render(root, [])
 
-		function remove(node) {
+		function onbeforeremove(node) {
 			o(node).equals(vnode)
 			o(root.childNodes.length).equals(1)
 			o(root.firstChild).equals(vnode.dom)
+			o(spy.callCount).equals(0)
 
 			callAsync(function() {
 				o(root.childNodes.length).equals(0)
@@ -103,17 +104,6 @@ o.spec("onbeforeremove", function() {
 		o(vnode.dom.onbeforeremove).equals(undefined)
 		o(vnode.dom.attributes["onbeforeremove"]).equals(undefined)
 	})
-	o("does not recycle when there's an onbeforeremove", function() {
-		var remove = function() {}
-		var vnode = m("div", {key: 1, onbeforeremove: remove})
-		var updated = m("div", {key: 1, onbeforeremove: remove})
-
-		render(root, vnode)
-		render(root, [])
-		render(root, updated)
-
-		o(vnode.dom).notEquals(updated.dom)
-	})
 	o("does not leave elements out of order during removal", function(done) {
 		var remove = function() {return Promise.resolve()}
 		var vnodes = [m("div", {key: 1, onbeforeremove: remove}, "1"), m("div", {key: 2, onbeforeremove: remove}, "2")]

render/tests/test-onremove.js

@@ -194,9 +194,10 @@ o.spec("onremove", function() {
 				o(onremove.callCount).equals(1)
 			})
 			// Warning: this test is complicated because it's replicating a race condition.
-			o("removes correct nodes when child delays removal, parent removes, then child resolves", function () {
+			o("removes correct nodes in fragment when child delays removal, parent removes, then child resolves", function () {
 				// Custom assertion - we need to test the entire tree for consistency.
-				function template(tpl) {return function (root) {
+
+				const template = (tpl) => (root) => {
 					var expected = []
 
 					for (var i = 0; i < tpl.length; i++) {
@@ -231,7 +232,7 @@ o.spec("onremove", function() {
   expected, got
 ${actual}`
 					}
-				}}
+				}
 				var finallyCB1
 				var finallyCB2
 				var C = createComponent({
@@ -241,84 +242,137 @@ ${actual}`
 					}
 				})
 				function update(id, showParent, showChild) {
+					const removeParent = o.spy()
+					const removeSyncChild = o.spy()
+					const removeAsyncChild = o.spy()
+
 					render(root,
 						m("div",
 							showParent && fragment(
-								"", // Required
+								{onremove: removeParent},
+								m("a", {onremove: removeSyncChild}, "sync child"),
 								showChild && m(C, {
 									onbeforeremove: function () {
 										return {then(){}, finally: function (fcb) { finallyCB2 = fcb }}
 									},
+									onremove: removeAsyncChild
 								}, m("div", id))
 							)
 						)
 					)
+					return {removeAsyncChild,removeParent, removeSyncChild}
 				}
 
-				update("1", true, true)
+				const hooks1 = update("1", true, true)
 				o(root).satisfies(template([
-					["#text", ""],
+					["A", "sync child"],
 					["DIV", "1"],
 				]))
 				o(finallyCB1).equals(undefined)
 				o(finallyCB2).equals(undefined)
 
-				update("2", true, false)
+				const hooks2 = update("2", true, false)
 
 				o(root).satisfies(template([
-					["#text", ""],
+					["A", "sync child"],
 					["DIV", "1"],
 				]))
+
 				o(typeof finallyCB1).equals("function")
+				o(typeof finallyCB2).equals("function")
 
 				var original1 = finallyCB1
 				var original2 = finallyCB2
 
-				update("3", true, true)
+				const hooks3 = update("3", true, true)
 
 				o(root).satisfies(template([
-					["#text", ""],
+					["A", "sync child"],
 					["DIV", "1"],
 					["DIV", "3"],
 				]))
+
+				o(hooks3.removeParent.callCount).equals(0)
+				o(hooks3.removeSyncChild.callCount).equals(0)
+				o(hooks3.removeAsyncChild.callCount).equals(0)
 				o(finallyCB1).equals(original1)
 				o(finallyCB2).equals(original2)
 
-				update("4", false, true)
+				const hooks4 = update("4", false, true)
 
 				o(root).satisfies(template([
 					["DIV", "1"],
 				]))
+
+				o(hooks3.removeParent.callCount).equals(1)
+				o(hooks3.removeSyncChild.callCount).equals(1)
+				o(hooks3.removeAsyncChild.callCount).equals(1)
+				o(hooks3.removeParent.args[0].tag).equals("[")
 				o(finallyCB1).equals(original1)
 				o(finallyCB2).equals(original2)
 
-				update("5", true, true)
+				const hooks5 = update("5", true, true)
+
 
 				o(root).satisfies(template([
 					["DIV", "1"],
-					["#text", ""],
+					["A", "sync child"],
 					["DIV", "5"],
 				]))
 				o(finallyCB1).equals(original1)
 				o(finallyCB2).equals(original2)
 
+				o(hooks1.removeAsyncChild.callCount).equals(0)
+
 				finallyCB1()
+
+				o(hooks1.removeAsyncChild.callCount).equals(0)
+
 				finallyCB2()
 
+				o(hooks1.removeAsyncChild.callCount).equals(1)
+
 				o(root).satisfies(template([
-					["#text", ""],
+					["A", "sync child"],
 					["DIV", "5"],
 				]))
 				o(finallyCB1).equals(original1)
 				o(finallyCB2).equals(original2)
 
-				update("6", true, true)
+				const hooks6 = update("6", true, true)
+
 				o(root).satisfies(template([
-					["#text", ""],
+					["A", "sync child"],
 					["DIV", "6"],
 				]))
 				o(finallyCB1).equals(original1)
 				o(finallyCB2).equals(original2)
+
+				// final tally
+				o(hooks1.removeParent.callCount).equals(0)
+				o(hooks1.removeSyncChild.callCount).equals(0)
+				o(hooks1.removeAsyncChild.callCount).equals(1)
+
+				o(hooks2.removeParent.callCount).equals(0)
+				o(hooks2.removeSyncChild.callCount).equals(0)
+				o(hooks2.removeAsyncChild.callCount).equals(0)
+
+				o(hooks3.removeParent.callCount).equals(1)
+				o(hooks3.removeSyncChild.callCount).equals(1)
+				o(hooks3.removeAsyncChild.callCount).equals(1)
+
+				o(hooks4.removeParent.callCount).equals(0)
+				o(hooks4.removeSyncChild.callCount).equals(0)
+				o(hooks4.removeAsyncChild.callCount).equals(0)
+
+				o(hooks5.removeParent.callCount).equals(0)
+				o(hooks5.removeSyncChild.callCount).equals(0)
+				o(hooks5.removeAsyncChild.callCount).equals(0)
+
+				o(hooks6.removeParent.callCount).equals(0)
+				o(hooks6.removeSyncChild.callCount).equals(0)
+				o(hooks6.removeAsyncChild.callCount).equals(0)
+
 			})
 		})
 	})