Prevent removal of nested slots within islands (#7093)

matthewp · web-flow · commit 3d525efc95cf · 2023-05-17T10:18:04.000-04:00
* Prevent removal of nested slots within islands

* Fix build errors
diff --git a/.changeset/unlucky-lamps-remember.md b/.changeset/unlucky-lamps-remember.md
@@ -0,0 +1,24 @@
+---
+'@astrojs/preact': minor
+'@astrojs/svelte': minor
+'@astrojs/react': minor
+'@astrojs/solid-js': minor
+'@astrojs/vue': minor
+'astro': minor
+---
+
+Prevent removal of nested slots within islands
+
+This change introduces a new flag that renderers can add called `supportsAstroStaticSlot`. What this does is let Astro know that the render is sending `<astro-static-slot>` as placeholder values for static (non-hydrated) slots which Astro will then remove.
+
+This change is completely backwards compatible, but fixes bugs caused by combining ssr-only and client-side framework components like so:
+
+```astro
+<Component>
+  <div>
+    <Component client:load>
+      <span>Nested</span>
+    </Component>
+  </div>
+</Component>
+```
diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts
@@ -94,6 +94,7 @@ export interface AstroComponentMetadata {
 	hydrateArgs?: any;
 	componentUrl?: string;
 	componentExport?: { value: string; namespace?: boolean };
+	astroStaticSlot: true;
 }
 
 /** The flags supported by the Astro CLI */
@@ -1718,6 +1719,7 @@ export interface SSRLoadedRenderer extends AstroRenderer {
 			html: string;
 			attrs?: Record<string, string>;
 		}>;
+		supportsAstroStaticSlot?: boolean;
 	};
 }
 
diff --git a/packages/astro/src/runtime/server/render/component.ts b/packages/astro/src/runtime/server/render/component.ts
@@ -54,6 +54,13 @@ function isHTMLComponent(Component: unknown) {
 	return Component && typeof Component === 'object' && (Component as any)['astro:html'];
 }
 
