Skip to content
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

Dont prefetch server loader when client loader exists #9580

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Dont prefetch server loader when client loader exists #9580

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/heavy-ways-rule.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/react": patch
---

Don't prefetch server loader data when clientLoader exists
86 changes: 86 additions & 0 deletions integration/client-data-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -977,6 +977,48 @@ test.describe("Client Data", () => {
'not have a server loader (routeId: "routes/parent.child")'
);
});

test("does not prefetch server loader if a client loader is present", async ({
page,
}) => {
appFixture = await createAppFixture(
await createTestFixture({
files: {
...getFiles({
parentClientLoader: true,
parentClientLoaderHydrate: false,
childClientLoader: false,
childClientLoaderHydrate: false,
}),
"app/routes/_index.tsx": js`
import { Link } from '@remix-run/react'
export default function Component() {
return (
<>
<Link prefetch="render" to="/parent">Go to /parent</Link>
<Link prefetch="render" to="/parent/child">Go to /parent/child</Link>
</>
);
}
`,
},
})
);

let dataUrls: string[] = [];
page.on("request", (request) => {
if (request.url().includes("_data")) {
dataUrls.push(request.url());
}
});

let app = new PlaywrightFixture(appFixture, page);
await app.goto("/", true);
// Only prefetch child server loader since parent has a `clientLoader`
expect(dataUrls).toEqual([
expect.stringMatching(/parent\/child\?_data=routes%2Fparent\.child/),
]);
});
});

test.describe("clientAction - critical route module", () => {
Expand Down Expand Up @@ -2212,6 +2254,50 @@ test.describe("single fetch", () => {
'not have a server loader (routeId: "routes/parent.child")'
);
});

test("does not prefetch server loader if a client loader is present", async ({
page,
}) => {
appFixture = await createAppFixture(
await createTestFixture({
files: {
...getFiles({
parentClientLoader: true,
parentClientLoaderHydrate: false,
childClientLoader: false,
childClientLoaderHydrate: false,
}),
"app/routes/_index.tsx": js`
import { Link } from '@remix-run/react'
export default function Component() {
return (
<>
<Link prefetch="render" to="/parent">Go to /parent</Link>
<Link prefetch="render" to="/parent/child">Go to /parent/child</Link>
</>
);
}
`,
},
})
);

let dataUrls: string[] = [];
page.on("request", (request) => {
if (request.url().includes(".data")) {
dataUrls.push(request.url());
}
});

let app = new PlaywrightFixture(appFixture, page);
await app.goto("/", true);
// Only prefetch child server loader since parent has a `clientLoader`
expect(dataUrls).toEqual([
expect.stringMatching(
/parent\/child\.data\?_routes=routes%2Fparent\.child/
),
]);
});
});

test.describe("clientAction - critical route module", () => {
Expand Down
6 changes: 5 additions & 1 deletion packages/remix-react/links.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,11 @@ export function getDataLinkHrefs(
let path = parsePathPatch(page);
return dedupeHrefs(
matches
.filter((match) => manifest.routes[match.route.id].hasLoader)
.filter(
(match) =>
manifest.routes[match.route.id].hasLoader &&
!manifest.routes[match.route.id].hasClientLoader
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check with @ryanflorence on this - current behavior is desired for folks who are calling await serverLoader() unconditionally because we can prefetch that so it resolves immediately. This new solution hurts those scenarios in favor of apps that conditionally call serverLoader(). Maybe we need to provide some way to enable "prefetching" (pre-executing) of client loaders - either by stashing their results into an internal cache or adding the type field to loaders we've previously discussed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Chatted and agreed we should only prefetch module links when a client loader exists

Copy link

Choose a reason for hiding this comment

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

Would be great to have the option to prefetch/pre-execute client loaders. In my case I am using a library that has its own cache, so calling the client loader is all that's needed.

.map((match) => {
let { pathname, search } = path;
let searchParams = new URLSearchParams(search);
Expand Down