fix: rendering islands inside other islands (#1295)

Author: marvinhagemeister

src/server/render.ts

@@ -1,5 +1,14 @@
 import { renderToString } from "preact-render-to-string";
-import { ComponentChildren, ComponentType, Fragment, h, options } from "preact";
+import {
+  Component,
+  ComponentChildren,
+  ComponentType,
+  Fragment,
+  h,
+  Options as PreactOptions,
+  options as preactOptions,
+  VNode,
+} from "preact";
 import {
   AppModule,
   ErrorPage,
@@ -20,6 +29,16 @@ import { assetHashingHook } from "../runtime/utils.ts";
 import { htmlEscapeJsonString } from "./htmlescape.ts";
 import { serialize } from "./serializer.ts";
 
+// These hooks are long stable, but when we originally added them we
+// weren't sure if they should be public.
+export interface AdvancedPreactOptions extends PreactOptions {
+  /** Attach a hook that is invoked after a tree was mounted or was updated. */
+  __c?(vnode: VNode, commitQueue: Component[]): void;
+  /** Attach a hook that is invoked before a vnode has rendered. */
+  __r?(vnode: VNode): void;
+}
+const options = preactOptions as AdvancedPreactOptions;
+
 export interface RenderOptions<Data> {
   route: Route<Data> | UnknownPage | ErrorPage;
   islands: Island[];
@@ -450,18 +469,51 @@ function wrapWithMarker(vnode: ComponentChildren, markerText: string) {
 const ISLANDS: Island[] = [];
 const ENCOUNTERED_ISLANDS: Set<Island> = new Set([]);
 let ISLAND_PROPS: unknown[] = [];
+
+// Keep track of which component rendered which vnode. This allows us
+// to detect when an island is rendered within another instead of being
+// passed as children.
+let ownerStack: VNode[] = [];
+const islandOwners = new Map<VNode, VNode>();
+
 const originalHook = options.vnode;
 let ignoreNext = false;
 options.vnode = (vnode) => {
   assetHashingHook(vnode);
   const originalType = vnode.type as ComponentType<unknown>;
+
+  // Use a labelled statement that allows ous to break out of it
+  // whilst still continuing execution. We still want to call previous
+  // `options.vnode` hooks if there were any, otherwise we'd break
+  // the change for other plugins hooking into Preact.
+  patchIslands:
   if (typeof vnode.type === "function") {
     const island = ISLANDS.find((island) => island.component === originalType);
     if (island) {
+      const hasOwners = ownerStack.length > 0;
+      if (hasOwners) {
+        const prevOwner = ownerStack[ownerStack.length - 1];
+        islandOwners.set(vnode, prevOwner);
+      }
+
+      // Check if we already patched this component
       if (ignoreNext) {
         ignoreNext = false;
-        return;
+        break patchIslands;
+      }
+
+      // Check if an island is rendered inside another island, not just
+      // passed as a child. Example:
+      //   function Island() {}
+      //     return <OtherIsland />
+      //   }
+      if (hasOwners) {
+        const prevOwner = ownerStack[ownerStack.length - 1];
+        if (islandOwners.has(prevOwner)) {
+          break patchIslands;
+        }
       }
+
       ENCOUNTERED_ISLANDS.add(island);
       vnode.type = (props) => {
         ignoreNext = true;
@@ -488,3 +540,30 @@ options.vnode = (vnode) => {
   }
   if (originalHook) originalHook(vnode);
 };
+
+// Keep track of owners
+const oldDiffed = options.diffed;
+const oldRender = options.__r;
+const oldCommit = options.__c;
+options.__r = (vnode) => {
+  if (
+    typeof vnode.type === "function" &&
+    vnode.type !== Fragment
+  ) {
+    ownerStack.push(vnode);
+  }
+  oldRender?.(vnode);
+};
+options.diffed = (vnode) => {
+  if (typeof vnode.type === "function") {
+    if (vnode.type !== Fragment) {
+      ownerStack.pop();
+    }
+  }
+  oldDiffed?.(vnode);
+};
+options.__c = (vnode, queue) => {
+  oldCommit?.(vnode, queue);
+  ownerStack = [];
+  islandOwners.clear();
+};

tests/fixture_island_nesting/fresh.gen.ts

@@ -4,29 +4,33 @@
 
 import * as $0 from "./routes/index.tsx";
 import * as $1 from "./routes/island_in_island.tsx";
-import * as $2 from "./routes/island_jsx_child.tsx";
-import * as $3 from "./routes/island_jsx_children.tsx";
-import * as $4 from "./routes/island_jsx_island_jsx.tsx";
-import * as $5 from "./routes/island_jsx_text.tsx";
-import * as $6 from "./routes/island_nested_props.tsx";
-import * as $7 from "./routes/island_siblings.tsx";
+import * as $2 from "./routes/island_in_island_definition.tsx";
+import * as $3 from "./routes/island_jsx_child.tsx";
+import * as $4 from "./routes/island_jsx_children.tsx";
+import * as $5 from "./routes/island_jsx_island_jsx.tsx";
+import * as $6 from "./routes/island_jsx_text.tsx";
+import * as $7 from "./routes/island_nested_props.tsx";
+import * as $8 from "./routes/island_siblings.tsx";
 import * as $$0 from "./islands/Island.tsx";
-import * as $$1 from "./islands/IslandWithProps.tsx";
+import * as $$1 from "./islands/IslandInsideIsland.tsx";
+import * as $$2 from "./islands/IslandWithProps.tsx";
 
 const manifest = {
   routes: {
     "./routes/index.tsx": $0,
     "./routes/island_in_island.tsx": $1,
-    "./routes/island_jsx_child.tsx": $2,
-    "./routes/island_jsx_children.tsx": $3,
-    "./routes/island_jsx_island_jsx.tsx": $4,
-    "./routes/island_jsx_text.tsx": $5,
-    "./routes/island_nested_props.tsx": $6,
-    "./routes/island_siblings.tsx": $7,
+    "./routes/island_in_island_definition.tsx": $2,
+    "./routes/island_jsx_child.tsx": $3,
+    "./routes/island_jsx_children.tsx": $4,
+    "./routes/island_jsx_island_jsx.tsx": $5,
+    "./routes/island_jsx_text.tsx": $6,
+    "./routes/island_nested_props.tsx": $7,
+    "./routes/island_siblings.tsx": $8,
   },
   islands: {
     "./islands/Island.tsx": $$0,
-    "./islands/IslandWithProps.tsx": $$1,
+    "./islands/IslandInsideIsland.tsx": $$1,
+    "./islands/IslandWithProps.tsx": $$2,
   },
   baseUrl: import.meta.url,
 };

tests/fixture_island_nesting/islands/IslandInsideIsland.tsx

@@ -0,0 +1,14 @@
+import { ComponentChildren } from "preact";
+import Island from "./Island.tsx";
+
+export default function IslandInsideIsland(
+  props: { children?: ComponentChildren },
+) {
+  return (
+    <div class="island">
+      <Island>
+        {props.children}
+      </Island>
+    </div>
+  );
+}

tests/fixture_island_nesting/routes/island_in_island_definition.tsx

@@ -0,0 +1,11 @@
+import IslandInsideIsland from "../islands/IslandInsideIsland.tsx";
+
+export default function Home() {
+  return (
+    <div id="page">
+      <IslandInsideIsland>
+        <p>it works</p>
+      </IslandInsideIsland>
+    </div>
+  );
+}

tests/islands_test.ts

@@ -344,6 +344,37 @@ Deno.test({
   sanitizeResources: false,
 });
 
+Deno.test({
+  name: "render island inside island definition",
+
+  async fn(_t) {
+    await withPageName(
+      "./tests/fixture_island_nesting/main.ts",
+      async (page, address) => {
+        await page.goto(`${address}/island_in_island_definition`, {
+          waitUntil: "networkidle2",
+        });
+        await page.waitForSelector(".island");
+
+        await delay(100);
+        const text = await page.$eval(
+          ".island .island p",
+          (el) => el.textContent,
+        );
+        assertEquals(text, "it works");
+
+        // Check that there is no duplicated content which could happen
+        // when islands aren't initialized correctly
+        const pageText = await page.$eval("#page", (el) => el.textContent);
+        assertEquals(pageText, "it works");
+      },
+    );
+  },
+
+  sanitizeOps: false,
+  sanitizeResources: false,
+});
+
 Deno.test({
   name:
     "render island with JSX children that render another island with JSX children",