Skip to content

Fix partial hydration bugs when hydrating with errors #12070

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 7, 2024
Merged

Fix partial hydration bugs when hydrating with errors #12070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/soft-maps-fix.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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 });
});
Comment on lines -2078 to -2082
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This didn't work because it would lose errors that had bubbled up - we keep all SSR errors since hydration is not the time to "fix" them, and we let errors thrown from the hydration data load take precedence, but I don't think there will ever be overlap. Merging is required though so if a higher route throws we catch the error at the higher boundary.

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