fix: bug bug where fallback resolved after children in Suspense compo…

…nents

.changeset/large-cheetahs-matter.md

@@ -0,0 +1,5 @@
+---
+'@kitajs/html': patch
+---
+
+Fixed bug where fallback resolved after children in Suspense components

packages/html/suspense.d.ts

@@ -57,8 +57,8 @@ export type RequestData = {
  * knows which request it belongs to.
  *
  * **Warning**: Using `Suspense` without any type of runtime support will _**LEAK
- * memory**_. Always use `renderToStream`, `renderToString` or within a specific package
- * like `@kitajs/fastify-html-plugin`
+ * memory**_ and not work. Always use with `renderToStream` or within a framework that
+ * supports it.
  */
 export function Suspense(props: SuspenseProps): JSX.Element;
 
@@ -69,13 +69,19 @@ export function Suspense(props: SuspenseProps): JSX.Element;
  * @example
  *
  * ```tsx
+ * import { text} from 'node:stream/consumers';
+ *
  * // Prints out the rendered stream (2nd example shows with a custom id)
  * const stream = renderToStream(r => <AppWithSuspense rid={r} />)
  * const stream = renderToStream(<AppWithSuspense rid={myCustomId} />, myCustomId)
  *
+ * // You can consume it as a stream
  * for await (const html of stream) {
  *  console.log(html.toString())
  * }
+ *
+ * // Or join it all together (Wastes ALL Suspense benefits, but useful for testing)
+ * console.log(await text(stream))
  * ```
  *
  * @param html The component tree to render or a function that returns the component tree.
@@ -89,44 +95,40 @@ export function renderToStream(
 ): Readable;
 
 /**
- * Returns a promise that resolves to the entire HTML generated by the component tree.
- * **Suspense calls are waited for**.
- *
- * This method is a shorthand to call {@linkcode renderToStream} and collect its result
- * into a string.
+ * Prepends the fallback into a html stream handled manually without
+ * {@linkcode renderToStream}.
  *
- * **Rendering to string will not give any benefits over streaming, it will only be
- * slower.**
+ * **This API is meant to be used by library authors and should not be used directly.**
  *
  * @example
  *
  * ```tsx
- * // Does not uses suspense benefits! Useful for testing. Prefer to
- * // use renderToStream instead. (2nd example shows with a custom id)
- * const html = await renderToString(r => <AppWithSuspense rid={r} />)
- * const html = await renderToString(<AppWithSuspense rid={myCustomId} />, myCustomId)
+ * const html = <RootLayout rid={rid} />
+ * const requestData = SUSPENSE_ROOT.requests.get(rid);
+ *
+ * if(!requestData) {
+ *   return html;
+ * }
  *
- * console.log(html);
+ * // This prepends the html into the stream, handling possible
+ * // cases where the html resolved after one of its async children
+ * return writeFallback(html, requestData.stream);
  * ```
  *
- * @param html The component tree to render or a function that returns the component tree.
- * @param rid The request id to identify the request, if not provided, a new incrementing
- *   id will be used.
- * @returns A promise that resolves to the entire HTML generated by the component tree.
+ * @param fallback The fallback to render while the async children are loading.
+ * @param stream The stream to write the fallback into.
+ * @returns The same stream or another one with the fallback prepended.
  * @see {@linkcode renderToStream}
  */
-export function renderToString(
-  html: JSX.Element | ((rid: number | string) => JSX.Element),
-  rid?: number | string
-): Promise<string>;
+export function writeFallback(fallback: JSX.Element, stream: Readable): Readable;
 
 /**
  * This script needs to be loaded at the top of the page. You do not need to load it
  * manually, unless GLOBAL_SUSPENSE.autoScript is set to false.
  *
  * @see {@linkcode Suspense}
  */
-export const SuspenseScript: string;
+export declare const SuspenseScript: string;
 
 /**
  * The props for the `Suspense` component.

packages/html/suspense.js

@@ -113,7 +113,7 @@ function Suspense(props) {
   // were used and 1 as the first suspense component
   const run = ++data.running;
 
-  children
+  void children
     .then(writeStreamTemplate)
     .catch(function errorRecover(error) {
       // No catch block was specified, so we can
@@ -140,27 +140,22 @@ function Suspense(props) {
       return html.then(writeStreamTemplate);
     })
     .catch(function writeFatalError(error) {
-      // stream.emit returns true if there's a listener
-      // Nothing else to do if no catch or listener was found
-      /* c8 ignore next 3 */
-      if (data?.stream.emit('error', error) === false) {
-        console.error(error);
-      }
+      data.stream.emit('error', error);
     })
     .finally(function clearRequestData() {
       // reduces current suspense id
       if (data && data.running > 1) {
         data.running -= 1;
+        return;
+      }
 
-        // Last suspense component, runs cleanup
-      } else {
-        if (data && !data.stream.closed) {
-          data.stream.push(null);
-        }
-
-        // Removes the current state
-        SUSPENSE_ROOT.requests.delete(props.rid);
+      // Last suspense component, runs cleanup
+      if (data && !data.stream.closed) {
+        data.stream.push(null);
       }
+
+      // Removes the current state
+      SUSPENSE_ROOT.requests.delete(props.rid);
     });
 
   // Always will be a single children because multiple
