Match null placeholders using skewed index

andrewiggins · andrewiggins · commit 987b7bbf5393 · 2024-02-21T16:06:04.000-08:00
If we've skewed our matching before hitting a null placeholder (e.g. we've inserted or removed an unmatched node) then let's pick up matching null placeholders from the skewedIndex.

Note I don't think we need to adjust the skew when we find a null placeholder, we treat it as
"matching" the current node.
diff --git a/src/diff/children.js b/src/diff/children.js
@@ -227,9 +227,11 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) {
 			childVNode = newParentVNode._children[i] = childVNode;
 		}
 
+		const skewedIndex = i + skew;
+
 		// Handle unmounting null placeholders, i.e. VNode => null in unkeyed children
 		if (childVNode == null) {
-			oldVNode = oldChildren[i];
+			oldVNode = oldChildren[skewedIndex];
 			if (
 				oldVNode &&
 				oldVNode.key == null &&
@@ -238,8 +240,6 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) {
 			) {
 				if (oldVNode._dom == newParentVNode._nextDom) {
 					newParentVNode._nextDom = getDomSibling(oldVNode);
-				} else {
-					skew++;
 				}
 				unmount(oldVNode, oldVNode, false);
 
@@ -252,7 +252,7 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) {
 				// to unmount this VNode again seeing `_match==true`.  Further,
 				// getDomSibling doesn't know about _match and so would incorrectly
 				// assume DOM nodes in this subtree are mounted and usable.
-				oldChildren[i] = null;
+				oldChildren[skewedIndex] = null;
 				remainingOldChildren--;
 			}
 			continue;
@@ -261,7 +261,6 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) {
 		childVNode._parent = newParentVNode;
 		childVNode._depth = newParentVNode._depth + 1;
 
-		const skewedIndex = i + skew;
 		const matchingIndex = findMatchingIndex(
 			childVNode,
 			oldChildren,
diff --git a/test/browser/fragments.test.js b/test/browser/fragments.test.js
@@ -1304,7 +1304,7 @@ describe('Fragment', () => {
 			'rendering from true to false'
 		);
 		expectDomLogToBe(
-			['<li>1.remove()', '<li>2.remove()', '<ol>043.appendChild(<li>4)'],
+			['<li>1.remove()', '<li>2.remove()'],
 			'rendering from true to false'
 		);
 
diff --git a/test/browser/render.test.js b/test/browser/render.test.js
@@ -1360,6 +1360,7 @@ describe('render()', () => {
 	it('should not crash or repeatedly add the same child when replacing a matched vnode with null (mixed dom-types)', () => {
 		const B = () => <div>B</div>;
 
+		/** @type {() => void} */
 		let update;
 		class App extends Component {
 			constructor(props) {
@@ -1375,38 +1376,38 @@ describe('render()', () => {
 					return (
 						<div>
 							<B />
-							<div />
+							<div>C</div>
 						</div>
 					);
 				}
 				return (
 					<div>
-						<span />
+						<span>A</span>
 						{null}
 						<B />
-						<div />
+						<div>C</div>
 					</div>
 				);
 			}
 		}
 
 		render(<App />, scratch);
-		expect(scratch.innerHTML).to.equal('<div><div>B</div><div></div></div>');
+		expect(scratch.innerHTML).to.equal('<div><div>B</div><div>C</div></div>');
 
 		update();
 		rerender();
 		expect(scratch.innerHTML).to.equal(
-			'<div><span></span><div>B</div><div></div></div>'
+			'<div><span>A</span><div>B</div><div>C</div></div>'
 		);
 
 		update();
 		rerender();
-		expect(scratch.innerHTML).to.equal('<div><div>B</div><div></div></div>');
+		expect(scratch.innerHTML).to.equal('<div><div>B</div><div>C</div></div>');
 
 		update();
 		rerender();
 		expect(scratch.innerHTML).to.equal(
-			'<div><span></span><div>B</div><div></div></div>'
+			'<div><span>A</span><div>B</div><div>C</div></div>'
 		);
 	});
 });