+const ASTRO_SLOT_EXP = /\<\/?astro-slot\b[^>]*>/g;
+const ASTRO_STATIC_SLOT_EXP = /\<\/?astro-static-slot\b[^>]*>/g;
+function removeStaticAstroSlot(html: string, supportsAstroStaticSlot: boolean) {
+	const exp = supportsAstroStaticSlot ? ASTRO_STATIC_SLOT_EXP : ASTRO_SLOT_EXP;
+	return html.replace(exp, '');
+}
+
 async function renderFrameworkComponent(
 	result: SSRResult,
 	displayName: string,
@@ -68,7 +75,10 @@ async function renderFrameworkComponent(
 	}
 
 	const { renderers, clientDirectives } = result._metadata;
-	const metadata: AstroComponentMetadata = { displayName };
+	const metadata: AstroComponentMetadata = {
+		astroStaticSlot: true,
+		displayName
+	};
 
 	const { hydration, isPage, props } = extractDirectives(_props, clientDirectives);
 	let html = '';
@@ -263,7 +273,7 @@ If you're still stuck, please open an issue on GitHub or join us at https://astr
 			if (isPage || renderer?.name === 'astro:jsx') {
 				yield html;
 			} else if (html && html.length > 0) {
-				yield markHTMLString(html.replace(/\<\/?astro-slot\b[^>]*>/g, ''));
+				yield markHTMLString(removeStaticAstroSlot(html, renderer?.ssr?.supportsAstroStaticSlot ?? false));
 			} else {
 				yield '';
 			}
@@ -288,7 +298,11 @@ If you're still stuck, please open an issue on GitHub or join us at https://astr
 	if (html) {
 		if (Object.keys(children).length > 0) {
 			for (const key of Object.keys(children)) {
-				if (!html.includes(key === 'default' ? `<astro-slot>` : `<astro-slot name="${key}">`)) {
+				let tagName = renderer?.ssr?.supportsAstroStaticSlot ?
+					!!metadata.hydrate ? 'astro-slot' : 'astro-static-slot'
+					: 'astro-slot';
+				let expectedHTML = key === 'default' ? `<${tagName}>` : `<${tagName} name="${key}">`;
+				if (!html.includes(expectedHTML)) {
 					unrenderedSlots.push(key);
 				}
 			}
diff --git a/packages/astro/src/vite-plugin-jsx/index.ts b/packages/astro/src/vite-plugin-jsx/index.ts
@@ -202,7 +202,7 @@ export default function jsx({ settings, logging }: AstroPluginJSXOptions): Plugi
 Unable to resolve a renderer that handles this file! With more than one renderer enabled, you should include an import or use a pragma comment.
 Add ${colors.cyan(
 						IMPORT_STATEMENTS[defaultRendererName] || `import '${defaultRendererName}';`
-					)} or ${colors.cyan(`/* jsxImportSource: ${defaultRendererName} */`)} to this file.
+					)} or ${colors.cyan(`/** @jsxImportSource: ${defaultRendererName} */`)} to this file.
 `
 				);
 				return null;
diff --git a/packages/astro/test/astro-slots-nested.test.js b/packages/astro/test/astro-slots-nested.test.js
@@ -3,6 +3,7 @@ import * as cheerio from 'cheerio';
 import { loadFixture } from './test-utils.js';
 
 describe('Nested Slots', () => {
+	/** @type {import('./test-utils').Fixture} */
 	let fixture;
 
 	before(async () => {
@@ -23,4 +24,38 @@ describe('Nested Slots', () => {
 		const $ = cheerio.load(html);
 		expect($('script')).to.have.a.lengthOf(1, 'script rendered');
 	});
+
+	describe('Client components nested inside server-only framework components', () => {
+		/** @type {cheerio.CheerioAPI} */
+		let $;
+		before(async () => {
+			const html = await fixture.readFile('/server-component-nested/index.html');
+			$ = cheerio.load(html);
+		});
+
+		it('react', () => {
+			expect($('#react astro-slot')).to.have.a.lengthOf(1);
+			expect($('#react astro-static-slot')).to.have.a.lengthOf(0);
+		});
+
+		it('vue', () => {
+			expect($('#vue astro-slot')).to.have.a.lengthOf(1);
+			expect($('#vue astro-static-slot')).to.have.a.lengthOf(0);
+		});
+
+		it('preact', () => {
+			expect($('#preact astro-slot')).to.have.a.lengthOf(1);
+			expect($('#preact astro-static-slot')).to.have.a.lengthOf(0);
+		});
+
+		it('solid', () => {
+			expect($('#solid astro-slot')).to.have.a.lengthOf(1);
+			expect($('#solid astro-static-slot')).to.have.a.lengthOf(0);
+		});
+
+		it('svelte', () => {
+			expect($('#svelte astro-slot')).to.have.a.lengthOf(1);
+			expect($('#svelte astro-static-slot')).to.have.a.lengthOf(0);
+		});
+	});
 });
diff --git a/packages/astro/test/fixtures/astro-slots-nested/astro.config.mjs b/packages/astro/test/fixtures/astro-slots-nested/astro.config.mjs
@@ -1,6 +1,16 @@
 import { defineConfig } from 'astro/config';
 import react from '@astrojs/react';
+import preact from '@astrojs/preact';
+import solid from '@astrojs/solid-js';
+import svelte from '@astrojs/svelte';
+import vue from '@astrojs/vue';
 
 export default defineConfig({
-	integrations: [react()]
+	integrations: [
+		react(),
+		preact(),
+		solid(),
+		svelte(),
+		vue()
+	]
 });
diff --git a/packages/astro/test/fixtures/astro-slots-nested/package.json b/packages/astro/test/fixtures/astro-slots-nested/package.json
@@ -3,9 +3,17 @@
   "version": "0.0.0",
   "private": true,
   "dependencies": {
+    "@astrojs/preact": "workspace:*",
     "@astrojs/react": "workspace:*",
+    "@astrojs/vue": "workspace:*",
+    "@astrojs/solid-js": "workspace:*",
+    "@astrojs/svelte": "workspace:*",
     "astro": "workspace:*",
     "react": "^18.2.0",
-    "react-dom": "^18.2.0"
+    "react-dom": "^18.2.0",
+    "solid-js": "^1.7.4",
+    "svelte": "^3.58.0",
+    "vue": "^3.2.47",
+    "preact": "^10.13.2"
   }
 }
diff --git a/packages/astro/test/fixtures/astro-slots-nested/src/components/Inner.tsx b/packages/astro/test/fixtures/astro-slots-nested/src/components/Inner.tsx
@@ -1,3 +1,5 @@
+import React from 'react';
+
 export default function Inner() {
   return <span>Inner</span>;
 }
diff --git a/packages/astro/test/fixtures/astro-slots-nested/src/components/PassesChildren.tsx b/packages/astro/test/fixtures/astro-slots-nested/src/components/PassesChildren.tsx
@@ -0,0 +1,5 @@
+import React, { Fragment } from 'react';
+
+export default function PassesChildren({ children }) {
+	return <Fragment>{ children }</Fragment>;
+}
diff --git a/packages/astro/test/fixtures/astro-slots-nested/src/components/PassesChildrenP.tsx b/packages/astro/test/fixtures/astro-slots-nested/src/components/PassesChildrenP.tsx
@@ -0,0 +1,5 @@
+import { h, Fragment } from 'preact';
+
+export default function PassesChildren({ children }) {
+	return <Fragment>{ children }</Fragment>;
+}
diff --git a/packages/astro/test/fixtures/astro-slots-nested/src/components/PassesChildrenS.tsx b/packages/astro/test/fixtures/astro-slots-nested/src/components/PassesChildrenS.tsx
@@ -0,0 +1,5 @@
+/** @jsxImportSource solid-js */
+
+export default function PassesChildren({ children }) {
+	return <>{ children }</>;
+}
diff --git a/packages/astro/test/fixtures/astro-slots-nested/src/components/PassesChildrenSv.svelte b/packages/astro/test/fixtures/astro-slots-nested/src/components/PassesChildrenSv.svelte
@@ -0,0 +1,3 @@
+<div class="svelte-children">
+	<slot></slot>
+</div>
diff --git a/packages/astro/test/fixtures/astro-slots-nested/src/components/PassesChildrenV.vue b/packages/astro/test/fixtures/astro-slots-nested/src/components/PassesChildrenV.vue
@@ -0,0 +1,5 @@
+<template>
+	<div class="vue-wrapper">
+    <slot></slot>
+  </div>
+</template>
diff --git a/packages/astro/test/fixtures/astro-slots-nested/src/pages/server-component-nested.astro b/packages/astro/test/fixtures/astro-slots-nested/src/pages/server-component-nested.astro
@@ -0,0 +1,62 @@
+---
+import PassesChildren from '../components/PassesChildren.jsx';
+import PassesChildrenP from '../components/PassesChildrenP.jsx';
+import PassesChildrenS from '../components/PassesChildrenS.jsx';
+import PassesChildrenSv from '../components/PassesChildrenSv.svelte';
+import PassesChildrenV from '../components/PassesChildrenV.vue';
+---
+
+<html lang="en">
+	<head>
+		<title>Testing</title>
+	</head>
+	<body>
+		<main>
+			<div id="react">
+				<PassesChildren>
+					<div>
+						<PassesChildren client:load>
+							<span>Inner children</span>
+						</PassesChildren>
+					</div>
+				</PassesChildren>
+			</div>
+			<div id="preact">
+				<PassesChildrenP>
+					<div>
+						<PassesChildrenP client:load>
+							<span>Inner children</span>
+						</PassesChildrenP>
+					</div>
+				</PassesChildrenP>
+			</div>
+			<div id="solid">
+				<PassesChildrenS>
+					<div>
+						<PassesChildrenS client:load>
+							<span>Inner children</span>
+						</PassesChildrenS>
+					</div>
+				</PassesChildrenS>
+			</div>
+			<div id="svelte">
+				<PassesChildrenSv>
+					<div>
+						<PassesChildrenSv client:load>
+							<span>Inner children</span>
+						</PassesChildrenSv>
+					</div>
+				</PassesChildrenSv>
+			</div>
+			<div id="vue">
+				<PassesChildrenV>
+					<div>
+						<PassesChildrenV client:load>
+							<span>Inner children</span>
+						</PassesChildrenV>
+					</div>
+				</PassesChildrenV>
+			</div>
+		</main>
+	</body>
+</html>
diff --git a/packages/integrations/preact/src/server.ts b/packages/integrations/preact/src/server.ts
@@ -1,3 +1,4 @@
+import type { AstroComponentMetadata } from 'astro';
 import { Component as BaseComponent, h } from 'preact';
 import render from 'preact-render-to-string';
 import { getContext } from './context.js';
@@ -10,7 +11,7 @@ const slotName = (str: string) => str.trim().replace(/[-_]([a-z])/g, (_, w) => w
 let originalConsoleError: typeof console.error;
 let consoleFilterRefs = 0;
 
-function check(this: RendererContext, Component: any, props: Record<string, any>, children: any) {
+function check(this: RendererContext, Component: any, props: Record<string, any>, children: any, ) {
 	if (typeof Component !== 'function') return false;
 
 	if (Component.prototype != null && typeof Component.prototype.render === 'function') {
@@ -21,7 +22,7 @@ function check(this: RendererContext, Component: any, props: Record<string, any>
 
 	try {
 		try {
-			const { html } = renderToStaticMarkup.call(this, Component, props, children);
+			const { html } = renderToStaticMarkup.call(this, Component, props, children, undefined);
 			if (typeof html !== 'string') {
 				return false;
 			}
@@ -38,18 +39,28 @@ function check(this: RendererContext, Component: any, props: Record<string, any>
 	}
 }
 
+function shouldHydrate(metadata: AstroComponentMetadata | undefined) {
+	// Adjust how this is hydrated only when the version of Astro supports `astroStaticSlot`
+	return metadata?.astroStaticSlot ? !!metadata.hydrate : true;
+}
+
 function renderToStaticMarkup(
 	this: RendererContext,
 	Component: any,
 	props: Record<string, any>,
-	{ default: children, ...slotted }: Record<string, any>
+	{ default: children, ...slotted }: Record<string, any>,
+	metadata: AstroComponentMetadata | undefined,
 ) {
 	const ctx = getContext(this.result);
 
 	const slots: Record<string, ReturnType<typeof h>> = {};
 	for (const [key, value] of Object.entries(slotted)) {
 		const name = slotName(key);
-		slots[name] = h(StaticHtml, { value, name });
+		slots[name] = h(StaticHtml, {
+			hydrate: shouldHydrate(metadata),
+			value,
+			name
+		});
 	}
 
 	// Restore signals back onto props so that they will be passed as-is to components
@@ -61,7 +72,9 @@ function renderToStaticMarkup(
 	serializeSignals(ctx, props, attrs, propsMap);
 
 	const html = render(
-		h(Component, newProps, children != null ? h(StaticHtml, { value: children }) : children)
+		h(Component, newProps, children != null ? h(StaticHtml, {
+			hydrate: shouldHydrate(metadata),
+			value: children}) : children)
 	);
 	return {
 		attrs,
@@ -127,4 +140,5 @@ function filteredConsoleError(msg: string, ...rest: any[]) {
 export default {
 	check,
 	renderToStaticMarkup,
+	supportsAstroStaticSlot: true,
 };
diff --git a/packages/integrations/preact/src/static-html.ts b/packages/integrations/preact/src/static-html.ts
@@ -1,15 +1,22 @@
 import { h } from 'preact';
 
+type Props = {
+	value: string;
+	name?: string;
+	hydrate?: boolean;
+}
+
 /**
  * Astro passes `children` as a string of HTML, so we need
  * a wrapper `div` to render that content as VNodes.
  *
  * As a bonus, we can signal to Preact that this subtree is
  * entirely static and will never change via `shouldComponentUpdate`.
  */
-const StaticHtml = ({ value, name }: { value: string; name?: string }) => {
+const StaticHtml = ({ value, name, hydrate }: Props) => {
 	if (!value) return null;
-	return h('astro-slot', { name, dangerouslySetInnerHTML: { __html: value } });
+	const tagName = hydrate === false ? 'astro-static-slot' : 'astro-slot';
+	return h(tagName, { name, dangerouslySetInnerHTML: { __html: value } });
 };
 
 /**
diff --git a/packages/integrations/react/server-v17.js b/packages/integrations/react/server-v17.js
@@ -65,7 +65,11 @@ function renderToStaticMarkup(Component, props, { default: children, ...slotted
 	};
 	const newChildren = children ?? props.children;
 	if (newChildren != null) {
-		newProps.children = React.createElement(StaticHtml, { value: newChildren });
+		newProps.children = React.createElement(StaticHtml, {
+			// Adjust how this is hydrated only when the version of Astro supports `astroStaticSlot`
+			hydrate: metadata.astroStaticSlot ? !!metadata.hydrate : true,
+			value: newChildren
+		});
 	}
 	const vnode = React.createElement(Component, newProps);
 	let html;
@@ -80,4 +84,5 @@ function renderToStaticMarkup(Component, props, { default: children, ...slotted
 export default {
 	check,
 	renderToStaticMarkup,
+	supportsAstroStaticSlot: true,
 };
diff --git a/packages/integrations/react/server.js b/packages/integrations/react/server.js
diff --git a/packages/integrations/react/static-html.js b/packages/integrations/react/static-html.js
diff --git a/packages/integrations/solid/src/server.ts b/packages/integrations/solid/src/server.ts
diff --git a/packages/integrations/svelte/server.js b/packages/integrations/svelte/server.js
diff --git a/packages/integrations/vue/server.js b/packages/integrations/vue/server.js
diff --git a/packages/integrations/vue/static-html.js b/packages/integrations/vue/static-html.js
diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml

Original file line number	Diff line number	Diff line change
`@@ -94,6 +94,7 @@ export interface AstroComponentMetadata {`
`94`	`94`	`hydrateArgs?: any;`
`95`	`95`	`componentUrl?: string;`
`96`	`96`	`componentExport?: { value: string; namespace?: boolean };`
	`97`	`+ astroStaticSlot: true;`
`97`	`98`	`}`
`98`	`99`
`99`	`100`	`/** The flags supported by the Astro CLI */`
`@@ -1718,6 +1719,7 @@ export interface SSRLoadedRenderer extends AstroRenderer {`
`1718`	`1719`	`html: string;`
`1719`	`1720`	`attrs?: Record<string, string>;`
`1720`	`1721`	`}>;`
	`1722`	`+ supportsAstroStaticSlot?: boolean;`
`1721`	`1723`	`};`
`1722`	`1724`	`}`
`1723`	`1725`
Original file line number	Diff line number	Diff line change
`@@ -202,7 +202,7 @@ export default function jsx({ settings, logging }: AstroPluginJSXOptions): Plugi`
`202`	`202`	`Unable to resolve a renderer that handles this file! With more than one renderer enabled, you should include an import or use a pragma comment.`
`203`	`203`	`Add ${colors.cyan(`
`204`	`204`	IMPORT_STATEMENTS[defaultRendererName] \|\| `import '${defaultRendererName}';`
`205`		- )} or ${colors.cyan(`/* jsxImportSource: ${defaultRendererName} */`)} to this file.
	`205`	+ )} or ${colors.cyan(`/** @jsxImportSource: ${defaultRendererName} */`)} to this file.
`206`	`206`	`
`207`	`207`	`);`
`208`	`208`	`return null;`
Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,5 @@`
	`1`	`+import React from 'react';`
	`2`	`+`
`1`	`3`	`export default function Inner() {`
`2`	`4`	`return <span>Inner</span>;`
`3`	`5`	`}`