Skip to content

Commit

Permalink
Fix partial hydration bugs when hydrating with errors
Browse files Browse the repository at this point in the history
  • Loading branch information
@brophdawg11
brophdawg11 committed Oct 4, 2024
1 parent eeb86ab commit 998558f
Show file tree
Hide file tree
Showing 3 changed files with 197 additions and 63 deletions.
5 changes: 5 additions & 0 deletions .changeset/soft-maps-fix.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/router": patch
---

Fix bugs with partialHydration when hydrating with errors
126 changes: 119 additions & 7 deletions packages/router/__tests__/route-fallback-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: [
{
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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: [
{
Expand Down Expand Up @@ -245,7 +243,6 @@ describe("future.v7_partialHydration", () => {
},
});

router.dispose();
consoleWarnSpy.mockReset();
});
});
Expand Down Expand Up @@ -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: [
{
Expand Down Expand Up @@ -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);
});
});
129 changes: 73 additions & 56 deletions packages/router/router.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -894,33 +894,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
Expand Down Expand Up @@ -1559,7 +1544,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 @@ -2073,13 +2058,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();
Expand Down Expand Up @@ -4352,20 +4333,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 @@ -4374,7 +4353,7 @@ function getMatchesToLoad(
matches: AgnosticDataRouteMatch[],
submission: Submission | undefined,
location: Location,
isInitialLoad: boolean,
initialHydration: boolean,
skipActionErrorRevalidation: boolean,
isRevalidationRequired: boolean,
cancelledDeferredRoutes: string[],
Expand All @@ -4395,13 +4374,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 @@ -4423,15 +4415,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
Expand Down Expand Up @@ -4473,12 +4458,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)
) {
Expand Down Expand Up @@ -4558,6 +4543,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 998558f

Please sign in to comment.