From e6edf13a95af061146e5ebdf0b1d92497f265840 Mon Sep 17 00:00:00 2001
From: Andre Wiggins <459878+andrewiggins@users.noreply.github.com>
Date: Tue, 24 Oct 2023 11:49:54 -0700
Subject: [PATCH] Always clear `_nextDom` field on VNodes (#4166)

* Always clear _nextDom

* Remove reference to _nextDom in Suspense

_nextDom was always null/undefined in our test suite in this code path so replacing it with a direct call to appendChild.

* Clear render root _nextDom in commitRoot

* improve types in diffChildren

* Undo variable order change

Locally improves perf??
---
 compat/src/suspense.js |  2 +-
 src/diff/children.js   | 32 ++++++++++++++++----------------
 src/diff/index.js      |  2 ++
 test/_util/helpers.js  | 23 +++++++++++++++++++++++
 4 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/compat/src/suspense.js b/compat/src/suspense.js
index 299b492759..83c6ec699f 100644
--- a/compat/src/suspense.js
+++ b/compat/src/suspense.js
@@ -81,7 +81,7 @@ function removeOriginal(vnode, detachedParent, originalParent) {
 		if (vnode._component) {
 			if (vnode._component._parentDom === detachedParent) {
 				if (vnode._dom) {
-					originalParent.insertBefore(vnode._dom, vnode._nextDom);
+					originalParent.appendChild(vnode._dom);
 				}
 				vnode._component._force = true;
 				vnode._component._parentDom = originalParent;
diff --git a/src/diff/children.js b/src/diff/children.js
index c39e54dc2a..49a2e0c360 100644
--- a/src/diff/children.js
+++ b/src/diff/children.js
@@ -196,28 +196,28 @@ export function diffChildren(
 
 			skewedIndex = i + skew;
 
-			if (
-				typeof childVNode.type == 'function' &&
-				(matchingIndex !== skewedIndex ||
-					oldVNode._children === childVNode._children)
-			) {
-				oldDom = reorderChildren(childVNode, oldDom, parentDom);
-			} else if (
-				typeof childVNode.type != 'function' &&
-				(matchingIndex !== skewedIndex || isMounting)
-			) {
-				oldDom = placeChild(parentDom, newDom, oldDom);
-			} else if (childVNode._nextDom !== undefined) {
-				// Only Fragments or components that return Fragment like VNodes will
-				// have a non-undefined _nextDom. Continue the diff from the sibling
-				// of last DOM child of this child VNode
-				oldDom = childVNode._nextDom;
+			if (typeof childVNode.type == 'function') {
+				if (
+					matchingIndex !== skewedIndex ||
+					oldVNode._children === childVNode._children
+				) {
+					oldDom = reorderChildren(childVNode, oldDom, parentDom);
+				} else if (childVNode._nextDom !== undefined) {
+					// Only Fragments or components that return Fragment like VNodes will
+					// have a non-undefined _nextDom. Continue the diff from the sibling
+					// of last DOM child of this child VNode
+					oldDom = childVNode._nextDom;
+				} else {
+					oldDom = newDom.nextSibling;
+				}
 
 				// Eagerly cleanup _nextDom. We don't need to persist the value because
 				// it is only used by `diffChildren` to determine where to resume the diff after
 				// diffing Components and Fragments. Once we store it the nextDOM local var, we
 				// can clean up the property
 				childVNode._nextDom = undefined;
+			} else if (matchingIndex !== skewedIndex || isMounting) {
+				oldDom = placeChild(parentDom, newDom, oldDom);
 			} else {
 				oldDom = newDom.nextSibling;
 			}
diff --git a/src/diff/index.js b/src/diff/index.js
index 2cd24bbb0f..3cb99a34b5 100644
--- a/src/diff/index.js
+++ b/src/diff/index.js
@@ -298,6 +298,8 @@ export function diff(
  * @param {import('../internal').VNode} root
  */
 export function commitRoot(commitQueue, root, refQueue) {
+	root._nextDom = undefined;
+
 	for (let i = 0; i < refQueue.length; i++) {
 		applyRef(refQueue[i], refQueue[++i], refQueue[++i]);
 	}
diff --git a/test/_util/helpers.js b/test/_util/helpers.js
index ab8d0d3f92..2f2bcecdfa 100644
--- a/test/_util/helpers.js
+++ b/test/_util/helpers.js
@@ -208,6 +208,14 @@ export function clearOptions() {
  * @param {HTMLElement} scratch
  */
 export function teardown(scratch) {
+	if (
+		scratch &&
+		('__k' in scratch || '_children' in scratch) &&
+		scratch._children
+	) {
+		verifyVNodeTree(scratch._children);
+	}
+
 	if (scratch) {
 		scratch.parentNode.removeChild(scratch);
 	}
@@ -226,6 +234,21 @@ export function teardown(scratch) {
 	restoreElementAttributes();
 }
 
+/** @type {(vnode: import('../../src/internal').VNode) => void} */
+function verifyVNodeTree(vnode) {
+	if (vnode._nextDom) {
+		expect.fail('vnode should not have _nextDom:' + vnode._nextDom);
+	}
+
+	if (vnode._children) {
+		for (let child of vnode._children) {
+			if (child) {
+				verifyVNodeTree(child);
+			}
+		}
+	}
+}
+
 const Foo = () => 'd';
 export const getMixedArray = () =>
 	// Make it a function so each test gets a new copy of the array