From 434ea9cbb0cc6b21b8e9e984f23542457d1bd230 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Mon, 23 Oct 2023 10:20:26 -0400 Subject: [PATCH 1/5] Fix client hydration in experimentalReactChildren --- .changeset/real-dryers-destroy.md | 5 ++++ packages/integrations/react/client.js | 39 ++++++++++++++++++++++++++- packages/integrations/react/server.js | 1 + 3 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 .changeset/real-dryers-destroy.md diff --git a/.changeset/real-dryers-destroy.md b/.changeset/real-dryers-destroy.md new file mode 100644 index 000000000000..a1955197e44f --- /dev/null +++ b/.changeset/real-dryers-destroy.md @@ -0,0 +1,5 @@ +--- +'@astrojs/react': patch +--- + +Fix for experimentalReactChildren diff --git a/packages/integrations/react/client.js b/packages/integrations/react/client.js index dbd32c0c5a54..3d6570d8b35e 100644 --- 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(experimentalReactChildren, childString) { + 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(element.hasAttribute('data-react-children'), 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 index c2400accb673..26596289eb8c 100644 --- 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) { From d5798100705fbf94a79a7afbb4b32909d868daac Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Mon, 23 Oct 2023 13:51:44 -0400 Subject: [PATCH 2/5] Add tests --- .../react-component/src/components/WithChildren.jsx | 4 ++-- .../fixtures/react-component/src/pages/children.astro | 6 +++++- packages/integrations/react/test/react-component.test.js | 8 +++++++- packages/integrations/react/vnode-children.js | 2 +- 4 files changed, 15 insertions(+), 5 deletions(-) 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 index 500c0c6949b3..a522bf95e9f1 100644 --- 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 ( -
+
{children}
{children.length}
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 index 59595c26642c..3f83eafcb71e 100644 --- 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'; - + +
child 1
child 2
+ + +
child 1
child 2
 diff --git a/packages/integrations/react/test/react-component.test.js b/packages/integrations/react/test/react-component.test.js index 43df1d9e4f0e..96d60fd54d20 100644 --- 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 index ea5bc0869ee6..7e8eced476bb 100644 --- a/packages/integrations/react/vnode-children.js +++ b/packages/integrations/react/vnode-children.js @@ -16,7 +16,7 @@ export default function convert(children) { } 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++}` }); + newNode = createElement(node.name, { ...props, className, key: `${id}-${key++}` }, []); nodeMap.set(node, newNode); if (parent) { const newParent = nodeMap.get(parent); From 1a34c14f69f855fbaf10dd1203dff1ba754aa25e Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Mon, 23 Oct 2023 13:52:40 -0400 Subject: [PATCH 3/5] Add a changeset --- .changeset/real-dryers-destroy.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/real-dryers-destroy.md b/.changeset/real-dryers-destroy.md index a1955197e44f..d5195b4a4dea 100644 --- a/.changeset/real-dryers-destroy.md +++ b/.changeset/real-dryers-destroy.md @@ -2,4 +2,4 @@ '@astrojs/react': patch --- -Fix for experimentalReactChildren +Fixes client hydration in islands when using experimentalReactChildren From 65cd3cabc5d685aeb31077f5b78988fa97028ab9 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Mon, 23 Oct 2023 15:53:51 -0400 Subject: [PATCH 4/5] Use recursion instead of walking --- packages/integrations/react/vnode-children.js | 39 ++++++++----------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/packages/integrations/react/vnode-children.js b/packages/integrations/react/vnode-children.js index 7e8eced476bb..57a7fb66ff56 100644 --- 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; } From 1f838381bc0aa141d30d5b8a35bac7b3b998621d Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Mon, 23 Oct 2023 15:54:45 -0400 Subject: [PATCH 5/5] getChildren -> swap order --- packages/integrations/react/client.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/integrations/react/client.js b/packages/integrations/react/client.js index 3d6570d8b35e..73f43aa94563 100644 --- a/packages/integrations/react/client.js +++ b/packages/integrations/react/client.js @@ -29,7 +29,7 @@ function createReactElementFromDOMElement(element) { ); } -function getChildren(experimentalReactChildren, childString) { +function getChildren(childString, experimentalReactChildren) { if(experimentalReactChildren && childString) { let children = []; let template = document.createElement('template'); @@ -59,7 +59,7 @@ export default (element) => const componentEl = createElement( Component, props, - getChildren(element.hasAttribute('data-react-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