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,11 @@ 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];
+		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];
 	}
 
 	if ((tmp = options._diff)) tmp(newVNode);
@@ -273,13 +276,55 @@ 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 shouldFallback = true,
+					commentMarkersToFind = 0,
+					done = false;
+
+				newVNode._component._excess = [];
+				for (let i = 0; i < excessDomChildren.length; i++) {
+					let child = excessDomChildren[i];
+					if (child == null || done) continue;
+
+					// When we are inside of the boundaries we set
+					// the excess child to null.
+					if (commentMarkersToFind > 0) {
+						excessDomChildren[i] = null;
+					}
+
+					// When we encounter a boundary with $s we are opening
+					// a boundary, this implies that we need to bump
+					// the amount of markers we need to find before closing
+					// the outer boundary.
+					// We exclude the open and closing marker from
+					// the future excessDomChildren but any nested one
+					// needs to be included for future suspensions.
+					if (child.nodeType == 8 && child.data == '$s') {
+						if (commentMarkersToFind > 0) {
+							newVNode._component._excess.push(child);
+						}
+						commentMarkersToFind++;
+						shouldFallback = false;
+					} else if (child.nodeType == 8 && child.data == '/$s') {
+						commentMarkersToFind--;
+						if (commentMarkersToFind > 0) {
+							newVNode._component._excess.push(child);
+						}
+						done = commentMarkersToFind === 0;
+					} else if (commentMarkersToFind > 0) {
+						newVNode._component._excess.push(child);
+					}
+				}
+
+				if (shouldFallback) {
+					excessDomChildren[excessDomChildren.indexOf(oldDom)] = null;
+					newVNode._component._excess.push(oldDom);
+				}
+
+				newVNode._dom = oldDom;
 			} 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