@@ -214,8 +209,17 @@ function renderToStream(html, rid) {
   if (!rid) {
     rid = SUSPENSE_ROOT.requestCounter++;
   } else if (SUSPENSE_ROOT.requests.has(rid)) {
-    // Ensures the request id is unique
-    throw new Error(`The provided Request Id is already in use: ${rid}.`);
+    // Ensures the request id is unique within the current request
+    // error here to keep original stack trace
+    const error = new Error(`The provided Request Id is already in use: ${rid}.`);
+
+    // returns errored stream to avoid throws
+    return new Readable({
+      read() {
+        this.emit('error', error);
+        this.push(null);
+      }
+    });
   }
 
   if (typeof html === 'function') {
@@ -224,7 +228,14 @@ function renderToStream(html, rid) {
     } catch (error) {
       // Avoids memory leaks by removing the request data
       SUSPENSE_ROOT.requests.delete(rid);
-      throw error;
+
+      // returns errored stream to avoid throws
+      return new Readable({
+        read() {
+          this.emit('error', error);
+          this.push(null);
+        }
+      });
     }
   }
 
@@ -237,57 +248,56 @@ function renderToStream(html, rid) {
       return Readable.from([html]);
     }
 
-    const readable = new Readable({ read: noop });
-
-    html.then(
-      (result) => {
-        readable.push(result);
-        readable.push(null); // self closes
-      },
-      (error) => {
-        // stream.emit returns true if there's a listener
-        // Nothing else to do if no catch or listener was found
-        /* c8 ignore next 3 */
-        if (readable.emit('error', error) === false) {
-          console.error(error);
-        }
+    return new Readable({
+      read() {
+        void html
+          .then((result) => {
+            this.push(result);
+            this.push(null);
+          })
+          .catch((error) => {
+            this.emit('error', error);
+          });
       }
-    );
+    });
+  }
 
+  return writeFallback(html, requestData.stream);
+}
+
+/** @type {import('./suspense').writeFallback} */
+function writeFallback(fallback, readable) {
+  // Impossible to sync templates have their
+  // streams being written before the fallback
+  if (typeof fallback === 'string') {
+    readable.push(fallback);
     return readable;
   }
 
-  if (typeof html === 'string') {
-    requestData.stream.push(html);
-  } else {
-    html.then(
-      (html) => requestData.stream.push(html),
-      (error) => {
-        /* c8 ignore next 6 */
-        // stream.emit returns true if there's a listener
-        // Nothing else to do if no catch or listener was found
-        if (requestData.stream.emit('error', error) === false) {
-          console.error(error);
-        }
-      }
-    );
-  }
+  // The fallback might resolve after a suspense resolves,
+  // so we need to ensure the fallback is written and sent
+  // before anything the requestData.stream might already have.
 
-  return requestData.stream;
-}
+  const copy = new Readable({ read: noop });
 
-/** @type {import('./suspense').renderToString} */
-async function renderToString(factory, rid) {
-  const chunks = [];
+  void fallback
+    .then(async (result) => {
+      copy.push(result);
 
-  for await (const chunk of renderToStream(factory, rid)) {
-    chunks.push(chunk);
-  }
+      for await (const chunk of readable) {
+        copy.push(chunk);
+      }
+
+      copy.push(null);
+    })
+    .catch((error) => {
+      copy.emit('error', error);
+    });
 
-  return chunks.join('');
+  return copy;
 }
 
 exports.Suspense = Suspense;
 exports.renderToStream = renderToStream;
-exports.renderToString = renderToString;
+exports.writeFallback = writeFallback;
 exports.SuspenseScript = SuspenseScript;