Fix solid recursion bug (#4215)

* Fix solid recursion bug * Fix types * Remove debug code * Remove logging from e2e test
withastro · Aug 10, 2022 · 0af5aa7 · 0af5aa7
1 parent 58941e9
commit 0af5aa7
Show file tree

Hide file tree

Showing 17 changed files with 227 additions and 43 deletions.
diff --git a/packages/astro/e2e/fixtures/solid-recurse/astro.config.mjs b/packages/astro/e2e/fixtures/solid-recurse/astro.config.mjs
@@ -0,0 +1,7 @@
+import { defineConfig } from 'astro/config';
+import solid from '@astrojs/solid-js';
+
+// https://astro.build/config
+export default defineConfig({
+	integrations: [solid()],
+});
diff --git a/packages/astro/e2e/fixtures/solid-recurse/package.json b/packages/astro/e2e/fixtures/solid-recurse/package.json
@@ -0,0 +1,12 @@
+{
+  "name": "@e2e/solid-recurse",
+  "version": "0.0.0",
+  "private": true,
+  "dependencies": {
+    "@astrojs/solid-js": "workspace:*",
+    "astro": "workspace:*"
+  },
+  "devDependencies": {
+    "solid-js": "^1.4.3"
+  }
+}
diff --git a/packages/astro/e2e/fixtures/solid-recurse/src/components/Counter.jsx b/packages/astro/e2e/fixtures/solid-recurse/src/components/Counter.jsx
@@ -0,0 +1,24 @@
+import { createSignal } from 'solid-js';
+
+export default function Counter(props) {
+  const [count, setCount] = createSignal(0);
+  const type = props.type;
+
+  return (
+    <button
+			id={props.id + '-' + type}
+      data-type={type}
+      ref={(el) =>
+        console.log(
+          ` ${type} ${type == el.dataset.type ? '==' : '!='} ${el.dataset.type}`
+        )
+      }
+      onClick={() => {
+        console.log('click');
+        setCount((p) => ++p);
+      }}
+    >
+      {type}: {count()}
+    </button>
+  );
+}
diff --git a/packages/astro/e2e/fixtures/solid-recurse/src/components/WrapperA.jsx b/packages/astro/e2e/fixtures/solid-recurse/src/components/WrapperA.jsx
@@ -0,0 +1,10 @@
+import Counter from './Counter';
+
+export default function WrapperA(props) {
+  return (
+    // removing the div avoids the error
+    <div data-wrapper-a-root>
+      <Counter id={props.id} type="A"></Counter>
+    </div>
+  );
+}
diff --git a/packages/astro/e2e/fixtures/solid-recurse/src/components/WrapperB.jsx b/packages/astro/e2e/fixtures/solid-recurse/src/components/WrapperB.jsx
@@ -0,0 +1,11 @@
+import Counter from './Counter';
+
+export default function WrapperB(props) {
+  return (
+    <div id={props.id}>
+      {/* Reversing the order of these avoids the error: */}
+      <div data-wrapper-children>{props.children}</div>
+      <Counter id={props.id} type="B"></Counter>
+    </div>
+  );
+}
diff --git a/packages/astro/e2e/fixtures/solid-recurse/src/pages/index.astro b/packages/astro/e2e/fixtures/solid-recurse/src/pages/index.astro
@@ -0,0 +1,20 @@
+---
+import WrapperB from "../components/WrapperB.jsx";
+import WrapperA from "../components/WrapperA.jsx";
+---
+
+<html>
+	<head>
+		<meta charset="utf-8" />
+		<meta name="viewport" content="width=device-width" />
+	</head>
+	<body>
+		<main>
+			Case 1
+			<WrapperB id="case1" client:load>
+				<WrapperA id="case1" />
+			</WrapperB>
+			<br/>
+		</main>
+	</body>
+</html>
diff --git a/packages/astro/e2e/solid-recurse.test.js b/packages/astro/e2e/solid-recurse.test.js
@@ -0,0 +1,29 @@
+import { expect } from '@playwright/test';
+import { testFactory } from './test-utils.js';
+
+const test = testFactory({ root: './fixtures/solid-recurse/' });
+
+let devServer;
+
+test.beforeEach(async ({ astro }) => {
+	devServer = await astro.startDevServer();
+});
+
+test.afterEach(async () => {
+	await devServer.stop();
+});
+
+test.describe('Recursive elements with Solid', () => {
+	test('Counter', async ({ astro, page }) => {
+		await page.goto(astro.resolveUrl('/'));
+
+		const wrapper = page.locator('#case1');
+		await expect(wrapper, 'component is visible').toBeVisible();
+
+		const increment = page.locator('#case1-B');
+		await expect(increment, 'initial count is 0').toHaveText('B: 0');
+
+		await increment.click();
+		await expect(increment, 'count is incremented').toHaveText('B: 1');
+	});
+});
diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts
@@ -1024,6 +1024,7 @@ export interface SSRLoadedRenderer extends AstroRenderer {
 		check: AsyncRendererComponentFn<boolean>;
 		renderToStaticMarkup: AsyncRendererComponentFn<{
 			html: string;
+			attrs?: Record<string, string>;
 		}>;
 	};
 }

