Skip to content

Commit c50e6be

Browse files
kfulekfule
authored
Fix nextSibling bugs and refactor updateNodes() and its fuzzer (#3059)
1 parent b88383d commit c50e6be

File tree

3 files changed

+114
-50
lines changed

3 files changed

+114
-50
lines changed

‎render/render.js‎

Lines changed: 35 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -274,32 +274,32 @@ module.exports = function() {
274274
else {
275275
var isOldKeyed = old[0] != null && old[0].key != null
276276
var isKeyed = vnodes[0] != null && vnodes[0].key != null
277-
var start = 0, oldStart = 0
278-
if (!isOldKeyed) while (oldStart < old.length && old[oldStart] == null) oldStart++
279-
if (!isKeyed) while (start < vnodes.length && vnodes[start] == null) start++
277+
var start = 0, oldStart = 0, o, v
280278
if (isOldKeyed !== isKeyed) {
281-
removeNodes(parent, old, oldStart, old.length)
282-
createNodes(parent, vnodes, start, vnodes.length, hooks, nextSibling, ns)
279+
removeNodes(parent, old, 0, old.length)
280+
createNodes(parent, vnodes, 0, vnodes.length, hooks, nextSibling, ns)
283281
} else if (!isKeyed) {
284282
// Don't index past the end of either list (causes deopts).
285283
var commonLength = old.length < vnodes.length ? old.length : vnodes.length
286284
// Rewind if necessary to the first non-null index on either side.
287285
// We could alternatively either explicitly create or remove nodes when `start !== oldStart`
288286
// but that would be optimizing for sparse lists which are more rare than dense ones.
287+
while (oldStart < old.length && old[oldStart] == null) oldStart++
288+
while (start < vnodes.length && vnodes[start] == null) start++
289289
start = start < oldStart ? start : oldStart
290290
for (; start < commonLength; start++) {
291291
o = old[start]
292292
v = vnodes[start]
293293
if (o === v || o == null && v == null) continue
294-
else if (o == null) createNode(parent, v, hooks, ns, getNextSibling(old, start + 1, nextSibling))
294+
else if (o == null) createNode(parent, v, hooks, ns, getNextSibling(old, start + 1, old.length, nextSibling))
295295
else if (v == null) removeNode(parent, o)
296-
else updateNode(parent, o, v, hooks, getNextSibling(old, start + 1, nextSibling), ns)
296+
else updateNode(parent, o, v, hooks, getNextSibling(old, start + 1, old.length, nextSibling), ns)
297297
}
298298
if (old.length > commonLength) removeNodes(parent, old, start, old.length)
299299
if (vnodes.length > commonLength) createNodes(parent, vnodes, start, vnodes.length, hooks, nextSibling, ns)
300300
} else {
301301
// keyed diff
302-
var oldEnd = old.length - 1, end = vnodes.length - 1, map, o, v, oe, ve, topSibling
302+
var oldEnd = old.length - 1, end = vnodes.length - 1, oe, ve, topSibling
303303

304304
// bottom-up
305305
while (oldEnd >= oldStart && end >= start) {
@@ -316,13 +316,13 @@ module.exports = function() {
316316
v = vnodes[start]
317317
if (o.key !== v.key) break
318318
oldStart++, start++
319-
if (o !== v) updateNode(parent, o, v, hooks, getNextSibling(old, oldStart, nextSibling), ns)
319+
if (o !== v) updateNode(parent, o, v, hooks, getNextSibling(old, oldStart, oldEnd + 1, nextSibling), ns)
320320
}
321321
// swaps and list reversals
322322
while (oldEnd >= oldStart && end >= start) {
323323
if (start === end) break
324324
if (o.key !== ve.key || oe.key !== v.key) break
325-
topSibling = getNextSibling(old, oldStart, nextSibling)
325+
topSibling = getNextSibling(old, oldStart, oldEnd, nextSibling)
326326
moveDOM(parent, oe, topSibling)
327327
if (oe !== v) updateNode(parent, oe, v, hooks, topSibling, ns)
328328
if (++start <= --end) moveDOM(parent, o, nextSibling)
@@ -347,17 +347,18 @@ module.exports = function() {
347347
else if (oldStart > oldEnd) createNodes(parent, vnodes, start, end + 1, hooks, nextSibling, ns)
348348
else {
349349
// inspired by ivi https://github.com/ivijs/ivi/ by Boris Kaul
350-
var originalNextSibling = nextSibling, vnodesLength = end - start + 1, oldIndices = new Array(vnodesLength), li=0, i=0, pos = 2147483647, matched = 0, map, lisIndices
351-
for (i = 0; i < vnodesLength; i++) oldIndices[i] = -1
352-
for (i = end; i >= start; i--) {
353-
if (map == null) map = getKeyMap(old, oldStart, oldEnd + 1)
354-
ve = vnodes[i]
355-
var oldIndex = map[ve.key]
356-
if (oldIndex != null) {
357-
pos = (oldIndex < pos) ? oldIndex : -1 // becomes -1 if nodes were re-ordered
358-
oldIndices[i-start] = oldIndex
359-
oe = old[oldIndex]
360-
old[oldIndex] = null
350+
var originalNextSibling = nextSibling, pos = 2147483647, matched = 0
351+
var oldIndices = new Array(end - start + 1).fill(-1)
352+
var map = Object.create(null)
353+
for (var i = start; i <= end; i++) map[vnodes[i].key] = i
354+
for (var i = oldEnd; i >= oldStart; i--) {
355+
oe = old[i]
356+
var newIndex = map[oe.key]
357+
if (newIndex != null) {
358+
pos = (newIndex < pos) ? newIndex : -1 // becomes -1 if nodes were re-ordered
359+
oldIndices[newIndex-start] = i
360+
ve = vnodes[newIndex]
361+
old[i] = null
361362
if (oe !== ve) updateNode(parent, oe, ve, hooks, nextSibling, ns)
362363
if (ve.dom != null) nextSibling = ve.dom
363364
matched++
@@ -370,22 +371,22 @@ module.exports = function() {
370371
if (pos === -1) {
371372
// the indices of the indices of the items that are part of the
372373
// longest increasing subsequence in the oldIndices list
373-
lisIndices = makeLisIndices(oldIndices)
374-
li = lisIndices.length - 1
375-
for (i = end; i >= start; i--) {
376-
v = vnodes[i]
377-
if (oldIndices[i-start] === -1) createNode(parent, v, hooks, ns, nextSibling)
374+
var lisIndices = makeLisIndices(oldIndices)
375+
var li = lisIndices.length - 1
376+
for (var i = end; i >= start; i--) {
377+
ve = vnodes[i]
378+
if (oldIndices[i-start] === -1) createNode(parent, ve, hooks, ns, nextSibling)
378379
else {
379380
if (lisIndices[li] === i - start) li--
380-
else moveDOM(parent, v, nextSibling)
381+
else moveDOM(parent, ve, nextSibling)
381382
}
382-
if (v.dom != null) nextSibling = vnodes[i].dom
383+
if (ve.dom != null) nextSibling = ve.dom
383384
}
384385
} else {
385-
for (i = end; i >= start; i--) {
386-
v = vnodes[i]
387-
if (oldIndices[i-start] === -1) createNode(parent, v, hooks, ns, nextSibling)
388-
if (v.dom != null) nextSibling = vnodes[i].dom
386+
for (var i = end; i >= start; i--) {
387+
ve = vnodes[i]
388+
if (oldIndices[i-start] === -1) createNode(parent, ve, hooks, ns, nextSibling)
389+
if (ve.dom != null) nextSibling = ve.dom
389390
}
390391
}
391392
}
@@ -475,17 +476,6 @@ module.exports = function() {
475476
vnode.domSize = 0
476477
}
477478
}
478-
function getKeyMap(vnodes, start, end) {
479-
var map = Object.create(null)
480-
for (; start < end; start++) {
481-
var vnode = vnodes[start]
482-
if (vnode != null) {
483-
var key = vnode.key
484-
if (key != null) map[key] = start
485-
}
486-
}
487-
return map
488-
}
489479
// Lifted from ivi https://github.com/ivijs/ivi/
490480
// takes a list of unique numbers (-1 is special and can
491481
// occur multiple times) and returns an array with the indices
@@ -533,8 +523,8 @@ module.exports = function() {
533523
return result
534524
}
535525

536-
function getNextSibling(vnodes, i, nextSibling) {
537-
for (; i < vnodes.length; i++) {
526+
function getNextSibling(vnodes, i, end, nextSibling) {
527+
for (; i < end; i++) {
538528
if (vnodes[i] != null && vnodes[i].dom != null) return vnodes[i].dom
539529
}
540530
return nextSibling

‎render/tests/test-updateNodes.js‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1169,6 +1169,51 @@ o.spec("updateNodes", function() {
11691169
o(root.appendChild.callCount + root.insertBefore.callCount).equals(5)
11701170
o(tagNames).deepEquals(expectedTagNames)
11711171
})
1172+
o("update keyed element vnodes with another tag (#3059)", function() {
1173+
var e = function(k) {return m(k, {key: k})} // element vnode
1174+
var p = function(k) {return m(k + "p", {key: k})} // element vnode (another tag)
1175+
1176+
const o1 = [e("k1"),e("k2"),e("k3"),e("k4")]
1177+
const v1 = [p("k2"),e("k4"),e("k3")]
1178+
1179+
// create
1180+
render(root, v1)
1181+
o(Array.from(root.childNodes).map(function(n) {return n.nodeName})).deepEquals(["K2P", "K4", "K3"])
1182+
1183+
// update
1184+
render(root, [])
1185+
render(root, o1)
1186+
render(root, v1)
1187+
o(Array.from(root.childNodes).map(function(n) {return n.nodeName})).deepEquals(["K2P", "K4", "K3"])
1188+
})
1189+
o("update keyed element vnodes with dom and keyed fragment vnodes without dom (1) (#3059)", function() {
1190+
o(function() {
1191+
var e = function(k) {return m(k, {key: k})} // element vnode (with dom)
1192+
var f = function(k) {return m("[", {key: k})} // fragment vnode (without dom)
1193+
1194+
var o1 = [f("k1"),e("k2")]
1195+
var v1 = [e("k1"),e("a"),f("k2")]
1196+
1197+
render(root, o1)
1198+
render(root, v1)
1199+
1200+
o(Array.from(root.childNodes).map(function(n) {return n.nodeName})).deepEquals(["K1", "A"])
1201+
}).notThrows(Error)
1202+
})
1203+
o("update keyed element vnodes with dom and keyed fragment vnodes without dom (2) (#3059)", function() {
1204+
o(function() {
1205+
var e = function(k) {return m(k, {key: k})} // element vnode (with dom)
1206+
var f = function(k) {return m("[", {key: k})} // fragment vnode (without dom)
1207+
1208+
var o1 = [f("k1"),f("k2"),e("k3")]
1209+
var v1 = [e("k1"),f("k3"),e("k2")]
1210+
1211+
render(root, o1)
1212+
render(root, v1)
1213+
1214+
o(Array.from(root.childNodes).map(function(n) {return n.nodeName})).deepEquals(["K1", "K2"])
1215+
}).notThrows(Error)
1216+
})
11721217

11731218
components.forEach(function(cmp){
11741219
o.spec(cmp.kind, function(){

‎render/tests/test-updateNodesFuzzer.js‎

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,18 +25,47 @@ o.spec("updateNodes keyed list Fuzzer", function() {
2525
var tests = 250
2626

2727
while (tests--) {
28-
var test = fuzzTest(c.delMax, c.movMax, c.insMax)
29-
o(i++ + ": " + test.list.join() + " -> " + test.updated.join(), function() {
28+
const test = fuzzTest(c.delMax, c.movMax, c.insMax)
29+
const id = i++
30+
o(id + ": " + test.list.join() + " -> " + test.updated.join(), function() {
3031
render(root, test.list.map(function(x){return m(x, {key: x})}))
3132
addSpies(root)
3233
render(root, test.updated.map(function(x){return m(x, {key: x})}))
3334

34-
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()}))
35-
36-
o(root.appendChild.callCount + root.insertBefore.callCount).equals(test.expected.creations + test.expected.moves)("moves")
35+
// FIXME: This does not take into account the "swaps and list reversals" heuristic in updateNodes().
36+
// Here, we’re checking whether the number of node moves matches the theoretical value derived from the LIS.
37+
// However, in updateNodes(), when patterns such as swaps or reversed lists are detected,
38+
// nodes are moved before the LIS-based reordering is applied.
39+
// Once these heuristic moves occur, the actual number of moves no longer matches the LIS-based theoretical value.
40+
// 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()}))
41+
//
42+
// o(root.appendChild.callCount + root.insertBefore.callCount).equals(test.expected.creations + test.expected.moves)("moves")
3743
o(root.removeChild.callCount).equals(test.expected.deletions)("deletions")
3844
o([].map.call(root.childNodes, function(n){return n.nodeName.toLowerCase()})).deepEquals(test.updated)
3945
})
46+
o(id + ": including tag changes", function() {
47+
// change some tags before and after the update
48+
var list = test.list.map(function(x){return m(x + (Math.random() > 0.5 ? "_" : ""), {key: x})})
49+
var updated = test.updated.map(function(x){return m(x, {key: x})})
50+
var str = list.map(function(v) {return v.tag}).join() + " -> " + updated.map(function(v) {return v.tag}).join()
51+
52+
render(root, list)
53+
render(root, updated)
54+
55+
o([].map.call(root.childNodes, function(n){return n.nodeName.toLowerCase()})).deepEquals(test.updated)(str)
56+
})
57+
o(id + ": including empty fragments (without dom)", function() {
58+
// change some vnodes to empty fragments without DOM before and after the update
59+
var list = test.list.map(function(x){return m(Math.random() > 0.5 ? x : "[", {key: x})})
60+
var updated = test.updated.map(function(x){return m(Math.random() > 0.5 ? x : "[", {key: x})})
61+
var expected = updated.map(function(v){return v.tag}).filter(function(x){return x !== "["})
62+
var str = list.map(function(v) {return v.tag + "." + v.key}).join() + " -> " + updated.map(function(v) {return v.tag + "." + v.key}).join()
63+
64+
render(root, list)
65+
render(root, updated)
66+
67+
o([].map.call(root.childNodes, function(n){return n.nodeName.toLowerCase()})).deepEquals(expected)(str)
68+
})
4069
}
4170
})
4271
})

0 commit comments

Comments
 (0)