Skip to content

Commit

Permalink
Fix partial hydration bugs when hydrating with errors (#12070)
Browse files Browse the repository at this point in the history
  • Loading branch information
@brophdawg11
brophdawg11 committed Oct 10, 2024
1 parent 4d56751 commit ebacd98
Show file tree
Hide file tree
Showing 2 changed files with 190 additions and 64 deletions.
120 changes: 114 additions & 6 deletions packages/react-router/__tests__/router/route-fallback-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { IDLE_NAVIGATION, createRouter } from "../../lib/router/router";

import { urlMatch } from "./utils/custom-matchers";
import { createDeferred } from "./utils/data-router-setup";
import { tick } from "./utils/utils";

interface CustomMatchers<R = jest.Expect> {
urlMatch(url: string);
Expand Down Expand Up @@ -188,7 +189,7 @@ describe("route HydrateFallback", () => {
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: [
{
Expand Down Expand Up @@ -230,7 +231,6 @@ describe("route HydrateFallback", () => {
},
});

router.dispose();
consoleWarnSpy.mockReset();
});

Expand All @@ -242,7 +242,7 @@ describe("route HydrateFallback", () => {
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: [
{
Expand Down Expand Up @@ -284,7 +284,6 @@ describe("route HydrateFallback", () => {
},
});

router.dispose();
consoleWarnSpy.mockReset();
});

Expand All @@ -294,7 +293,7 @@ describe("route HydrateFallback", () => {
.mockImplementation(() => {});
let parentDfd = createDeferred();
let parentSpy = jest.fn(() => parentDfd.promise);
let router = createRouter({
router = createRouter({
history: createMemoryHistory({ initialEntries: ["/child"] }),
routes: [
{
Expand Down Expand Up @@ -322,7 +321,116 @@ describe("route HydrateFallback", () => {
},
});

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,
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,
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);
});
});
134 changes: 76 additions & 58 deletions packages/react-router/lib/router/router.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -897,33 +897,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)
);
}
}

Expand Down Expand Up @@ -1564,7 +1549,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 (
Expand Down Expand Up @@ -2043,13 +2028,9 @@ export function createRouter(init: RouterInit): Router {
fetcherResults
);

// With "partial hydration", preserve SSR errors for routes that don't re-run
// Preserve SSR errors during partial hydration
if (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();
Expand Down Expand Up @@ -4181,20 +4162,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(
Expand All @@ -4203,7 +4182,7 @@ function getMatchesToLoad(
matches: AgnosticDataRouteMatch[],
submission: Submission | undefined,
location: Location,
isInitialLoad: boolean,
initialHydration: boolean,
isRevalidationRequired: boolean,
cancelledFetcherLoads: Set<string>,
fetchersQueuedForDeletion: Set<string>,
Expand All @@ -4222,13 +4201,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
Expand All @@ -4249,15 +4241,8 @@ function getMatchesToLoad(
return false;
}

if (isInitialLoad) {
if (typeof route.loader !== "function" || route.loader.hydrate) {
return true;
}
return (
!state.loaderData.hasOwnProperty(route.id) &&
// 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
Expand Down Expand Up @@ -4296,11 +4281,12 @@ function getMatchesToLoad(
let revalidatingFetchers: RevalidatingFetcher[] = [];
fetchLoadMatches.forEach((f, key) => {
// Don't revalidate:
// - on initial load (shouldn't be any fetchers then anyway)
// - if fetcher no longer matches the URL
// - if fetcher was unmounted
// - 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) ||
fetchersQueuedForDeletion.has(key)
) {
Expand Down Expand Up @@ -4380,6 +4366,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,
Expand Down

0 comments on commit ebacd98

Please sign in to comment.