diff --git a/packages/astro/src/runtime/server/hydration.ts b/packages/astro/src/runtime/server/hydration.ts
@@ -107,14 +107,15 @@ interface HydrateScriptOptions {
 	result: SSRResult;
 	astroId: string;
 	props: Record<string | number, any>;
+	attrs: Record<string, string> | undefined;
 }
 
 /** For hydrated components, generate a <script type="module"> to load the component */
 export async function generateHydrateScript(
 	scriptOptions: HydrateScriptOptions,
 	metadata: Required<AstroComponentMetadata>
 ): Promise<SSRElement> {
-	const { renderer, result, astroId, props } = scriptOptions;
+	const { renderer, result, astroId, props, attrs } = scriptOptions;
 	const { hydrate, componentUrl, componentExport } = metadata;
 
 	if (!componentExport) {
@@ -131,6 +132,13 @@ export async function generateHydrateScript(
 		},
 	};
 
+	// Attach renderer-provided attributes
+	if(attrs) {
+		for(const [key, value] of Object.entries(attrs)) {
+			island.props[key] = value;
+		}
+	}
+
 	// Add component url
 	island.props['component-url'] = await result.resolve(componentUrl);
 

diff --git a/packages/astro/src/runtime/server/render/component.ts b/packages/astro/src/runtime/server/render/component.ts
@@ -102,6 +102,7 @@ export async function renderComponent(
 
 	const { hydration, isPage, props } = extractDirectives(_props);
 	let html = '';
+	let attrs: Record<string, string> | undefined = undefined;
 
 	if (hydration) {
 		metadata.hydrate = hydration.directive as AstroComponentMetadata['hydrate'];
@@ -222,7 +223,7 @@ Did you mean to enable ${formatList(probableRendererNames.map((r) => '`' + r + '
 				// We already know that renderer.ssr.check() has failed
 				// but this will throw a much more descriptive error!
 				renderer = matchingRenderers[0];
-				({ html } = await renderer.ssr.renderToStaticMarkup.call(
+				({ html, attrs } = await renderer.ssr.renderToStaticMarkup.call(
 					{ result },
 					Component,
 					props,
@@ -247,7 +248,7 @@ If you're still stuck, please open an issue on GitHub or join us at https://astr
 		if (metadata.hydrate === 'only') {
 			html = await renderSlot(result, slots?.fallback);
 		} else {
-			({ html } = await renderer.ssr.renderToStaticMarkup.call(
+			({ html, attrs } = await renderer.ssr.renderToStaticMarkup.call(
 				{ result },
 				Component,
 				props,
@@ -302,7 +303,7 @@ If you're still stuck, please open an issue on GitHub or join us at https://astr
 	);
 
 	const island = await generateHydrateScript(
-		{ renderer: renderer!, result, astroId, props },
+		{ renderer: renderer!, result, astroId, props, attrs },
 		metadata as Required<AstroComponentMetadata>
 	);
 

diff --git a/packages/integrations/solid/package.json b/packages/integrations/solid/package.json
@@ -22,8 +22,8 @@
   "exports": {
     ".": "./dist/index.js",
     "./*": "./*",
-    "./client.js": "./client.js",
-    "./server.js": "./server.js",
+    "./client.js": "./dist/client.js",
+    "./server.js": "./dist/server.js",
     "./package.json": "./package.json"
   },
   "scripts": {

diff --git a/packages/integrations/solid/server.js b/packages/integrations/solid/server.js
diff --git a/packages/integrations/solid/client.js → packages/integrations/solid/src/client.ts b/packages/integrations/solid/client.js → packages/integrations/solid/src/client.ts
@@ -1,17 +1,17 @@
 import { sharedConfig } from 'solid-js';
 import { hydrate, render, createComponent } from 'solid-js/web';
 
-export default (element) =>
-	(Component, props, slotted, { client }) => {
+export default (element: HTMLElement) =>
+	(Component: any, props: any, slotted: any, { client }: { client: string }) => {
 		// Prepare global object expected by Solid's hydration logic
-		if (!window._$HY) {
-			window._$HY = { events: [], completed: new WeakSet(), r: {} };
+		if (!(window as any)._$HY) {
+			(window as any)._$HY = { events: [], completed: new WeakSet(), r: {} };
 		}
 		if (!element.hasAttribute('ssr')) return;
 
 		const fn = client === 'only' ? render : hydrate;
 
-		let _slots = {};
+		let _slots: Record<string, any> = {};
 		if (Object.keys(slotted).length > 0) {
 			// hydrating
 			if (sharedConfig.context) {
@@ -28,6 +28,7 @@ export default (element) =>
 		}
 
 		const { default: children, ...slots } = _slots;
+		const renderId = element.dataset.solidRenderId;
 
 		fn(
 			() =>
@@ -36,6 +37,9 @@ export default (element) =>
 					...slots,
 					children,
 				}),
-			element
+			element,
+			{
+				renderId
+			}
 		);
 	};
diff --git a/packages/integrations/solid/src/context.ts b/packages/integrations/solid/src/context.ts
@@ -0,0 +1,28 @@
+import type { RendererContext } from './types';
+
+type Context = {
+	id: string;
+	c: number;
+}
+
+const contexts = new WeakMap<RendererContext['result'], Context>();
+
+export function getContext(result: RendererContext['result']): Context {
+	if(contexts.has(result)) {
+		return contexts.get(result)!;
+	}
+	let ctx = {
+		c: 0,
+		get id() {
+			return 's' + this.c.toString();
+		}
+	};
+	contexts.set(result, ctx);
+	return ctx;
+}
+
+export function incrementId(ctx: Context): string {
+	let id = ctx.id;
+	ctx.c++;
+	return id;
+}
diff --git a/packages/integrations/solid/src/server.ts b/packages/integrations/solid/src/server.ts
@@ -0,0 +1,45 @@
+import type { RendererContext } from './types';
+import { renderToString, ssr, createComponent } from 'solid-js/web';
+import { getContext, incrementId } from './context.js';
+
+const slotName = (str: string) => str.trim().replace(/[-_]([a-z])/g, (_, w) => w.toUpperCase());
+
+function check(this: RendererContext, Component: any, props: Record<string, any>, children: any) {
+	if (typeof Component !== 'function') return false;
+	const { html } = renderToStaticMarkup.call(this, Component, props, children);
+	return typeof html === 'string';
+}
+
+function renderToStaticMarkup(this: RendererContext, Component: any, props: Record<string, any>, { default: children, ...slotted }: any, metadata?: undefined | Record<string, any>) {
+	const renderId = metadata?.hydrate ? incrementId(getContext(this.result)) : '';
+
+	const html = renderToString(() => {
+		const slots: Record<string, any> = {};
+		for (const [key, value] of Object.entries(slotted)) {
+			const name = slotName(key);
+			slots[name] = ssr(`<astro-slot name="${name}">${value}</astro-slot>`);
+		}
+		// Note: create newProps to avoid mutating `props` before they are serialized
+		const newProps = {
+			...props,
+			...slots,
+			// In Solid SSR mode, `ssr` creates the expected structure for `children`.
+			children: children != null ? ssr(`<astro-slot>${children}</astro-slot>`) : children,
+		};
+
+		return createComponent(Component, newProps);
+	}, {
+		renderId
+	});
+	return {
+		attrs: {
+			'data-solid-render-id': renderId
+		},
+		html
+	};
+}
+
+export default {
+	check,
+	renderToStaticMarkup,
+};
diff --git a/packages/integrations/solid/src/types.ts b/packages/integrations/solid/src/types.ts
@@ -0,0 +1,4 @@
+import type { SSRResult } from 'astro';
+export type RendererContext = {
+	result: SSRResult;
+};
diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml