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

Jump to bottom

fix(react): dedupe prefetch links #7060

Merged
merged 11 commits into from
Aug 9, 2023
Merged

fix(react): dedupe prefetch links #7060

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e73c2f9
fix(react): dedupe prefetch links
markdalgleish Aug 4, 2023
6f966c1
sort keys when creating id, add dedupe test
markdalgleish Aug 7, 2023
86dc3a4
use consistent logic for dedupe and keys
markdalgleish Aug 7, 2023
57d7bb9
add changeset
markdalgleish Aug 7, 2023
e148a5c
Merge branch 'dev' into markdalgleish/dedupe-prefetch-links
markdalgleish Aug 7, 2023
741f0f9
add keys to return value when deduping
markdalgleish Aug 7, 2023
115e25f
inline key generation
markdalgleish Aug 7, 2023
3ae2067
Merge branch 'dev' into markdalgleish/dedupe-prefetch-links
markdalgleish Aug 7, 2023
17ebefd
Merge branch 'dev' into markdalgleish/dedupe-prefetch-links
markdalgleish Aug 7, 2023
0136026
Merge branch 'dev' into markdalgleish/dedupe-prefetch-links
markdalgleish Aug 8, 2023
ee1a692
Merge branch 'dev' into markdalgleish/dedupe-prefetch-links
markdalgleish Aug 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/deduplicate-prefetch-link-tags.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/react": patch
---

Deduplicate prefetch link tags
143 changes: 142 additions & 1 deletion integration/prefetch-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import { test, expect } from "@playwright/test";

import { createAppFixture, createFixture, js } from "./helpers/create-fixture";
import {
createAppFixture,
createFixture,
js,
css,
} from "./helpers/create-fixture";
import type {
Fixture,
FixtureInit,
Expand Down Expand Up @@ -424,4 +429,140 @@ test.describe("other scenarios", () => {
);
expect(stylesheets.length).toBe(1);
});

