Fix issues by correctly skipping comments in diffChildren

JoviDeCroock · JoviDeCroock · commit 647d13865b3c · 2024-07-15T10:50:52.000+02:00
diff --git a/compat/test/browser/suspense-hydration.test.js b/compat/test/browser/suspense-hydration.test.js
@@ -132,7 +132,7 @@ describe('suspense hydration', () => {
 	});
 
 	it('Should hydrate a fragment with no children correctly', () => {
-		scratch.innerHTML = '<!--$s--><div>Hello</div><div>World!</div><!--/$s-->';
+		scratch.innerHTML = '<!--$s--><!--/$s-->';
 		clearLog();
 
 		const [Lazy, resolve] = createLazy();
@@ -143,22 +143,13 @@ describe('suspense hydration', () => {
 			scratch
 		);
 		rerender(); // Flush rerender queue to mimic what preact will really do
-		expect(scratch.innerHTML).to.equal(
-			'<!--$s--><div>Hello</div><div>World!</div><!--/$s-->'
-		);
+		expect(scratch.innerHTML).to.equal('<!--$s--><!--/$s-->');
 		expect(getLog()).to.deep.equal([]);
 		clearLog();
 
-		return resolve(() => (
-			<>
-				<div>Hello</div>
-				<div>World!</div>
-			</>
-		)).then(() => {
+		return resolve(() => null).then(() => {
 			rerender();
-			expect(scratch.innerHTML).to.equal(
-				'<!--$s--><div>Hello</div><div>World!</div><!--/$s-->'
-			);
+			expect(scratch.innerHTML).to.equal('<!--$s--><!--/$s-->');
 			expect(getLog()).to.deep.equal([]);
 
 			clearLog();
@@ -167,7 +158,7 @@ describe('suspense hydration', () => {
 
 	// This is in theory correct but still it shows that our oldDom becomes stale very quickly
 	// and moves DOM into weird places
-	it.skip('Should hydrate a fragment with no children and an adjacent node correctly', () => {
+	it('Should hydrate a fragment with no children and an adjacent node correctly', () => {
 		scratch.innerHTML = '<!--$s--><!--/$s--><div>Baz</div>';
 		clearLog();
 
diff --git a/src/component.js b/src/component.js
@@ -100,7 +100,11 @@ export function getDomSibling(vnode, childIndex) {
 	for (; childIndex < vnode._children.length; childIndex++) {
 		sibling = vnode._children[childIndex];
 
-		if (sibling != null && sibling._dom != null) {
+		if (
+			sibling != null &&
+			sibling._dom != null &&
+			sibling._dom.nodeType !== 8
+		) {
 			// Since updateParentDomPointers keeps _dom pointer correct,
 			// we can rely on _dom to tell us if this subtree contains a
 			// rendered DOM node, and what the first rendered DOM node is
diff --git a/src/diff/children.js b/src/diff/children.js
@@ -135,6 +135,9 @@ export function diffChildren(
 			oldDom = childVNode._nextDom;
 		} else if (newDom) {
 			oldDom = newDom.nextSibling;
+			while (oldDom && oldDom.nodeType == 8) {
+				oldDom = oldDom.nextSibling;
+			}
 		}
 
 		// Eagerly cleanup _nextDom. We don't need to persist the value because it
diff --git a/src/diff/index.js b/src/diff/index.js
@@ -54,7 +54,7 @@ export function diff(
 		isHydrating = !!(oldVNode._flags & MODE_HYDRATE);
 		if (oldVNode._excess) {
 			excessDomChildren = oldVNode._excess;
-			oldDom = newVNode._dom = oldVNode._dom = excessDomChildren[1];
+			oldDom = newVNode._dom = oldVNode._dom = excessDomChildren[0];
 		} else {
 			oldDom = newVNode._dom = oldVNode._dom;
 			excessDomChildren = [oldDom];
@@ -283,26 +283,32 @@ export function diff(
 					: MODE_HYDRATE;
 
 				let found = excessDomChildren.find(
-						child => child && child.nodeType == 8 && child.data == '$s'
-					),
-					index = excessDomChildren.indexOf(found) + 1;
+					child => child && child.nodeType == 8 && child.data == '$s'
+				);
 
 				newVNode._dom = oldDom;
 				if (found) {
-					let commentMarkersToFind = 1;
-					newVNode._excess = [found];
+					let commentMarkersToFind = 1,
+						index = excessDomChildren.indexOf(found) + 1;
+					newVNode._excess = [];
+					// Clear the comment marker so we don't reuse them for sibling
+					// Suspenders.
 					excessDomChildren[index - 1] = null;
+
 					while (commentMarkersToFind && index <= excessDomChildren.length) {
 						const node = excessDomChildren[index];
 						excessDomChildren[index] = null;
 						index++;
-						newVNode._excess.push(node);
+						// node being undefined here would be a problem as it would
+						// imply that we have a mismatch.
 						if (node.nodeType == 8) {
 							if (node.data == '$s') {
 								commentMarkersToFind++;
 							} else if (node.data == '/$s') {
 								commentMarkersToFind--;
 							}
+						} else {
+							newVNode._excess.push(node);
 						}
 					}
 				} else {

Original file line number	Diff line number	Diff line change
`@@ -135,6 +135,9 @@ export function diffChildren(`
`135`	`135`	`oldDom = childVNode._nextDom;`
`136`	`136`	`} else if (newDom) {`
`137`	`137`	`oldDom = newDom.nextSibling;`
	`138`	`+ while (oldDom && oldDom.nodeType == 8) {`
	`139`	`+ oldDom = oldDom.nextSibling;`
	`140`	`+ }`
`138`	`141`	`}`
`139`	`142`
`140`	`143`	`// Eagerly cleanup _nextDom. We don't need to persist the value because it`