Fix partial hydration bugs when hydrating with errors (#12070)

Author: brophdawg11

.changeset/soft-maps-fix.md

@@ -0,0 +1,5 @@
+---
+"@remix-run/router": patch
+---
+
+Fix bugs with partialHydration when hydrating with errors

packages/router/__tests__/route-fallback-test.ts

@@ -157,7 +157,7 @@ describe("future.v7_partialHydration", () => {
     it("starts with initialized=true when loaders exist with partial hydration data", async () => {
       let parentSpy = jest.fn();
       let childSpy = jest.fn();
-      let router = createRouter({
+      router = createRouter({
         history: createMemoryHistory({ initialEntries: ["/child"] }),
         routes: [
           {
@@ -191,8 +191,6 @@ describe("future.v7_partialHydration", () => {
       expect(router.state.loaderData).toEqual({
         "0": "PARENT DATA",
       });
-
-      router.dispose();
     });
 
     it("does not kick off initial data load if errors exist", async () => {
@@ -203,7 +201,7 @@ describe("future.v7_partialHydration", () => {
       let parentSpy = jest.fn(() => parentDfd.promise);
       let childDfd = createDeferred();
       let childSpy = jest.fn(() => childDfd.promise);
-      let router = createRouter({
+      router = createRouter({
         history: createMemoryHistory({ initialEntries: ["/child"] }),
         routes: [
           {
@@ -245,7 +243,6 @@ describe("future.v7_partialHydration", () => {
         },
       });
 
-      router.dispose();
       consoleWarnSpy.mockReset();
     });
   });
@@ -522,7 +519,7 @@ describe("future.v7_partialHydration", () => {
       .mockImplementation(() => {});
     let parentDfd = createDeferred();
     let parentSpy = jest.fn(() => parentDfd.promise);
-    let router = createRouter({
+    router = createRouter({
       history: createMemoryHistory({ initialEntries: ["/child"] }),
       routes: [
         {
@@ -553,7 +550,122 @@ describe("future.v7_partialHydration", () => {
       },
     });
 
-    router.dispose();
     consoleWarnSpy.mockReset();
   });
+
+  it("does not kick off initial data loads below SSR error boundaries (child throw)", async () => {
+    let parentCount = 0;
+    let childCount = 0;
+    let routes = [
+      {
+        id: "parent",
+        path: "/",
+        loader: () => `PARENT ${++parentCount}`,
+        hasErrorBoundary: true,
+        children: [
+          {
+            path: "child",
+            loader: () => `CHILD ${++childCount}`,
+          },
+        ],
+      },
+    ];
+
+    // @ts-expect-error
+    routes[0].loader.hydrate = true;
+    // @ts-expect-error
+    routes[0].children[0].loader.hydrate = true;
+
+    router = createRouter({
+      history: createMemoryHistory({ initialEntries: ["/child"] }),
+      routes,
+      future: {
+        v7_partialHydration: true,
+      },
+      hydrationData: {
+        loaderData: {
+          parent: "PARENT 0",
+        },
+        errors: {
+          // Child threw and bubbled to parent
+          parent: "CHILD SSR ERROR",
+        },
+      },
+    }).initialize();
+    expect(router.state).toMatchObject({
+      initialized: false,
+      navigation: IDLE_NAVIGATION,
+      loaderData: {
+        parent: "PARENT 0",
+      },
+      errors: {
+        parent: "CHILD SSR ERROR",
+      },
+    });
+    await tick();
+    expect(router.state).toMatchObject({
+      initialized: true,
+      navigation: IDLE_NAVIGATION,
+      loaderData: {
+        parent: "PARENT 1",
+      },
+      errors: {
+        parent: "CHILD SSR ERROR",
+      },
+    });
+
+    expect(parentCount).toBe(1);
+    expect(childCount).toBe(0);
+  });
+
+  it("does not kick off initial data loads at SSR error boundaries (boundary throw)", async () => {
+    let parentCount = 0;
+    let childCount = 0;
+    let routes = [
+      {
+        id: "parent",
+        path: "/",
+        loader: () => `PARENT ${++parentCount}`,
+        hasErrorBoundary: true,
+        children: [
+          {
+            path: "child",
+            loader: () => `CHILD ${++childCount}`,
+          },
+        ],
+      },
+    ];
+
+    // @ts-expect-error
+    routes[0].loader.hydrate = true;
+    // @ts-expect-error
+    routes[0].children[0].loader.hydrate = true;
+
+    router = createRouter({
+      history: createMemoryHistory({ initialEntries: ["/child"] }),
+      routes,
+      future: {
+        v7_partialHydration: true,
+      },
+      hydrationData: {
+        loaderData: {},
+        errors: {
+          // Parent threw
+          parent: "PARENT SSR ERROR",
+        },
+      },
+    }).initialize();
+
+    expect(router.state).toMatchObject({
+      initialized: true,
+      navigation: IDLE_NAVIGATION,
+      loaderData: {},
+      errors: {
+        parent: "PARENT SSR ERROR",
+      },
+    });
+
+    expect(parentCount).toBe(0);
+    expect(childCount).toBe(0);
+  });
 });

packages/router/router.ts

@@ -890,33 +890,18 @@ export function createRouter(init: RouterInit): Router {
     // were marked for explicit hydration
     let loaderData = init.hydrationData ? init.hydrationData.loaderData : null;
     let errors = init.hydrationData ? init.hydrationData.errors : null;
-    let isRouteInitialized = (m: AgnosticDataRouteMatch) => {
-      // No loader, nothing to initialize
-      if (!m.route.loader) {
-        return true;
-      }
-      // Explicitly opting-in to running on hydration
-      if (
-        typeof m.route.loader === "function" &&
-        m.route.loader.hydrate === true
-      ) {
-        return false;
-      }
-      // Otherwise, initialized if hydrated with data or an error
-      return (
-        (loaderData && loaderData[m.route.id] !== undefined) ||
-        (errors && errors[m.route.id] !== undefined)
-      );
-    };
-
     // If errors exist, don't consider routes below the boundary
     if (errors) {
       let idx = initialMatches.findIndex(
         (m) => errors![m.route.id] !== undefined
       );
-      initialized = initialMatches.slice(0, idx + 1).every(isRouteInitialized);
+      initialized = initialMatches
+        .slice(0, idx + 1)
+        .every((m) => !shouldLoadRouteOnHydration(m.route, loaderData, errors));
     } else {
-      initialized = initialMatches.every(isRouteInitialized);
+      initialized = initialMatches.every(
+        (m) => !shouldLoadRouteOnHydration(m.route, loaderData, errors)
+      );
     }
   } else {
     // Without partial hydration - we're initialized if we were provided any
@@ -1555,7 +1540,7 @@ export function createRouter(init: RouterInit): Router {
     // Short circuit if it's only a hash change and not a revalidation or
     // mutation submission.
     //
-    // Ignore on initial page loads because since the initial load will always
+    // Ignore on initial page loads because since the initial hydration will always
     // be "same hash".  For example, on /page#hash and submit a <Form method="post">
     // which will default to a navigation to /page
     if (
@@ -2067,13 +2052,9 @@ export function createRouter(init: RouterInit): Router {
       });
     });
 
-    // During partial hydration, preserve SSR errors for routes that don't re-run
+    // Preserve SSR errors during partial hydration
     if (future.v7_partialHydration && initialHydration && state.errors) {
-      Object.entries(state.errors)
-        .filter(([id]) => !matchesToLoad.some((m) => m.route.id === id))
-        .forEach(([routeId, error]) => {
-          errors = Object.assign(errors || {}, { [routeId]: error });
-        });
+      errors = { ...state.errors, ...errors };
     }
 
     let updatedFetchers = markFetchRedirectsDone();
@@ -4355,20 +4336,18 @@ function normalizeNavigateOptions(
   return { path: createPath(parsedPath), submission };
 }
 
-// Filter out all routes below any caught error as they aren't going to
+// Filter out all routes at/below any caught error as they aren't going to
 // render so we don't need to load them
 function getLoaderMatchesUntilBoundary(
   matches: AgnosticDataRouteMatch[],
-  boundaryId: string
+  boundaryId: string,
+  includeBoundary = false
 ) {
-  let boundaryMatches = matches;
-  if (boundaryId) {
-    let index = matches.findIndex((m) => m.route.id === boundaryId);
-    if (index >= 0) {
-      boundaryMatches = matches.slice(0, index);
-    }
+  let index = matches.findIndex((m) => m.route.id === boundaryId);
+  if (index >= 0) {
+    return matches.slice(0, includeBoundary ? index + 1 : index);
   }
-  return boundaryMatches;
+  return matches;
 }
 
 function getMatchesToLoad(
@@ -4377,7 +4356,7 @@ function getMatchesToLoad(
   matches: AgnosticDataRouteMatch[],
   submission: Submission | undefined,
   location: Location,
-  isInitialLoad: boolean,
+  initialHydration: boolean,
   skipActionErrorRevalidation: boolean,
   isRevalidationRequired: boolean,
   cancelledDeferredRoutes: string[],
@@ -4398,13 +4377,26 @@ function getMatchesToLoad(
   let nextUrl = history.createURL(location);
 
   // Pick navigation matches that are net-new or qualify for revalidation
-  let boundaryId =
-    pendingActionResult && isErrorResult(pendingActionResult[1])
-      ? pendingActionResult[0]
-      : undefined;
-  let boundaryMatches = boundaryId
-    ? getLoaderMatchesUntilBoundary(matches, boundaryId)
-    : matches;
+  let boundaryMatches = matches;
+  if (initialHydration && state.errors) {
+    // On initial hydration, only consider matches up to _and including_ the boundary.
+    // This is inclusive to handle cases where a server loader ran successfully,
+    // a child server loader bubbled up to this route, but this route has
+    // `clientLoader.hydrate` so we want to still run the `clientLoader` so that
+    // we have a complete version of `loaderData`
+    boundaryMatches = getLoaderMatchesUntilBoundary(
+      matches,
+      Object.keys(state.errors)[0],
+      true
+    );
+  } else if (pendingActionResult && isErrorResult(pendingActionResult[1])) {
+    // If an action threw an error, we call loaders up to, but not including the
+    // boundary
+    boundaryMatches = getLoaderMatchesUntilBoundary(
+      matches,
+      pendingActionResult[0]
+    );
+  }
 
   // Don't revalidate loaders by default after action 4xx/5xx responses
   // when the flag is enabled.  They can still opt-into revalidation via
@@ -4426,15 +4418,8 @@ function getMatchesToLoad(
       return false;
     }
 
-    if (isInitialLoad) {
-      if (typeof route.loader !== "function" || route.loader.hydrate) {
-        return true;
-      }
-      return (
-        state.loaderData[route.id] === undefined &&
-        // Don't re-run if the loader ran and threw an error
-        (!state.errors || state.errors[route.id] === undefined)
-      );
+    if (initialHydration) {
+      return shouldLoadRouteOnHydration(route, state.loaderData, state.errors);
     }
 
     // Always call the loader on new route instances and pending defer cancellations
@@ -4476,12 +4461,12 @@ function getMatchesToLoad(
   let revalidatingFetchers: RevalidatingFetcher[] = [];
   fetchLoadMatches.forEach((f, key) => {
     // Don't revalidate:
-    //  - on initial load (shouldn't be any fetchers then anyway)
+    //  - on initial hydration (shouldn't be any fetchers then anyway)
     //  - if fetcher won't be present in the subsequent render
     //    - no longer matches the URL (v7_fetcherPersist=false)
     //    - was unmounted but persisted due to v7_fetcherPersist=true
     if (
-      isInitialLoad ||
+      initialHydration ||
       !matches.some((m) => m.route.id === f.routeId) ||
       deletedFetchers.has(key)
     ) {
@@ -4561,6 +4546,38 @@ function getMatchesToLoad(
   return [navigationMatches, revalidatingFetchers];
 }
 
+function shouldLoadRouteOnHydration(
+  route: AgnosticDataRouteObject,
+  loaderData: RouteData | null | undefined,
+  errors: RouteData | null | undefined
+) {
+  // We dunno if we have a loader - gotta find out!
+  if (route.lazy) {
+    return true;
+  }
+
+  // No loader, nothing to initialize
+  if (!route.loader) {
+    return false;
+  }
+
+  let hasData = loaderData != null && loaderData[route.id] !== undefined;
+  let hasError = errors != null && errors[route.id] !== undefined;
+
+  // Don't run if we error'd during SSR
+  if (!hasData && hasError) {
+    return false;
+  }
+
+  // Explicitly opting-in to running on hydration
+  if (typeof route.loader === "function" && route.loader.hydrate === true) {
+    return true;
+  }
+
+  // Otherwise, run if we're not yet initialized with anything
+  return !hasData && !hasError;
+}
+
 function isNewLoader(
   currentLoaderData: RouteData,
   currentMatch: AgnosticDataRouteMatch,