Minor client data implementation cleanups (#8236)

.changeset/pink-pumpkins-clean.md

@@ -0,0 +1,5 @@
+---
+"@remix-run/react": patch
+---
+
+[REMOVE] Minor client data updates

integration/client-data-test.ts

@@ -404,7 +404,61 @@ test.describe("Client Data", () => {
       expect(html).toMatch("Child Client Loader");
     });
 
-    test("clientLoader.hydrate is automatically implied when no server loader exists", async ({
+    test("HydrateFallback is not rendered if clientLoader.hydrate is not set (w/server loader)", async ({
+      page,
+    }) => {
+      let fixture = await createFixture({
+        files: {
+          ...getFiles({
+            parentClientLoader: false,
+            parentClientLoaderHydrate: false,
+            childClientLoader: false,
+            childClientLoaderHydrate: false,
+          }),
+          // Blow away parent.child.tsx with our own version
+          "app/routes/parent.child.tsx": js`
+            import * as React from 'react';
+            import { json } from '@remix-run/node';
+            import { useLoaderData } from '@remix-run/react';
+            export function loader() {
+              return json({
+                message: "Child Server Loader Data",
+              });
+            }
+            export async function clientLoader({ serverLoader }) {
+              await new Promise(r => setTimeout(r, 100));
+              return {
+                message: "Child Client Loader Data",
+              };
+            }
+            export function HydrateFallback() {
+              return <p>SHOULD NOT SEE ME</p>
+            }
+            export default function Component() {
+              let data = useLoaderData();
+              return <p id="child-data">{data.message}</p>;
+            }
+          `,
+        },
+      });
+      appFixture = await createAppFixture(fixture);
+
+      // Ensure initial document request contains the child fallback _and_ the
+      // subsequent streamed/resolved deferred data
+      let doc = await fixture.requestDocument("/parent/child");
+      let html = await doc.text();
+      expect(html).toMatch("Child Server Loader Data");
+      expect(html).not.toMatch("SHOULD NOT SEE ME");
+
+      let app = new PlaywrightFixture(appFixture, page);
+
+      await app.goto("/parent/child");
+      await page.waitForSelector("#child-data");
+      html = await app.getHtml("main");
+      expect(html).toMatch("Child Server Loader Data");
+    });
+
+    test("clientLoader.hydrate is automatically implied when no server loader exists (w HydrateFallback)", async ({
       page,
     }) => {
       appFixture = await createAppFixture(
@@ -447,6 +501,50 @@ test.describe("Client Data", () => {
       html = await app.getHtml("main");
       expect(html).toMatch("Loader Data (clientLoader only)");
     });
+
+    test("clientLoader.hydrate is automatically implied when no server loader exists (w/o HydrateFallback)", async ({
+      page,
+    }) => {
+      appFixture = await createAppFixture(
+        await createFixture({
+          files: {
+            ...getFiles({
+              parentClientLoader: false,
+              parentClientLoaderHydrate: false,
+              childClientLoader: false,
+              childClientLoaderHydrate: false,
+            }),
+            // Blow away parent.child.tsx with our own version without a server loader
+            "app/routes/parent.child.tsx": js`
+              import * as React from 'react';
+              import { useLoaderData } from '@remix-run/react';
+              // Even without setting hydrate=true, this should run on hydration
+              export async function clientLoader({ serverLoader }) {
+                await new Promise(r => setTimeout(r, 100));
+                return {
+                  message: "Loader Data (clientLoader only)",
+                };
+              }
+              export default function Component() {
+                let data = useLoaderData();
+                return <p id="child-data">{data.message}</p>;
+              }
+            `,
+          },
+        })
+      );
+      let app = new PlaywrightFixture(appFixture, page);
+
+      await app.goto("/parent/child");
+      let html = await app.getHtml();
+      expect(html).toMatch(
+        "💿 Hey developer 👋. You can provide a way better UX than this"
+      );
+      expect(html).not.toMatch("child-data");
+      await page.waitForSelector("#child-data");
+      html = await app.getHtml("main");
+      expect(html).toMatch("Loader Data (clientLoader only)");
+    });
   });
 
   test.describe("clientLoader - lazy route module", () => {

packages/remix-react/browser.tsx

@@ -17,6 +17,7 @@ import type { RouteModules } from "./routeModules";
 import {
   createClientRoutes,
   createClientRoutesWithHMRRevalidationOptOut,
+  shouldHydrateRouteLoader,
 } from "./routes";
 
 /* eslint-disable prefer-let/prefer-let */
@@ -53,7 +54,6 @@ declare global {
 }
 
 let router: Router;
-let didServerRenderFallback = false;
 let routerInitialized = false;
 let hmrAbortController: AbortController | undefined;
 let hmrRouterReadyResolve: ((router: Router) => void) | undefined;
@@ -232,9 +232,16 @@ export function RemixBrowser(_props: RemixBrowserProps): ReactElement {
         let routeId = match.route.id;
         let route = window.__remixRouteModules[routeId];
         let manifestRoute = window.__remixManifest.routes[routeId];
-        if (route && route.clientLoader && route.HydrateFallback) {
+        // Clear out the loaderData to avoid rendering the route component when the
+        // route opted into clientLoader hydration and either:
+        // * gave us a HydrateFallback
+        // * or doesn't have a server loader and we have no data to render
+        if (
+          route &&
+          shouldHydrateRouteLoader(manifestRoute, route) &&
+          (route.HydrateFallback || !manifestRoute.hasLoader)
+        ) {
           hydrationData.loaderData[routeId] = undefined;
-          didServerRenderFallback = true;
         } else if (manifestRoute && !manifestRoute.hasLoader) {
           // Since every Remix route gets a `loader` on the client side to load
           // the route JS module, we need to add a `null` value to `loaderData`
@@ -266,9 +273,10 @@ export function RemixBrowser(_props: RemixBrowserProps): ReactElement {
       mapRouteProperties,
     });
 
-    // As long as we didn't SSR a `HydrateFallback`, we can initialize immediately since
-    // there's no initial client-side data loading to perform
-    if (!didServerRenderFallback) {
+    // We can call initialize() immediately if the router doesn't have any
+    // loaders to run on hydration
+    if (router.state.initialized) {
+      routerInitialized = true;
       router.initialize();
     }
 
@@ -300,10 +308,9 @@ export function RemixBrowser(_props: RemixBrowserProps): ReactElement {
 
   // eslint-disable-next-line react-hooks/rules-of-hooks
   React.useLayoutEffect(() => {
-    // If we rendered a `HydrateFallback` on the server, delay initialization until
-    // after we've hydrated with the `HydrateFallback` in case the client loaders
-    // are synchronous
-    if (didServerRenderFallback && !routerInitialized) {
+    // If we had to run clientLoaders on hydration, we delay initialization until
+    // after we've hydrated to avoid hydration issues from synchronous client loaders
+    if (!routerInitialized) {
       routerInitialized = true;
       router.initialize();
     }

packages/remix-react/routes.tsx

@@ -264,9 +264,7 @@ export function createClientRoutes(
       };
 
       // Let React Router know whether to run this on hydration
-      dataRoute.loader.hydrate =
-        routeModule.clientLoader != null &&
-        (routeModule.clientLoader.hydrate === true || route.hasLoader !== true);
+      dataRoute.loader.hydrate = shouldHydrateRouteLoader(route, routeModule);
 
       dataRoute.action = ({ request, params }: ActionFunctionArgs) => {
         return prefetchStylesAndCallHandler(async () => {
@@ -478,3 +476,13 @@ function getRouteModuleComponent(routeModule: RouteModule) {
     return routeModule.default;
   }
 }
+
+export function shouldHydrateRouteLoader(
+  route: EntryRoute,
+  routeModule: RouteModule
+) {
+  return (
+    routeModule.clientLoader != null &&
+    (routeModule.clientLoader.hydrate === true || route.hasLoader !== true)
+  );
+}

packages/remix-react/server.tsx

@@ -8,7 +8,7 @@ import {
 import { RemixContext } from "./components";
 import type { EntryContext } from "./entry";
 import { RemixErrorBoundary } from "./errorBoundaries";
-import { createServerRoutes } from "./routes";
+import { createServerRoutes, shouldHydrateRouteLoader } from "./routes";
 
 export interface RemixServerProps {
   context: EntryContext;
@@ -49,17 +49,14 @@ export function RemixServer({
     let routeId = match.route.id;
     let route = routeModules[routeId];
     let manifestRoute = context.manifest.routes[routeId];
+    // Clear out the loaderData to avoid rendering the route component when the
+    // route opted into clientLoader hydration and either:
+    // * gave us a HydrateFallback
+    // * or doesn't have a server loader and we have no data to render
     if (
-      // This route specifically gave us a HydrateFallback
-      (route && route.clientLoader && route.HydrateFallback) ||
-      // This handles routes without a server loader but _with_ a clientLoader
-      // that will automatically opt-into clientLoader.hydrate=true.  The
-      // staticHandler always puts a `null` in loaderData for non-loader routes
-      // for proper serialization but we need to set that back to `undefined`
-      // so _renderMatches will detect a required fallback at this level
-      (manifestRoute &&
-        manifestRoute.hasLoader == false &&
-        context.staticHandlerContext.loaderData[routeId] === null)
+      route &&
+      shouldHydrateRouteLoader(manifestRoute, route) &&
+      (route.HydrateFallback || !manifestRoute.hasLoader)
     ) {
       context.staticHandlerContext.loaderData[routeId] = undefined;
     }