Fix client hydration in experimentalReactChildren (#8898)

* Fix client hydration in experimentalReactChildren * Add tests * Add a changeset * Use recursion instead of walking * getChildren -> swap order --------- Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>
withastro · Oct 24, 2023 · 4dee387 · 4dee387
1 parent 5dd1ed5
commit 4dee387
Show file tree

Hide file tree

Showing 7 changed files with 75 additions and 27 deletions.
diff --git a/.changeset/real-dryers-destroy.md b/.changeset/real-dryers-destroy.md
@@ -0,0 +1,5 @@
+---
+'@astrojs/react': patch
+---
+
+Fixes client hydration in islands when using experimentalReactChildren
diff --git a/packages/integrations/react/client.js b/packages/integrations/react/client.js
@@ -10,6 +10,42 @@ function isAlreadyHydrated(element) {
 	}
 }
 
+function createReactElementFromDOMElement(element) {
+	let attrs = {};
+	for(const attr of element.attributes) {
+		attrs[attr.name] = attr.value;
+	}
+
+	return createElement(element.localName, attrs,
+		Array.from(element.childNodes).map(c => {
+			if(c.nodeType === Node.TEXT_NODE) {
+				return c.data;
+			} else if(c.nodeType === Node.ELEMENT_NODE) {
+				return createReactElementFromDOMElement(c)
+			} else {
+				return undefined;
+			}
+		}).filter(a => !!a)
+	);
+}
+
+function getChildren(childString, experimentalReactChildren) {
+	if(experimentalReactChildren && childString) {
+		let children = [];
+		let template = document.createElement('template');
+		template.innerHTML = childString;
+		for(let child of template.content.children) {
+			children.push(createReactElementFromDOMElement(child))
+		}
+		return children;
+	} else if(childString) {
+		return createElement(StaticHtml, { value: childString });
+	} else {
+		return undefined;
+	}
+
+}
+
 export default (element) =>
 	(Component, props, { default: children, ...slotted }, { client }) => {
 		if (!element.hasAttribute('ssr')) return;
@@ -19,10 +55,11 @@ export default (element) =>
 		for (const [key, value] of Object.entries(slotted)) {
 			props[key] = createElement(StaticHtml, { value, name: key });
 		}
+
 		const componentEl = createElement(
 			Component,
 			props,
-			children != null ? createElement(StaticHtml, { value: children }) : children
+			getChildren(children, element.hasAttribute('data-react-children'))
 		);
 		const rootKey = isAlreadyHydrated(element);
 		// HACK: delete internal react marker for nested components to suppress aggressive warnings

diff --git a/packages/integrations/react/server.js b/packages/integrations/react/server.js
@@ -87,6 +87,7 @@ async function renderToStaticMarkup(Component, props, { default: children, ...sl
 	};
 	const newChildren = children ?? props.children;
 	if (children && opts.experimentalReactChildren) {
+		attrs['data-react-children'] = true;
 		const convert = await import('./vnode-children.js').then((mod) => mod.default);
 		newProps.children = convert(children);
 	} else if (newChildren != null) {

diff --git a/packages/integrations/react/test/fixtures/react-component/src/components/WithChildren.jsx b/packages/integrations/react/test/fixtures/react-component/src/components/WithChildren.jsx
@@ -1,8 +1,8 @@
 import React from 'react';
 
-export default function ({ children }) {
+export default function ({ id, children }) {
 	return (
-		<div>
+		<div id={id}>
 			<div className="with-children">{children}</div>
 			<div className="with-children-count">{children.length}</div>
 		</div>

diff --git a/packages/integrations/react/test/fixtures/react-component/src/pages/children.astro b/packages/integrations/react/test/fixtures/react-component/src/pages/children.astro
@@ -7,7 +7,11 @@ import WithChildren from '../components/WithChildren';
     <!-- Head Stuff -->
   </head>
   <body>
-		<WithChildren>
+		<WithChildren id="one">
+			<div>child 1</div><div>child 2</div>
+		</WithChildren>
+
+		<WithChildren id="two" client:load>
 			<div>child 1</div><div>child 2</div>
 		</WithChildren>
   </body>

diff --git a/packages/integrations/react/test/react-component.test.js b/packages/integrations/react/test/react-component.test.js
@@ -105,7 +105,13 @@ describe('React Components', () => {
 		it('Children are parsed as React components, can be manipulated', async () => {
 			const html = await fixture.readFile('/children/index.html');
 			const $ = cheerioLoad(html);
-			expect($('.with-children-count').text()).to.equal('2');
+			expect($('#one .with-children-count').text()).to.equal('2');
+		});
+
+		it('Client children passes option to the client', async () => {
+			const html = await fixture.readFile('/children/index.html');
+			const $ = cheerioLoad(html);
+			expect($('[data-react-children]')).to.have.lengthOf(1);
 		});
 	});
 

diff --git a/packages/integrations/react/vnode-children.js b/packages/integrations/react/vnode-children.js
@@ -1,35 +1,30 @@
-import { parse, walkSync, DOCUMENT_NODE, ELEMENT_NODE, TEXT_NODE } from 'ultrahtml';
+import { parse, DOCUMENT_NODE, ELEMENT_NODE, TEXT_NODE } from 'ultrahtml';
 import { createElement, Fragment } from 'react';
 
 let ids = 0;
 export default function convert(children) {
-	const nodeMap = new WeakMap();
 	let doc = parse(children.toString().trim());
 	let id = ids++;
 	let key = 0;
-	let root = createElement(Fragment, { children: [] });
 
-	walkSync(doc, (node, parent, index) => {
-		let newNode = {};
-		if (node.type === DOCUMENT_NODE) {
-			nodeMap.set(node, root);
-		} else if (node.type === ELEMENT_NODE) {
-			const { class: className, ...props } = node.attributes;
-			// NOTE: do not manually pass `children`, React handles this internally
-			newNode = createElement(node.name, { ...props, className, key: `${id}-${key++}` });
-			nodeMap.set(node, newNode);
-			if (parent) {
-				const newParent = nodeMap.get(parent);
-				newParent.props.children[index] = newNode;
-			}
-		} else if (node.type === TEXT_NODE) {
-			newNode = node.value;
-			if (newNode.trim() && parent) {
-				const newParent = nodeMap.get(parent);
-				newParent.props.children[index] = newNode;
+	function createReactElementFromNode(node) {
+		const childVnodes = Array.isArray(node.children) ? node.children.map(child => {
+			if(child.type === ELEMENT_NODE) {
+				return createReactElementFromNode(child);
+			} else if(child.type === TEXT_NODE) {
+				// 0-length text gets omitted in JSX
+				return child.value.trim() ? child.value : undefined;
 			}
+		}).filter(n => !!n) : undefined;
+
+		if(node.type === DOCUMENT_NODE) {
+			return createElement(Fragment, {}, childVnodes);
+		} else if(node.type === ELEMENT_NODE) {
+			const { class: className, ...props } = node.attributes;
+			return createElement(node.name, { ...props, className, key: `${id}-${key++}` }, childVnodes);
 		}
-	});
+	}
 
+	const root = createReactElementFromNode(doc);
 	return root.props.children;
 }