chore(hydration): Test approach to comment markers

compat/test/browser/suspense-hydration.test.js

@@ -97,6 +97,95 @@ describe('suspense hydration', () => {
 		});
 	});
 
+	it('Should hydrate a fragment with multiple children correctly', () => {
+		scratch.innerHTML = '<!--$s--><div>Hello</div><div>World!</div><!--/$s-->';
+		clearLog();
+
+		const [Lazy, resolve] = createLazy();
+		hydrate(
+			<Suspense>
+				<Lazy />
+			</Suspense>,
+			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(getLog()).to.deep.equal([]);
+		clearLog();
+
+		return resolve(() => (
+			<>
+				<div>Hello</div>
+				<div>World!</div>
+			</>
+		)).then(() => {
+			rerender();
+			expect(scratch.innerHTML).to.equal(
+				'<!--$s--><div>Hello</div><div>World!</div><!--/$s-->'
+			);
+			expect(getLog()).to.deep.equal([]);
+
+			clearLog();
+		});
+	});
+
+	it('Should hydrate a fragment with no children correctly', () => {
+		scratch.innerHTML = '<!--$s--><!--/$s-->';
+		clearLog();
+
+		const [Lazy, resolve] = createLazy();
+		hydrate(
+			<Suspense>
+				<Lazy />
+			</Suspense>,
+			scratch
+		);
+		rerender(); // Flush rerender queue to mimic what preact will really do
+		expect(scratch.innerHTML).to.equal('<!--$s--><!--/$s-->');
+		expect(getLog()).to.deep.equal([]);
+		clearLog();
+
+		return resolve(() => null).then(() => {
+			rerender();
+			expect(scratch.innerHTML).to.equal('<!--$s--><!--/$s-->');
+			expect(getLog()).to.deep.equal([]);
+
+			clearLog();
+		});
+	});
+
+	// This is in theory correct but still it shows that our oldDom becomes stale very quickly
+	// and moves DOM into weird places
+	it('Should hydrate a fragment with no children and an adjacent node correctly', () => {
+		scratch.innerHTML = '<!--$s--><!--/$s--><div>Baz</div>';
+		clearLog();
+
+		const [Lazy, resolve] = createLazy();
+		hydrate(
+			<>
+				<Suspense>
+					<Lazy />
+				</Suspense>
+				<div>Baz</div>
+			</>,
+			scratch
+		);
+		rerender(); // Flush rerender queue to mimic what preact will really do
+		expect(scratch.innerHTML).to.equal('<!--$s--><!--/$s--><div>Baz</div>');
+		expect(getLog()).to.deep.equal([]);
+		clearLog();
+
+		return resolve(() => null).then(() => {
+			rerender();
+			expect(scratch.innerHTML).to.equal('<!--$s--><!--/$s--><div>Baz</div>');
+			expect(getLog()).to.deep.equal([]);
+
+			clearLog();
+		});
+	});
+
 	it('should properly attach event listeners when suspending while hydrating', () => {
 		scratch.innerHTML = '<div>Hello</div><div>World</div>';
 		clearLog();

mangle.json

@@ -31,6 +31,7 @@
       "$_list": "__",
       "$_pendingEffects": "__h",
       "$_value": "__",
+      "$_excess": "__x",
       "$_nextValue": "__N",
       "$_original": "__v",
       "$_args": "__H",

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,8 +52,16 @@ export function diff(
 	// If the previous diff bailed out, resume creating/hydrating.
 	if (oldVNode._flags & MODE_SUSPENDED) {
 		isHydrating = !!(oldVNode._flags & MODE_HYDRATE);
-		oldDom = newVNode._dom = oldVNode._dom;
-		excessDomChildren = [oldDom];
+		if (oldVNode._component._excess) {
+			excessDomChildren = oldVNode._component._excess;
+			// TODO: it's entirely possible for nested Suspense scenario's that we
+			// take another comment-node here as oldDom which isn't ideal however
+			// let's try it out for now.
+			oldDom = newVNode._dom = oldVNode._dom = excessDomChildren[0];
+		} else {
+			oldDom = newVNode._dom = oldVNode._dom;
+			excessDomChildren = [oldDom];
+		}
 	}
 
 	if ((tmp = options._diff)) tmp(newVNode);
@@ -273,13 +281,42 @@ export function diff(
 			newVNode._original = null;
 			// if hydrating or creating initial tree, bailout preserves DOM:
 			if (isHydrating || excessDomChildren != null) {
-				newVNode._dom = oldDom;
 				newVNode._flags |= isHydrating
 					? MODE_HYDRATE | MODE_SUSPENDED
 					: MODE_HYDRATE;
-				excessDomChildren[excessDomChildren.indexOf(oldDom)] = null;
-				// ^ could possibly be simplified to:
-				// excessDomChildren.length = 0;
+
+				let found = excessDomChildren.find(
+					child => child && child.nodeType == 8 && child.data == '$s'
+				);
+
+				newVNode._dom = oldDom;
+				if (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++;
+						// 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 {
+					excessDomChildren[excessDomChildren.indexOf(oldDom)] = null;
+				}
 			} else {
 				newVNode._dom = oldVNode._dom;
 				newVNode._children = oldVNode._children;

src/internal.d.ts

@@ -162,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