implement m.domFor() and use it internally to move and remove nodes. Fix #2780

render/dom-for.js

@@ -0,0 +1,24 @@
+"use strict"
+
+var delayedRemoval = module.exports.delayedRemoval = new WeakMap
+
+module.exports.domFor = function *domFor(vnode, {generation} = {generation: undefined}) {
+	let {dom, domSize} = vnode
+	if (dom != null) {
+		if (domSize == null) {
+			if (delayedRemoval.get(dom) === generation) {
+				yield dom
+			}
+		} else {
+			let i = 0, next
+			while (i < domSize) {
+				next = dom.nextSibling
+				if (delayedRemoval.get(dom) === generation) {
+					yield dom
+					i++
+				}
+				dom = next
+			}
+		}
+	}
+}

render/render.js

@@ -1,16 +1,20 @@
 "use strict"
 
-var Vnode = require("../render/vnode")
+const Vnode = require("../render/vnode")
+const {domFor, delayedRemoval} = require("../render/dom-for")
 
 module.exports = function($window) {
-	var $doc = $window && $window.document
-	var currentRedraw
+	const $doc = $window && $window.document
 
-	var nameSpace = {
+	const nameSpace = {
 		svg: "http://www.w3.org/2000/svg",
 		math: "http://www.w3.org/1998/Math/MathML"
 	}
 
+	let currentRedraw
+	let currentDOM
+	let currentRender
+
 	function getNameSpace(vnode) {
 		return vnode.attrs && vnode.attrs.xmlns || nameSpace[vnode.tag]
 	}
@@ -87,11 +91,9 @@ module.exports = function($window) {
 		vnode.dom = temp.firstChild
 		vnode.domSize = temp.childNodes.length
 		// Capture nodes to remove, so we don't confuse them.
-		vnode.instance = []
 		var fragment = $doc.createDocumentFragment()
 		var child
 		while (child = temp.firstChild) {
-			vnode.instance.push(child)
 			fragment.appendChild(child)
 		}
 		insertNode(parent, fragment, nextSibling)
@@ -421,13 +423,12 @@ module.exports = function($window) {
 	}
 	function updateHTML(parent, old, vnode, ns, nextSibling) {
 		if (old.children !== vnode.children) {
-			removeHTML(parent, old)
+			removeDOM(parent, old, undefined)
 			createHTML(parent, vnode, ns, nextSibling)
 		}
 		else {
 			vnode.dom = old.dom
 			vnode.domSize = old.domSize
-			vnode.instance = old.instance
 		}
 	}
 	function updateFragment(parent, old, vnode, hooks, nextSibling, ns) {
@@ -543,42 +544,18 @@ module.exports = function($window) {
 		return nextSibling
 	}
 
-	// This covers a really specific edge case:
-	// - Parent node is keyed and contains child
-	// - Child is removed, returns unresolved promise in `onbeforeremove`
-	// - Parent node is moved in keyed diff
-	// - Remaining children still need moved appropriately
-	//
-	// Ideally, I'd track removed nodes as well, but that introduces a lot more
-	// complexity and I'm not exactly interested in doing that.
+	// This handles fragments with zombie children (removed from vdom, but persisted in DOM throug onbeforeremove)
 	function moveNodes(parent, vnode, nextSibling) {
-		var frag = $doc.createDocumentFragment()
-		moveChildToFrag(parent, frag, vnode)
-		insertNode(parent, frag, nextSibling)
-	}
-	function moveChildToFrag(parent, frag, vnode) {
-		// Dodge the recursion overhead in a few of the most common cases.
-		while (vnode.dom != null && vnode.dom.parentNode === parent) {
-			if (typeof vnode.tag !== "string") {
-				vnode = vnode.instance
-				if (vnode != null) continue
-			} else if (vnode.tag === "<") {
-				for (var i = 0; i < vnode.instance.length; i++) {
-					frag.appendChild(vnode.instance[i])
-				}
-			} else if (vnode.tag !== "[") {
-				// Don't recurse for text nodes *or* elements, just fragments
-				frag.appendChild(vnode.dom)
-			} else if (vnode.children.length === 1) {
-				vnode = vnode.children[0]
-				if (vnode != null) continue
+		if (vnode.dom != null) {
+			var target
+			if (vnode.domSize == null) {
+				// don't allocate for the common case
+				target = vnode.dom
 			} else {
-				for (var i = 0; i < vnode.children.length; i++) {
-					var child = vnode.children[i]
-					if (child != null) moveChildToFrag(parent, frag, child)
-				}
+				target = $doc.createDocumentFragment()
+				for (const dom of domFor(vnode)) target.appendChild(dom)
 			}
-			break
+			insertNode(parent, target, nextSibling)
 		}
 	}
 
@@ -628,65 +605,42 @@ module.exports = function($window) {
 			}
 		}
 		checkState(vnode, original)
-
+		var generation
 		// If we can, try to fast-path it and avoid all the overhead of awaiting
 		if (!mask) {
 			onremove(vnode)
-			removeChild(parent, vnode)
+			removeDOM(parent, vnode, undefined)
 		} else {
-			if (stateResult != null) {
-				var next = function () {
+			generation = currentRender
+			for (const dom of domFor(vnode)) delayedRemoval.set(dom, generation)
+			function finalizer(a, b) {
+				return function () {
 					// eslint-disable-next-line no-bitwise
-					if (mask & 1) { mask &= 2; if (!mask) reallyRemove() }
+					if (mask & a) { mask &= b; if (!mask) {
+						checkState(vnode, original)
+						onremove(vnode)
+						removeDOM(parent, vnode, generation)
+					} }
 				}
-				stateResult.then(next, next)
+			}
+			if (stateResult != null) {
+				stateResult.finally(finalizer(1, 2))
 			}
 			if (attrsResult != null) {
-				var next = function () {
-					// eslint-disable-next-line no-bitwise
-					if (mask & 2) { mask &= 1; if (!mask) reallyRemove() }
-				}
-				attrsResult.then(next, next)
+				attrsResult.finally(finalizer(2, 1))
 			}
 		}
+	}
+	function removeDOM(parent, vnode, generation) {
+		if (vnode.dom == null) return
+		if (vnode.domSize == null) {
+			// don't allocate for the common case
+			if (delayedRemoval.get(vnode.dom) === generation) parent.removeChild(vnode.dom)
+		} else {
+			for (const dom of domFor(vnode, {generation})) parent.removeChild(dom)
+		}
+	}
 
-		function reallyRemove() {
-			checkState(vnode, original)
-			onremove(vnode)
-			removeChild(parent, vnode)
-		}
-	}
-	function removeHTML(parent, vnode) {
-		for (var i = 0; i < vnode.instance.length; i++) {
-			parent.removeChild(vnode.instance[i])
-		}
-	}
-	function removeChild(parent, vnode) {
-		// Dodge the recursion overhead in a few of the most common cases.
-		while (vnode.dom != null && vnode.dom.parentNode === parent) {
-			if (typeof vnode.tag !== "string") {
-				vnode = vnode.instance
-				if (vnode != null) continue
-			} else if (vnode.tag === "<") {
-				removeHTML(parent, vnode)
-			} else {
-				if (vnode.tag !== "[") {
-					parent.removeChild(vnode.dom)
-					if (!Array.isArray(vnode.children)) break
-				}
-				if (vnode.children.length === 1) {
-					vnode = vnode.children[0]
-					if (vnode != null) continue
-				} else {
-					for (var i = 0; i < vnode.children.length; i++) {
-						var child = vnode.children[i]
-						if (child != null) removeChild(parent, child)
-					}
-				}
-			}
-			break
-		}
-	}
 	function onremove(vnode) {
 		if (typeof vnode.tag !== "string" && typeof vnode.state.onremove === "function") callHook.call(vnode.state.onremove, vnode)
 		if (vnode.attrs && typeof vnode.attrs.onremove === "function") callHook.call(vnode.attrs.onremove, vnode)
@@ -946,8 +900,6 @@ 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)) {
@@ -961,6 +913,7 @@ module.exports = function($window) {
 
 		currentDOM = dom
 		currentRedraw = typeof redraw === "function" ? redraw : undefined
+		currentRender = {}
 		try {
 			// First time rendering into a node clears it out
 			if (dom.vnodes == null) dom.textContent = ""

render/tests/test-dom-for.js

@@ -0,0 +1,176 @@
+var o = require("ospec")
+var components = require("../../test-utils/components")
+var domMock = require("../../test-utils/domMock")
+var vdom = require("../../render/render")
+var m = require("../../render/hyperscript")
+var fragment = require("../../render/fragment")
+var domFor = require('../../render/dom-for').domFor
+
+o.spec("domFor(vnode)", function() {
+	var $window, root, render
+	o.beforeEach(function() {
+		$window = domMock()
+		root = $window.document.createElement("div")
+		render = vdom($window)
+	})
+    o('works for simple vnodes', function() {
+        render(root, m('div', {oncreate(vnode){
+            let n = 0
+            for (const dom of domFor(vnode)) {
+                o(dom).equals(root.firstChild)
+                o(++n).equals(1)
+            }
+        }}))
+    })
+    o('works for fragments', function () {
+        render(root, fragment({
+            oncreate(vnode){
+                let n = 0
+                for (const dom of domFor(vnode)) {
+                    o(dom).equals(root.childNodes[n])
+                    n++
+                }
+                o(n).equals(2)
+            }
+        }, [
+            m('a'),
+            m('b')
+        ]))
+    })
+    o('works in fragments with children that have delayed removal', function() {
+        function oncreate(vnode){
+            o(root.childNodes.length).equals(3)
+            o(root.childNodes[0].nodeName).equals('A')
+            o(root.childNodes[1].nodeName).equals('B')
+            o(root.childNodes[2].nodeName).equals('C')
+
+            const iter = domFor(vnode)
+            o(iter.next()).deepEquals({done:false, value: root.childNodes[0]})
+            o(iter.next()).deepEquals({done:false, value: root.childNodes[1]})
+            o(iter.next()).deepEquals({done:false, value: root.childNodes[2]})
+            o(iter.next().done).deepEquals(true)
+            o(root.childNodes.length).equals(3)
+        }
+        function onupdate(vnode) {
+            // the b node is still present in the DOM
+            o(root.childNodes.length).equals(3)
+            o(root.childNodes[0].nodeName).equals('A')
+            o(root.childNodes[1].nodeName).equals('B')
+            o(root.childNodes[2].nodeName).equals('C')
+
+            const iter = domFor(vnode)
+            o(iter.next()).deepEquals({done:false, value: root.childNodes[0]})
+            o(iter.next()).deepEquals({done:false, value: root.childNodes[2]})
+            o(iter.next().done).deepEquals(true)
+            o(root.childNodes.length).equals(3)
+        }
+        
+        render(root, fragment(
+            {oncreate, onupdate},
+            [
+                m('a'),
+                m('b', {onbeforeremove(){return {then(){}, finally(){}}}}),
+                m('c')
+            ]
+        ))
+        render(root, fragment(
+            {oncreate, onupdate},
+            [
+                m('a'),
+                null,
+                m('c'),
+            ]
+        ))
+
+    })
+    components.forEach(function(cmp){
+		o.spec(cmp.kind, function(){
+			var createComponent = cmp.create
+            o('works for components that return one element', function() {
+                const C = createComponent({
+                    view(){return m('div')},
+                    oncreate(vnode){
+                        let n = 0
+                        for (const dom of domFor(vnode)) {
+                            o(dom).equals(root.firstChild)
+                            o(++n).equals(1)
+                        }
+                    }
+                })
+                render(root, m(C))
+            })
+            o('works for components that return fragments', function () {
+                const oncreate = o.spy(function oncreate(vnode){
+                    o(root.childNodes.length).equals(3)
+                    o(root.childNodes[0].nodeName).equals('A')
+                    o(root.childNodes[1].nodeName).equals('B')
+                    o(root.childNodes[2].nodeName).equals('C')
+        
+                    const iter = domFor(vnode)
+                    o(iter.next()).deepEquals({done:false, value: root.childNodes[0]})
+                    o(iter.next()).deepEquals({done:false, value: root.childNodes[1]})
+                    o(iter.next()).deepEquals({done:false, value: root.childNodes[2]})
+                    o(iter.next().done).deepEquals(true)
+                    o(root.childNodes.length).equals(3)
+                })
+                const C = createComponent({
+                    view({children}){return children},
+                    oncreate
+                })
+                render(root, m(C, [
+                    m('a'),
+                    m('b'),
+                    m('c')
+                ]))
+                o(oncreate.callCount).equals(1)
+            })
+            o('works for components that return fragments with delayed removal', function () {
+                const onbeforeremove = o.spy(function onbeforeremove(){return {then(){}, finally(){}}})
+                const oncreate = o.spy(function oncreate(vnode){
+                    o(root.childNodes.length).equals(3)
+                    o(root.childNodes[0].nodeName).equals('A')
+                    o(root.childNodes[1].nodeName).equals('B')
+                    o(root.childNodes[2].nodeName).equals('C')
+        
+                    const iter = domFor(vnode)
+                    o(iter.next()).deepEquals({done:false, value: root.childNodes[0]})
+                    o(iter.next()).deepEquals({done:false, value: root.childNodes[1]})
+                    o(iter.next()).deepEquals({done:false, value: root.childNodes[2]})
+                    o(iter.next().done).deepEquals(true)
+                    o(root.childNodes.length).equals(3)
+                })
+                const onupdate = o.spy(function onupdate(vnode) {
+                    o(root.childNodes.length).equals(3)
+                    o(root.childNodes[0].nodeName).equals('A')
+                    o(root.childNodes[1].nodeName).equals('B')
+                    o(root.childNodes[2].nodeName).equals('C')
+        
+                    const iter = domFor(vnode)
+
+                    o(iter.next()).deepEquals({done:false, value: root.childNodes[0]})
+                    o(iter.next()).deepEquals({done:false, value: root.childNodes[2]})
+                    o(iter.next().done).deepEquals(true)
+                    o(root.childNodes.length).equals(3)            
+                })
+                const C = createComponent({
+                    view({children}){return children},
+                    oncreate,
+                    onupdate
+                })
+                render(root, m(C, [
+                    m('a'),
+                    m('b', {onbeforeremove}),
+                    m('c')
+                ]))
+                render(root, m(C, [
+                    m('a'),
+                    null,
+                    m('c')
+                ]))
+                o(oncreate.callCount).equals(1)
+                o(onupdate.callCount).equals(1)
+                o(onbeforeremove.callCount).equals(1)
+            })
+        })
+    })
+})

render/tests/test-onremove.js

@@ -195,13 +195,13 @@ o.spec("onremove", function() {
 			})
 			// 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 () {
-				// Sugar over the complexity - I need to test the entire tree for consistency.
-				function expect(expectedPairs) {
+				// Custom assertion - we need to test the entire tree for consistency.
+				function template(tpl) {return function (root) {
 					var expected = []
 
-					for (var i = 0; i < expectedPairs.length; i++) {
-						var name = expectedPairs[i][0]
-						var text = expectedPairs[i][1]
+					for (var i = 0; i < tpl.length; i++) {
+						var name = tpl[i][0]
+						var text = tpl[i][1]
 						expected.push({
 							name: name,
 							firstType: name === "#text" ? null : "#text",
@@ -222,23 +222,30 @@ o.spec("onremove", function() {
 							text: textNode.nodeValue,
 						})
 					}
+					actual = JSON.stringify(actual, null, '  ')
+					expected = JSON.stringify(expected, null, '  ')
+					return {
+						pass: actual === expected,
+						message:
+`${expected}
+  expected, got
+${actual}`
+					}
+				}}
 
-					o(actual).deepEquals(expected)
-				}
-
-				var resolve
-
+				var finallyCB
 				function update(id, showParent, showChild) {
 					render(root,
 						m("div",
 							showParent && fragment(
 								"", // Required
-								showChild && fragment({
-									onbeforeremove: function () {
-										return {then: function (r) { resolve = r }}
+								showChild && fragment(
+									{
+										onbeforeremove: function () {
+											return {then(){}, finally: function (fcb) { finallyCB = fcb }}
+										},
 									},
-								},
-								m("div", id)
+									m("div", id)
 								)
 							)
 						)
@@ -246,170 +253,61 @@ o.spec("onremove", function() {
 				}
 
 				update("1", true, true)
-				expect([
+				o(root).satisfies(template([
 					["#text", ""],
 					["DIV", "1"],
-				])
-				o(resolve).equals(undefined)
+				]))
+				o(finallyCB).equals(undefined)
 
 				update("2", true, false)
-				expect([
+
+				o(root).satisfies(template([
 					["#text", ""],
 					["DIV", "1"],
-				])
-				o(typeof resolve).equals("function")
-				var original = resolve
+				]))
+				o(typeof finallyCB).equals("function")
+
+				var original = finallyCB
 
 				update("3", true, true)
-				expect([
+
+				o(root).satisfies(template([
 					["#text", ""],
 					["DIV", "1"],
 					["DIV", "3"],
-				])
-				o(resolve).equals(original)
+				]))
+				o(finallyCB).equals(original)
 
 				update("4", false, true)
-				expect([
+
+				o(root).satisfies(template([
 					["DIV", "1"],
-				])
-				o(resolve).equals(original)
+				]))
+				o(finallyCB).equals(original)
 
 				update("5", true, true)
-				expect([
+
+				o(root).satisfies(template([
 					["DIV", "1"],
 					["#text", ""],
 					["DIV", "5"],
-				])
-				o(resolve).equals(original)
+				]))
+				o(finallyCB).equals(original)
 
-				resolve()
-				expect([
+				finallyCB()
+
+				o(root).satisfies(template([
 					["#text", ""],
 					["DIV", "5"],
-				])
-				o(resolve).equals(original)
+				]))
+				o(finallyCB).equals(original)
 
 				update("6", true, true)
-				expect([
+				o(root).satisfies(template([
 					["#text", ""],
 					["DIV", "6"],
-				])
-				o(resolve).equals(original)
-			})
-			// 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 + rejects both", function () {
-				// Sugar over the complexity - I need to test the entire tree for consistency.
-				function expect(expectedPairs) {
-					var expected = []
-
-					for (var i = 0; i < expectedPairs.length; i++) {
-						var name = expectedPairs[i][0]
-						var text = expectedPairs[i][1]
-						expected.push({
-							name: name,
-							firstType: name === "#text" ? null : "#text",
-							text: text,
-						})
-					}
-
-					var actual = []
-					var list = root.firstChild.childNodes
-					for (var i = 0; i < list.length; i++) {
-						var current = list[i]
-						var textNode = current.childNodes.length === 1
-							? current.firstChild
-							: current
-						actual.push({
-							name: current.nodeName,
-							firstType: textNode === current ? null : textNode.nodeName,
-							text: textNode.nodeValue,
-						})
-					}
-
-					o(actual).deepEquals(expected)
-				}
-
-				var resolve, reject
-
-				function update(id, showParent, showChild) {
-					render(root,
-						m("div",
-							showParent && fragment(
-								"", // Required
-								showChild && fragment({
-									onbeforeremove: function () {
-										return {then: function (res, rej) {
-											resolve = res
-											reject = rej
-										}}
-									},
-								},
-								m("div", id)
-								)
-							)
-						)
-					)
-				}
-
-				update("1", true, true)
-				expect([
-					["#text", ""],
-					["DIV", "1"],
-				])
-				o(resolve).equals(undefined)
-
-				update("2", true, false)
-				expect([
-					["#text", ""],
-					["DIV", "1"],
-				])
-				o(typeof resolve).equals("function")
-				var originalResolve = resolve
-				var originalReject = reject
-
-				update("3", true, true)
-				expect([
-					["#text", ""],
-					["DIV", "1"],
-					["DIV", "3"],
-				])
-				o(resolve).equals(originalResolve)
-				o(reject).equals(originalReject)
-
-				update("4", false, true)
-				expect([
-					["DIV", "1"],
-				])
-				o(resolve).equals(originalResolve)
-				o(reject).equals(originalReject)
-
-				update("5", true, true)
-				expect([
-					["DIV", "1"],
-					["#text", ""],
-					["DIV", "5"],
-				])
-				o(resolve).equals(originalResolve)
-				o(reject).equals(originalReject)
-
-				resolve()
-				reject()
-				reject()
-				resolve()
-				expect([
-					["#text", ""],
-					["DIV", "5"],
-				])
-				o(resolve).equals(originalResolve)
-				o(reject).equals(originalReject)
-
-				update("6", true, true)
-				expect([
-					["#text", ""],
-					["DIV", "6"],
-				])
-				o(resolve).equals(originalResolve)
-				o(reject).equals(originalReject)
+				]))
+				o(finallyCB).equals(original)
 			})
 		})
 	})