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

Align single fetch prefetching with new revalidation logic #9958

Merged
merged 6 commits into from
Sep 6, 2024
+282 −98
Merged

Align single fetch prefetching with new revalidation logic #9958

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4b9ba88
Align single fetch prefetching with new revalidation logic
brophdawg11 Sep 6, 2024
2c31dc8
Remove unused function
brophdawg11 Sep 6, 2024
8ccdf7b
Flatten
brophdawg11 Sep 6, 2024
25f27f9
Fix current page links
brophdawg11 Sep 6, 2024
1ace15c
Add back into useMemo
brophdawg11 Sep 6, 2024
98186d6
memoize just the hrefs
brophdawg11 Sep 6, 2024
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/giant-olives-sort.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/react": patch
---

[REMOVE] Align single fetch prefetchign with new revalidation logic
212 changes: 208 additions & 4 deletions integration/single-fetch-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3253,9 +3253,9 @@ test.describe("single-fetch", () => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/", true);

// A/B can be prefetched, C doesn't get prefetched due to its `clientLoader`
// root/A/B can be prefetched, C doesn't get prefetched due to its `clientLoader`
await page.waitForSelector(
"nav link[rel='prefetch'][as='fetch'][href='/a/b/c.data?_routes=routes%2Fa%2Croutes%2Fa.b']",
"nav link[rel='prefetch'][as='fetch'][href='/a/b/c.data?_routes=root%2Croutes%2Fa%2Croutes%2Fa.b']",
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 was wrong before because the root route has a loader and should be re-fetched by default

{ state: "attached" }
);
expect(await app.page.locator("nav link[as='fetch']").count()).toEqual(1);
Expand Down Expand Up @@ -3351,9 +3351,9 @@ test.describe("single-fetch", () => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/", true);

// Only A can get prefetched, B/C can't due to `clientLoader`
// root/A can get prefetched, B/C can't due to `clientLoader`
await page.waitForSelector(
"nav link[rel='prefetch'][as='fetch'][href='/a/b/c.data?_routes=routes%2Fa']",
"nav link[rel='prefetch'][as='fetch'][href='/a/b/c.data?_routes=root%2Croutes%2Fa']",
{ state: "attached" }
);
expect(await app.page.locator("nav link[as='fetch']").count()).toEqual(1);
Expand All @@ -3368,6 +3368,35 @@ test.describe("single-fetch", () => {
},
files: {
...files,
"app/root.tsx": js`
import { Links, Meta, Outlet, Scripts } from "@remix-run/react";

export function loader() {
return {
message: "ROOT",
};
}

export async function clientLoader({ serverLoader }) {
let data = await serverLoader();
return { message: data.message + " (root client loader)" };
}

export default function Root() {
return (
<html lang="en">
<head>
<Meta />
<Links />
</head>
<body>
<Outlet />
<Scripts />
</body>
</html>
);
}
`,
"app/routes/_index.tsx": js`
import { Link } from "@remix-run/react";

Expand Down Expand Up @@ -3457,6 +3486,181 @@ test.describe("single-fetch", () => {
// No prefetching due to clientLoaders
expect(await app.page.locator("nav link[as='fetch']").count()).toEqual(0);
});

test("when a reused route opts out of revalidation", async ({ page }) => {
let fixture = await createFixture({
config: {
future: {
unstable_singleFetch: true,
},
},
files: {
...files,
"app/routes/a.tsx": js`
import { Link, Outlet, useLoaderData } from '@remix-run/react';

export function loader() {
return { message: "A server loader" };
}

export function shouldRevalidate() {
return false;
}

export default function Comp() {
let data = useLoaderData();
return (
<>
<h1>A</h1>
<p id="a-data">{data.message}</p>
<nav>
<Link to="/a/b/c" prefetch="render">/a/b/c</Link>
</nav>
<Outlet/>
</>
);
}
`,
"app/routes/a.b.tsx": js`
import { Outlet, useLoaderData } from '@remix-run/react';

export function loader() {
return { message: "B server loader" };
}

export default function Comp() {
let data = useLoaderData();
return (
<>
<h1>B</h1>
<p id="b-data">{data.message}</p>
<Outlet/>
</>
);
}
`,
"app/routes/a.b.c.tsx": js`
import { useLoaderData } from '@remix-run/react';

export function loader() {
return { message: "C server loader" };
}

export default function Comp() {
let data = useLoaderData();
return (
<>
<h1>C</h1>
<p id="c-data">{data.message}</p>
</>
);
}
`,
},
});

let appFixture = await createAppFixture(fixture);
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/a", true);

// A opted out of revalidation
await page.waitForSelector(
"link[rel='prefetch'][as='fetch'][href='/a/b/c.data?_routes=root%2Croutes%2Fa.b%2Croutes%2Fa.b.c']",
{ state: "attached" }
);
expect(await app.page.locator("nav link[as='fetch']").count()).toEqual(1);
});

test("when a reused route opts out of revalidation and another route has a clientLoader", async ({
page,
}) => {
let fixture = await createFixture({
config: {
future: {
unstable_singleFetch: true,
},
},
files: {
...files,
"app/routes/a.tsx": js`
import { Link, Outlet, useLoaderData } from '@remix-run/react';

export function loader() {
return { message: "A server loader" };
}

export function shouldRevalidate() {
return false;
}

export default function Comp() {
let data = useLoaderData();
return (
<>
<h1>A</h1>
<p id="a-data">{data.message}</p>
<nav>
<Link to="/a/b/c" prefetch="render">/a/b/c</Link>
</nav>
<Outlet/>
</>
);
}
`,
"app/routes/a.b.tsx": js`
import { Outlet, useLoaderData } from '@remix-run/react';

export function loader() {
return { message: "B server loader" };
}

export default function Comp() {
let data = useLoaderData();
return (
<>
<h1>B</h1>
<p id="b-data">{data.message}</p>
<Outlet/>
</>
);
}
`,
"app/routes/a.b.c.tsx": js`
import { useLoaderData } from '@remix-run/react';

export function loader() {
return { message: "C server loader" };
}

export async function clientLoader({ serverLoader }) {
let data = await serverLoader();
return { message: data.message + " (C client loader)" };
}

export default function Comp() {
let data = useLoaderData();
return (
<>
<h1>C</h1>
<p id="c-data">{data.message}</p>
</>
);
}
`,
},
});

let appFixture = await createAppFixture(fixture);
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/a", true);

// A opted out of revalidation
await page.waitForSelector(
"nav link[rel='prefetch'][as='fetch'][href='/a/b/c.data?_routes=root%2Croutes%2Fa.b']",
{ state: "attached" }
);
expect(await app.page.locator("nav link[as='fetch']").count()).toEqual(1);
});
});

test("supports nonce on streaming script tags", async ({ page }) => {
Expand Down
96 changes: 66 additions & 30 deletions packages/remix-react/components.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ function PrefetchPageLinksImpl({
}) {
let location = useLocation();
let { future, manifest, routeModules } = useRemixContext();
let { matches } = useDataRouterStateContext();
let { loaderData, matches } = useDataRouterStateContext();

let newMatchesForData = React.useMemo(
() =>
Expand Down Expand Up @@ -491,37 +491,73 @@ function PrefetchPageLinksImpl({
// just the manifest like the other links in here.
let keyedPrefetchLinks = useKeyedPrefetchLinks(newMatchesForAssets);

let linksToRender: React.ReactNode | React.ReactNode[] | null = null;
if (!future.unstable_singleFetch) {
// Non-single-fetch prefetching
linksToRender = dataHrefs.map((href) => (
<link key={href} rel="prefetch" as="fetch" href={href} {...linkProps} />
));
} else if (newMatchesForData.length > 0) {
// Single-fetch with routes that require data
let url = addRevalidationParam(
manifest,
routeModules,
nextMatches.map((m) => m.route),
newMatchesForData.map((m) => m.route),
singleFetchUrl(page)
);
if (url.searchParams.get("_routes") !== "") {
linksToRender = (
<link
key={url.pathname + url.search}
rel="prefetch"
as="fetch"
href={url.pathname + url.search}
{...linkProps}
/>
);
} else {
let linksToRender = React.useMemo(() => {
if (!future.unstable_singleFetch) {
// Non-single-fetch prefetching is easy...
return dataHrefs.map((href) => (
<link key={href} rel="prefetch" as="fetch" href={href} {...linkProps} />
));
}

// Single-fetch is harder :)
// This parallels the logic in the single fetch data strategy
let routesParams = new Set<string>();
let foundOptOutRoute = false;
nextMatches.forEach((m) => {
if (manifest.routes[m.route.id].hasLoader) {
if (
!newMatchesForData.some((m2) => m2.route.id === m.route.id) &&
m.route.id in loaderData &&
routeModules[m.route.id]?.shouldRevalidate
) {
foundOptOutRoute = true;
} else if (manifest.routes[m.route.id].hasClientLoader) {
foundOptOutRoute = true;
} else {
routesParams.add(m.route.id);
}
}
});

if (routesParams.size === 0) {
// No single-fetch prefetching if _routes param is empty due to `clientLoader`'s
return null;
}
} else {
// No single-fetch prefetching if there are no new matches for data
}

let url = singleFetchUrl(page);
// When one or more routes have opted out, we add a _routes param to
// limit the loaders to those that have a server loader and did not
// opt out
if (foundOptOutRoute && routesParams.size > 0) {
url.searchParams.set(
"_routes",
nextMatches
.filter((m) => routesParams.has(m.route.id))
.map((m) => m.route.id)
.join(",")
);
}

return (
<link
key={url.pathname + url.search}
rel="prefetch"
as="fetch"
href={url.pathname + url.search}
{...linkProps}
/>
);
}, [
dataHrefs,
future.unstable_singleFetch,
linkProps,
loaderData,
manifest.routes,
newMatchesForData,
nextMatches,
page,
routeModules,
]);

return (
<>
Expand Down
Loading