test("dedupes prefetch tags", async ({ page }) => {
fixture = await createFixture({
files: {
"app/root.tsx": js`
import {
Link,
Links,
Meta,
Outlet,
Scripts,
useLoaderData,
} from "@remix-run/react";

export default function Root() {
const styles =
'a:hover { color: red; } a:hover:after { content: " (hovered)"; }' +
'a:focus { color: green; } a:focus:after { content: " (focused)"; }';

return (
<html lang="en">
<head>
<Meta />
<Links />
</head>
<body>
<style>{styles}</style>
<h1>Root</h1>
<nav id="nav">
<Link to="/with-nested-links/nested" prefetch="intent">
Nested Links Page
</Link>
</nav>
<Outlet />
<Scripts />
</body>
</html>
);
}
`,

"app/global.css": css`
.global-class {
background-color: gray;
color: black;
}
`,

"app/local.css": css`
.local-class {
background-color: black;
color: white;
}
`,

"app/routes/_index.tsx": js`
export default function() {
return <h2 className="index">Index</h2>;
}
`,

"app/routes/with-nested-links.tsx": js`
import { Outlet } from "@remix-run/react";
import globalCss from "../global.css";

export function links() {
return [
// Same links as child route but with different key order
{
rel: "stylesheet",
href: globalCss,
},
{
rel: "preload",
as: "image",
imageSrcSet: "image-600.jpg 600w, image-1200.jpg 1200w",
imageSizes: "9999px",
},
];
}
export default function() {
return <Outlet />;
}
`,

"app/routes/with-nested-links.nested.tsx": js`
import globalCss from '../global.css';
import localCss from '../local.css';

export function links() {
return [
// Same links as parent route but with different key order
{
href: globalCss,
rel: "stylesheet",
},
{
imageSrcSet: "image-600.jpg 600w, image-1200.jpg 1200w",
imageSizes: "9999px",
rel: "preload",
as: "image",
},
// Unique links for child route
{
rel: "stylesheet",
href: localCss,
},
{
rel: "preload",
as: "image",
imageSrcSet: "image-700.jpg 700w, image-1400.jpg 1400w",
imageSizes: "9999px",
},
];
}
export default function() {
return <h2 className="with-nested-links">With Nested Links</h2>;
}
`,
},
});
appFixture = await createAppFixture(fixture);

let app = new PlaywrightFixture(appFixture, page);
await app.goto("/");
await page.hover("a[href='/with-nested-links/nested']");
await page.waitForSelector("#nav link[rel='prefetch'][as='style']", {
state: "attached",
});
expect(
await page.locator("#nav link[rel='prefetch'][as='style']").count()
).toBe(2);
expect(
await page.locator("#nav link[rel='prefetch'][as='image']").count()
).toBe(2);
});
});
38 changes: 20 additions & 18 deletions packages/remix-react/components.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,13 @@ import { RemixRootDefaultErrorBoundary } from "./errorBoundaries";
import invariant from "./invariant";
import {
getDataLinkHrefs,
getLinksForMatches,
getKeyedLinksForMatches,
getKeyedPrefetchLinks,
getModuleLinkHrefs,
getNewMatchesForLinks,
getStylesheetPrefetchLinks,
isPageLinkDescriptor,
} from "./links";
import type { HtmlLinkDescriptor, PrefetchPageDescriptor } from "./links";
import type { KeyedHtmlLinkDescriptor, PrefetchPageDescriptor } from "./links";
import { createHtml, escapeHtml } from "./markup";
import type {
MetaFunction,
Expand Down Expand Up @@ -327,16 +327,16 @@ export function Links() {
)
: routerMatches;

let links = React.useMemo(
() => getLinksForMatches(matches, routeModules, manifest),
let keyedLinks = React.useMemo(
() => getKeyedLinksForMatches(matches, routeModules, manifest),
[matches, routeModules, manifest]
);

return (
<>
{links.map((link) => {
{keyedLinks.map(({ key, link }) => {
if (isPageLinkDescriptor(link)) {
return <PrefetchPageLinks key={link.page} {...link} />;
return <PrefetchPageLinks key={key} {...link} />;
}

let imageSrcSet: string | null = null;
Expand All @@ -360,7 +360,7 @@ export function Links() {

return (
<link
key={link.rel + (link.href || "") + (imageSrcSet || "")}
key={key}
Copy link
Member Author

Choose a reason for hiding this comment

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

They key is now the exact same string used for deduping. To enable this, the utilities that return arrays of link descriptors now provide Keyed objects that expose a key property for each link.

This ensures we never get any duplicate key errors, as opposed to the previous logic which could result in duplicate keys if rel, href and/or imageSrcSet properties were the same but other properties differed.

{...{
...link,
[imageSizesKey]: imageSizes,
Expand Down Expand Up @@ -402,26 +402,28 @@ export function PrefetchPageLinks({
);
}

function usePrefetchedStylesheets(matches: AgnosticDataRouteMatch[]) {
function useKeyedPrefetchLinks(matches: AgnosticDataRouteMatch[]) {
let { manifest, routeModules } = useRemixContext();

let [styleLinks, setStyleLinks] = React.useState<HtmlLinkDescriptor[]>([]);
let [keyedPrefetchLinks, setKeyedPrefetchLinks] = React.useState<
KeyedHtmlLinkDescriptor[]
>([]);

React.useEffect(() => {
let interrupted: boolean = false;

getStylesheetPrefetchLinks(matches, manifest, routeModules).then(
(links) => {
if (!interrupted) setStyleLinks(links);
getKeyedPrefetchLinks(matches, manifest, routeModules).then((links) => {
if (!interrupted) {
setKeyedPrefetchLinks(links);
}
);
});

return () => {
interrupted = true;
};
}, [matches, manifest, routeModules]);

return styleLinks;
return keyedPrefetchLinks;
}

function PrefetchPageLinksImpl({
Expand Down Expand Up @@ -473,7 +475,7 @@ function PrefetchPageLinksImpl({

// needs to be a hook with async behavior because we need the modules, not
// just the manifest like the other links in here.
let styleLinks = usePrefetchedStylesheets(newMatchesForAssets);
let keyedPrefetchLinks = useKeyedPrefetchLinks(newMatchesForAssets);

return (
<>
Expand All @@ -483,10 +485,10 @@ function PrefetchPageLinksImpl({
{moduleHrefs.map((href) => (
<link key={href} rel="modulepreload" href={href} {...linkProps} />
))}
{styleLinks.map((link) => (
{keyedPrefetchLinks.map(({ key, link }) => (
// these don't spread `linkProps` because they are full link descriptors
// already with their own props
<link key={link.href} {...link} />
<link key={key} {...link} />
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes an unreported bug where responsive image prefetch links would all have a key of undefined since they have imgSrcSet and imgSizes properties but no href property.

))}
</>
);
Expand Down
64 changes: 44 additions & 20 deletions packages/remix-react/links.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,11 +205,11 @@ export type LinkDescriptor = HtmlLinkDescriptor | PrefetchPageDescriptor;
* Gets all the links for a set of matches. The modules are assumed to have been
* loaded already.
*/
export function getLinksForMatches(
export function getKeyedLinksForMatches(
matches: AgnosticDataRouteMatch[],
routeModules: RouteModules,
manifest: AssetsManifest
): LinkDescriptor[] {
): KeyedLinkDescriptor[] {
let descriptors = matches
.map((match): LinkDescriptor[] => {
let module = routeModules[match.route.id];
Expand All @@ -218,7 +218,7 @@ export function getLinksForMatches(
.flat(1);

let preloads = getCurrentPageModulePreloadHrefs(matches, manifest);
return dedupe(descriptors, preloads);
return dedupeLinkDescriptors(descriptors, preloads);
}

export async function prefetchStyleLinks(
Expand Down Expand Up @@ -306,11 +306,13 @@ function isHtmlLinkDescriptor(object: any): object is HtmlLinkDescriptor {
return typeof object.rel === "string" && typeof object.href === "string";
}

export async function getStylesheetPrefetchLinks(
export type KeyedHtmlLinkDescriptor = { key: string; link: HtmlLinkDescriptor };

export async function getKeyedPrefetchLinks(
Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed this from getStylesheetPrefetchLinks to getKeyedPrefetchLinks because, apart from now being an array of objects with key and link properties so consumers don't need to generate their own keys, it also makes it clearer that this code also converts preload links into prefetches (as seen on line 330).

matches: AgnosticDataRouteMatch[],
manifest: AssetsManifest,
routeModules: RouteModules
): Promise<HtmlLinkDescriptor[]> {
): Promise<KeyedHtmlLinkDescriptor[]> {
let links = await Promise.all(
matches.map(async (match) => {
let mod = await loadRouteModule(
Expand All @@ -321,15 +323,17 @@ export async function getStylesheetPrefetchLinks(
})
);

return links
.flat(1)
.filter(isHtmlLinkDescriptor)
.filter((link) => link.rel === "stylesheet" || link.rel === "preload")
.map((link) =>
link.rel === "preload"
? ({ ...link, rel: "prefetch" } as HtmlLinkDescriptor)
: ({ ...link, rel: "prefetch", as: "style" } as HtmlLinkDescriptor)
);
return dedupeLinkDescriptors(
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the core fix for #5677. The prefetch links weren't deduped as they were in getLinksForMatches (now called getKeyedLinksForMatches).

links
.flat(1)
.filter(isHtmlLinkDescriptor)
.filter((link) => link.rel === "stylesheet" || link.rel === "preload")
.map((link) =>
link.rel === "stylesheet"
? ({ ...link, rel: "prefetch", as: "style" } as HtmlLinkDescriptor)
Copy link
Member Author

Choose a reason for hiding this comment

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

This was just a slight refactor for legibility since link.rel === "stylesheet" was checked first in the earlier filter.

: ({ ...link, rel: "prefetch" } as HtmlLinkDescriptor)
)
);
}

// This is ridiculously identical to transition.ts `filterMatchesToLoad`
Expand Down Expand Up @@ -466,12 +470,32 @@ function dedupeHrefs(hrefs: string[]): string[] {
return [...new Set(hrefs)];
}

export function dedupe(descriptors: LinkDescriptor[], preloads: string[]) {
function sortKeys<Obj extends { [Key in keyof Obj]: Obj[Key] }>(obj: Obj): Obj {
let sorted = {} as Obj;
let keys = Object.keys(obj).sort();

for (let key of keys) {
sorted[key as keyof Obj] = obj[key as keyof Obj];
}

return sorted;
}

type KeyedLinkDescriptor<Descriptor extends LinkDescriptor = LinkDescriptor> = {
key: string;
link: Descriptor;
};

function dedupeLinkDescriptors<Descriptor extends LinkDescriptor>(
descriptors: Descriptor[],
preloads?: string[]
): KeyedLinkDescriptor<Descriptor>[] {
let set = new Set();
let preloadsSet = new Set(preloads);

return descriptors.reduce((deduped, descriptor) => {
let alreadyModulePreload =
preloads &&
Copy link
Member Author

Choose a reason for hiding this comment

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

Since the preloads array is now optional, we bail out early if it hasn't been provided.

!isPageLinkDescriptor(descriptor) &&
descriptor.as === "script" &&
descriptor.href &&
Expand All @@ -481,14 +505,14 @@ export function dedupe(descriptors: LinkDescriptor[], preloads: string[]) {
return deduped;
}

let str = JSON.stringify(descriptor);
if (!set.has(str)) {
set.add(str);
deduped.push(descriptor);
let key = JSON.stringify(sortKeys(descriptor));
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes an unreported bug where the deduping didn't work correctly if properties were specified in a different order. This behaviour is covered in the newly added test.

if (!set.has(key)) {
set.add(key);
deduped.push({ key, link: descriptor });
}

return deduped;
}, [] as LinkDescriptor[]);
}, [] as KeyedLinkDescriptor<Descriptor>[]);
}

// https://github.com/remix-run/history/issues/897
Expand Down