Fix issues by correctly skipping comments in diffChildren

Author: JoviDeCroock

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();
 

jsx-runtime/src/index.js

@@ -59,7 +59,6 @@ function createVNode(type, props, key, isStaticChildren, __source, __self) {
 		_nextDom: undefined,
 		_component: null,
 		constructor: undefined,
-		_excess: null,
 		_original: --vnodeId,
 		_index: -1,
 		_flags: 0,

src/create-element.js

@@ -66,7 +66,6 @@ export function createVNode(type, props, key, ref, original) {
 		_parent: null,
 		_depth: 0,
 		_dom: null,
-		_excess: null,
 		// _nextDom must be initialized to undefined b/c it will eventually
 		// be set to dom.nextSibling which can return `null` and it is important
 		// to be able to distinguish between an uninitialized _nextDom and

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.nextSibling) {
+				oldDom = oldDom.nextSibling;
+			}
 		}
 
 		// Eagerly cleanup _nextDom. We don't need to persist the value because it

src/diff/index.js

@@ -52,9 +52,9 @@ export function diff(
 	// If the previous diff bailed out, resume creating/hydrating.
 	if (oldVNode._flags & MODE_SUSPENDED) {
 		isHydrating = !!(oldVNode._flags & MODE_HYDRATE);
-		if (oldVNode._excess) {
-			excessDomChildren = oldVNode._excess;
-			oldDom = newVNode._dom = oldVNode._dom = excessDomChildren[1];
+		if (oldVNode._component._excess) {
+			excessDomChildren = oldVNode._component._excess;
+			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._component._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._component._excess.push(node);
 						}
 					}
 				} else {

src/internal.d.ts

@@ -145,7 +145,6 @@ declare global {
 		 * The [first (for Fragments)] DOM child of a VNode
 		 */
 		_dom: PreactElement | null;
-		_excess: PreactElement[] | null;
 		/**
 		 * The last dom child of a Fragment, or components that return a Fragment
 		 */
@@ -163,6 +162,7 @@ declare global {
 		state: S; // Override Component["state"] to not be readonly for internal use, specifically Hooks
 		base?: PreactElement;
 
+		_excess: PreactElement[] | null;
 		_dirty: boolean;
 		_force?: boolean;
 		_renderCallbacks: Array<() => void>; // Only class components