From 39fa2b32c2e4f1d72fc1fb98f82eba1faf1b9bd4 Mon Sep 17 00:00:00 2001 From: Isiah Meadows Date: Fri, 26 Jul 2019 18:19:40 -0400 Subject: [PATCH] Fix #1881 + related ospec bug (#2492) * Fix #1881 + related ospec bug * Test duplicate resolves, update changelog --- docs/change-log.md | 2 + ospec/ospec.js | 11 +- render/render.js | 161 +++++++++++++----- render/tests/test-onremove.js | 218 +++++++++++++++++++++++++ render/tests/test-updateNodesFuzzer.js | 5 +- test-utils/domMock.js | 24 ++- 6 files changed, 362 insertions(+), 59 deletions(-) diff --git a/docs/change-log.md b/docs/change-log.md index bfea03d3..a6c54fc8 100644 --- a/docs/change-log.md +++ b/docs/change-log.md @@ -16,6 +16,8 @@ ### Upcoming... +- Ensure vnodes are removed correctly in the face of `onbeforeremove` resolving after new nodes are added ([#2492](https://github.com/MithrilJS/mithril.js/pull/2492) [@isiahmeadows](https://github.com/isiahmeadows)) + --> ### v2.0.1 diff --git a/ospec/ospec.js b/ospec/ospec.js index e18cb4b6..7b63b5b5 100644 --- a/ospec/ospec.js +++ b/ospec/ospec.js @@ -289,12 +289,11 @@ else window.o = m() Assert.prototype[name] = function assert(value) { var self = this var message = serialize(self.value) + "\n " + verb + "\n" + serialize(value) - if (compare(self.value, value)){ - succeed(self, message) - return function(message) { - if (!self.pass) self.message = message + "\n\n" + self.message - } - }else fail(self, message) + if (compare(self.value, value)) succeed(self, message) + else fail(self, message) + return function(message) { + if (!self.pass) self.message = message + "\n\n" + self.message + } } } function succeed(assertion, message) { diff --git a/render/render.js b/render/render.js index d6ff5873..5d93c41e 100644 --- a/render/render.js +++ b/render/render.js @@ -86,9 +86,12 @@ 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) @@ -272,7 +275,7 @@ module.exports = function($window) { function updateNodes(parent, old, vnodes, hooks, nextSibling, ns) { if (old === vnodes || old == null && vnodes == null) return else if (old == null || old.length === 0) createNodes(parent, vnodes, 0, vnodes.length, hooks, nextSibling, ns) - else if (vnodes == null || vnodes.length === 0) removeNodes(old, 0, old.length) + else if (vnodes == null || vnodes.length === 0) removeNodes(parent, old, 0, old.length) else { var isOldKeyed = old[0] != null && old[0].key != null var isKeyed = vnodes[0] != null && vnodes[0].key != null @@ -281,7 +284,7 @@ module.exports = function($window) { if (!isKeyed) while (start < vnodes.length && vnodes[start] == null) start++ if (isKeyed === null && isOldKeyed == null) return // both lists are full of nulls if (isOldKeyed !== isKeyed) { - removeNodes(old, oldStart, old.length) + removeNodes(parent, old, oldStart, old.length) createNodes(parent, vnodes, start, vnodes.length, hooks, nextSibling, ns) } else if (!isKeyed) { // Don't index past the end of either list (causes deopts). @@ -295,10 +298,10 @@ module.exports = function($window) { v = vnodes[start] if (o === v || o == null && v == null) continue else if (o == null) createNode(parent, v, hooks, ns, getNextSibling(old, start + 1, nextSibling)) - else if (v == null) removeNode(o) + else if (v == null) removeNode(parent, o) else updateNode(parent, o, v, hooks, getNextSibling(old, start + 1, nextSibling), ns) } - if (old.length > commonLength) removeNodes(old, start, old.length) + if (old.length > commonLength) removeNodes(parent, old, start, old.length) if (vnodes.length > commonLength) createNodes(parent, vnodes, start, vnodes.length, hooks, nextSibling, ns) } else { // keyed diff @@ -326,9 +329,9 @@ module.exports = function($window) { if (start === end) break if (o.key !== ve.key || oe.key !== v.key) break topSibling = getNextSibling(old, oldStart, nextSibling) - insertNode(parent, toFragment(oe), topSibling) + moveNodes(parent, oe, topSibling) if (oe !== v) updateNode(parent, oe, v, hooks, topSibling, ns) - if (++start <= --end) insertNode(parent, toFragment(o), nextSibling) + if (++start <= --end) moveNodes(parent, o, nextSibling) if (o !== ve) updateNode(parent, o, ve, hooks, nextSibling, ns) if (ve.dom != null) nextSibling = ve.dom oldStart++; oldEnd-- @@ -346,7 +349,7 @@ module.exports = function($window) { oe = old[oldEnd] ve = vnodes[end] } - if (start > end) removeNodes(old, oldStart, oldEnd + 1) + if (start > end) removeNodes(parent, old, oldStart, oldEnd + 1) else if (oldStart > oldEnd) createNodes(parent, vnodes, start, end + 1, hooks, nextSibling, ns) else { // inspired by ivi https://github.com/ivijs/ivi/ by Boris Kaul @@ -367,7 +370,7 @@ module.exports = function($window) { } } nextSibling = originalNextSibling - if (matched !== oldEnd - oldStart + 1) removeNodes(old, oldStart, oldEnd + 1) + if (matched !== oldEnd - oldStart + 1) removeNodes(parent, old, oldStart, oldEnd + 1) if (matched === 0) createNodes(parent, vnodes, start, end + 1, hooks, nextSibling, ns) else { if (pos === -1) { @@ -380,7 +383,7 @@ module.exports = function($window) { if (oldIndices[i-start] === -1) createNode(parent, v, hooks, ns, nextSibling) else { if (lisIndices[li] === i - start) li-- - else insertNode(parent, toFragment(v), nextSibling) + else moveNodes(parent, v, nextSibling) } if (v.dom != null) nextSibling = vnodes[i].dom } @@ -416,7 +419,7 @@ module.exports = function($window) { else updateComponent(parent, old, vnode, hooks, nextSibling, ns) } else { - removeNode(old) + removeNode(parent, old) createNode(parent, vnode, hooks, ns, nextSibling) } } @@ -428,7 +431,7 @@ module.exports = function($window) { } function updateHTML(parent, old, vnode, ns, nextSibling) { if (old.children !== vnode.children) { - toFragment(old) + removeHTML(parent, old) createHTML(parent, vnode, ns, nextSibling) } else vnode.dom = old.dom, vnode.domSize = old.domSize @@ -483,7 +486,7 @@ module.exports = function($window) { vnode.domSize = vnode.instance.domSize } else if (old.instance != null) { - removeNode(old.instance) + removeNode(parent, old.instance) vnode.dom = undefined vnode.domSize = 0 } @@ -550,19 +553,6 @@ module.exports = function($window) { return result } - function toFragment(vnode) { - var count = vnode.domSize - if (count != null || vnode.dom == null) { - var fragment = $doc.createDocumentFragment() - if (count > 0) { - var dom = vnode.dom - while (--count) fragment.appendChild(dom.nextSibling) - fragment.insertBefore(dom, fragment.firstChild) - } - return fragment - } - else return vnode.dom - } function getNextSibling(vnodes, i, nextSibling) { for (; i < vnodes.length; i++) { if (vnodes[i] != null && vnodes[i].dom != null) return vnodes[i].dom @@ -570,6 +560,45 @@ 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. + 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 + } else { + for (var i = 0; i < vnode.children.length; i++) { + var child = vnode.children[i] + if (child != null) moveChildToFrag(parent, frag, child) + } + } + break + } + } + function insertNode(parent, dom, nextSibling) { if (nextSibling != null) parent.insertBefore(dom, nextSibling) else parent.appendChild(dom) @@ -589,41 +618,89 @@ module.exports = function($window) { } //remove - function removeNodes(vnodes, start, end) { + function removeNodes(parent, vnodes, start, end) { for (var i = start; i < end; i++) { var vnode = vnodes[i] - if (vnode != null) removeNode(vnode) + if (vnode != null) removeNode(parent, vnode) } } - function removeNode(vnode) { - var expected = 1, called = 0 + function removeNode(parent, vnode) { + var mask = 0 var original = vnode.state + var stateResult, attrsResult if (typeof vnode.tag !== "string" && typeof vnode.state.onbeforeremove === "function") { var result = callHook.call(vnode.state.onbeforeremove, vnode) if (result != null && typeof result.then === "function") { - expected++ - result.then(continuation, continuation) + mask = 1 + stateResult = result } } if (vnode.attrs && typeof vnode.attrs.onbeforeremove === "function") { var result = callHook.call(vnode.attrs.onbeforeremove, vnode) if (result != null && typeof result.then === "function") { - expected++ - result.then(continuation, continuation) + // eslint-disable-next-line no-bitwise + mask |= 2 + attrsResult = result } } - continuation() - function continuation() { - if (++called === expected) { - checkState(vnode, original) - onremove(vnode) - if (vnode.dom) { - var parent = vnode.dom.parentNode - var count = vnode.domSize || 1 - while (--count) parent.removeChild(vnode.dom.nextSibling) + checkState(vnode, original) + + // If we can, try to fast-path it and avoid all the overhead of awaiting + if (!mask) { + onremove(vnode) + removeChild(parent, vnode) + } else { + if (stateResult != null) { + var next = function () { + // eslint-disable-next-line no-bitwise + if (mask & 1) { mask &= 2; if (!mask) reallyRemove() } + } + stateResult.then(next, next) + } + if (attrsResult != null) { + var next = function () { + // eslint-disable-next-line no-bitwise + if (mask & 2) { mask &= 1; if (!mask) reallyRemove() } + } + attrsResult.then(next, next) + } + } + + 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) { diff --git a/render/tests/test-onremove.js b/render/tests/test-onremove.js index bab0d308..c10b3c89 100644 --- a/render/tests/test-onremove.js +++ b/render/tests/test-onremove.js @@ -214,6 +214,224 @@ o.spec("onremove", function() { o(vnode.dom).notEquals(updated.dom) // this used to be a recycling pool test 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 () { + // 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 + + function update(id, showParent, showChild) { + render(root, [ + {tag: "div", children: [ + showParent ? {tag: "[", children: [ + {tag: "#", children: ""}, // Required + showChild ? {tag: "[", attrs: { + onbeforeremove: function () { + return {then: function (r) { resolve = r }} + }, + }, children: [ + {tag: "div", text: id}, + ]} : undefined, + ]} : undefined, + ]} + ]) + } + + 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 original = resolve + + update("3", true, true) + expect([ + ["#text", ""], + ["DIV", "1"], + ["DIV", "3"], + ]) + o(resolve).equals(original) + + update("4", false, true) + expect([ + ["DIV", "1"], + ]) + o(resolve).equals(original) + + update("5", true, true) + expect([ + ["DIV", "1"], + ["#text", ""], + ["DIV", "5"], + ]) + o(resolve).equals(original) + + resolve() + expect([ + ["#text", ""], + ["DIV", "5"], + ]) + o(resolve).equals(original) + + update("6", true, true) + expect([ + ["#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, [ + {tag: "div", children: [ + showParent ? {tag: "[", children: [ + {tag: "#", children: ""}, // Required + showChild ? {tag: "[", attrs: { + onbeforeremove: function () { + return {then: function (res, rej) { + resolve = res + reject = rej + }} + }, + }, children: [ + {tag: "div", text: id}, + ]} : undefined, + ]} : undefined, + ]} + ]) + } + + 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) + }) }) }) }) diff --git a/render/tests/test-updateNodesFuzzer.js b/render/tests/test-updateNodesFuzzer.js index c5ad44d0..1e2116b3 100644 --- a/render/tests/test-updateNodesFuzzer.js +++ b/render/tests/test-updateNodesFuzzer.js @@ -13,7 +13,7 @@ o.spec("updateNodes keyed list Fuzzer", function() { render = vdom($window) }) - + void [ {delMax: 0, movMax: 50, insMax: 9}, {delMax: 3, movMax: 5, insMax: 5}, @@ -31,7 +31,7 @@ o.spec("updateNodes keyed list Fuzzer", function() { render(root, test.updated.map(function(x){return {tag: x, key: x}})) if (root.appendChild.callCount + root.insertBefore.callCount !== test.expected.creations + test.expected.moves) console.log(test, {aC: root.appendChild.callCount, iB: root.insertBefore.callCount}, [].map.call(root.childNodes, function(n){return n.nodeName.toLowerCase()})) - + o(root.appendChild.callCount + root.insertBefore.callCount).equals(test.expected.creations + test.expected.moves)("moves") o(root.removeChild.callCount).equals(test.expected.deletions)("deletions") o([].map.call(root.childNodes, function(n){return n.nodeName.toLowerCase()})).deepEquals(test.updated) @@ -154,4 +154,3 @@ function addSpies(node) { node.insertBefore = o.spy(node.insertBefore) node.removeChild = o.spy(node.removeChild) } - diff --git a/test-utils/domMock.js b/test-utils/domMock.js index c8a56b27..01407e31 100644 --- a/test-utils/domMock.js +++ b/test-utils/domMock.js @@ -94,7 +94,7 @@ module.exports = function(options) { } else { this.childNodes.push(child) - if (child.parentNode != null && child.parentNode !== this) child.parentNode.removeChild(child) + if (child.parentNode != null && child.parentNode !== this) removeChild.call(child.parentNode, child) child.parentNode = this } } @@ -124,14 +124,14 @@ module.exports = function(options) { this.childNodes.splice.apply(this.childNodes, [refIndex, 0].concat(child.childNodes)) while (child.firstChild) { var subchild = child.firstChild - child.removeChild(subchild) + removeChild.call(child, subchild) subchild.parentNode = this } child.childNodes = [] } else { this.childNodes.splice(refIndex, 0, child) - if (child.parentNode != null && child.parentNode !== this) child.parentNode.removeChild(child) + if (child.parentNode != null && child.parentNode !== this) removeChild.call(child.parentNode, child) child.parentNode = this } } @@ -214,14 +214,14 @@ module.exports = function(options) { if (ns != null) element.setAttributeNS(ns, name, value) else element.setAttribute(name, value) }) - stack[depth].appendChild(element) + appendChild.call(stack[depth], element) if (!selfClosed && voidElements.indexOf(startTag.toLowerCase()) < 0) stack[++depth] = element } else if (endTag) { depth-- } else if (text) { - stack[depth].appendChild($window.document.createTextNode(text)) // FIXME handle html entities + appendChild.call(stack[depth], $window.document.createTextNode(text)) // FIXME handle html entities } }) } @@ -319,7 +319,7 @@ module.exports = function(options) { }, set innerHTML(value) { var voidElements = ["area", "base", "br", "col", "command", "embed", "hr", "img", "input", "keygen", "link", "meta", "param", "source", "track", "wbr"] - while (this.firstChild) this.removeChild(this.firstChild) + while (this.firstChild) removeChild.call(this, this.firstChild) var match = value.match(/^(.*)<\/svg>$/), root, ns if (match) { var value = match[1] @@ -659,12 +659,20 @@ module.exports = function(options) { nodeType: 3, nodeName: "#text", parentNode: null, + get childNodes() { return [] }, + get firstChild() { return null }, get nodeValue() {return nodeValue}, set nodeValue(value) { /*eslint-disable no-implicit-coercion*/ nodeValue = "" + value /*eslint-enable no-implicit-coercion*/ }, + get nextSibling() { + if (this.parentNode == null) return null + var index = this.parentNode.childNodes.indexOf(this) + if (index < 0) throw new TypeError("Parent's childNodes is out of sync") + return this.parentNode.childNodes[index + 1] || null + }, } }, createDocumentFragment: function() { @@ -691,9 +699,9 @@ module.exports = function(options) { }, } $window.document.documentElement = $window.document.createElement("html") - $window.document.documentElement.appendChild($window.document.createElement("head")) + appendChild.call($window.document.documentElement, $window.document.createElement("head")) $window.document.body = $window.document.createElement("body") - $window.document.documentElement.appendChild($window.document.body) + appendChild.call($window.document.documentElement, $window.document.body) activeElement = $window.document.body if (options.spy) $window.__getSpies = getSpies