Skip to content

Commit

Permalink
Sort lazy route discovery manifest request params (#9888)
Browse files Browse the repository at this point in the history
  • Loading branch information
@brophdawg11
brophdawg11 authored Aug 20, 2024
1 parent dfa8120 commit 79067be
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 19 deletions.
5 changes: 5 additions & 0 deletions .changeset/dirty-adults-sparkle.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/react": patch
---

Fog of War: Sort `/__manifest` query parameters for better caching
124 changes: 106 additions & 18 deletions integration/fog-of-war-test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import type { Request as PlaywrightRequest } from "@playwright/test";
import { test, expect } from "@playwright/test";

import {
Expand Down Expand Up @@ -836,9 +835,7 @@ test.describe("Fog of War", () => {
expect(await app.getHtml("#parent")).toMatch(`Parent`);
expect(await app.getHtml("#child2")).toMatch(`Child 2`);
expect(manifestRequests).toEqual([
expect.stringMatching(
/\/__manifest\?version=[a-z0-9]{8}&p=%2Fparent%2Fchild2/
),
expect.stringMatching(/\/__manifest\?p=%2Fparent%2Fchild2/),
]);
});

Expand Down Expand Up @@ -912,7 +909,7 @@ test.describe("Fog of War", () => {
)
).toEqual(["root", "routes/_index", "routes/$slug"]);
expect(manifestRequests).toEqual([
expect.stringMatching(/\/__manifest\?version=[a-z0-9]{8}&p=%2Fsomething/),
expect.stringMatching(/\/__manifest\?p=%2Fsomething/),
]);
manifestRequests = [];

Expand All @@ -926,7 +923,7 @@ test.describe("Fog of War", () => {
await page.waitForSelector("#static");
expect(await app.getHtml("#static")).toMatch("Static");
expect(manifestRequests).toEqual([
expect.stringMatching(/\/__manifest\?version=[a-z0-9]{8}&p=%2Fstatic/),
expect.stringMatching(/\/__manifest\?p=%2Fstatic/),
]);
expect(
await page.evaluate(() =>
Expand Down Expand Up @@ -1005,7 +1002,7 @@ test.describe("Fog of War", () => {
)
).toEqual(["root", "routes/_index", "routes/$"]);
expect(manifestRequests).toEqual([
expect.stringMatching(/\/__manifest\?version=[a-z0-9]{8}&p=%2Fsomething/),
expect.stringMatching(/\/__manifest\?p=%2Fsomething/),
]);
manifestRequests = [];

Expand All @@ -1019,7 +1016,7 @@ test.describe("Fog of War", () => {
await page.waitForSelector("#static");
expect(await app.getHtml("#static")).toMatch("Static");
expect(manifestRequests).toEqual([
expect.stringMatching(/\/__manifest\?version=[a-z0-9]{8}&p=%2Fstatic/),
expect.stringMatching(/\/__manifest\?p=%2Fstatic/),
]);
expect(
await page.evaluate(() =>
Expand Down Expand Up @@ -1097,7 +1094,7 @@ test.describe("Fog of War", () => {
await page.waitForSelector("#slug");
expect(await app.getHtml("#slug")).toMatch("Slug: a");
expect(manifestRequests).toEqual([
expect.stringMatching(/\/__manifest\?version=[a-z0-9]{8}&p=%2Fa/),
expect.stringMatching(/\/__manifest\?p=%2Fa/),
]);
manifestRequests = [];

Expand All @@ -1118,7 +1115,7 @@ test.describe("Fog of War", () => {
await page.waitForSelector("#slug");
expect(await app.getHtml("#slug")).toMatch("Slug: b");
expect(manifestRequests).toEqual([
expect.stringMatching(/\/__manifest\?version=[a-z0-9]{8}&p=%2Fb/),
expect.stringMatching(/\/__manifest\?p=%2Fb/),
]);
});

Expand Down Expand Up @@ -1191,7 +1188,7 @@ test.describe("Fog of War", () => {
await page.waitForSelector("#splat");
expect(await app.getHtml("#splat")).toMatch("Splat: a");
expect(manifestRequests).toEqual([
expect.stringMatching(/\/__manifest\?version=[a-z0-9]{8}&p=%2Fa/),
expect.stringMatching(/\/__manifest\?p=%2Fa/),
]);
manifestRequests = [];

Expand All @@ -1212,7 +1209,7 @@ test.describe("Fog of War", () => {
await page.waitForSelector("#splat");
expect(await app.getHtml("#splat")).toMatch("Splat: b/c");
expect(manifestRequests).toEqual([
expect.stringMatching(/\/__manifest\?version=[a-z0-9]{8}&p=%2Fb%2Fc/),
expect.stringMatching(/\/__manifest\?p=%2Fb%2Fc/),
]);
});

Expand Down Expand Up @@ -1291,17 +1288,15 @@ test.describe("Fog of War", () => {
await app.clickLink("/not/a/path");
await page.waitForSelector("#error");
expect(manifestRequests).toEqual([
expect.stringMatching(
/\/__manifest\?version=[a-z0-9]{8}&p=%2Fnot%2Fa%2Fpath/
),
expect.stringMatching(/\/__manifest\?p=%2Fnot%2Fa%2Fpath/),
]);
manifestRequests = [];

// Go to a valid slug route
await app.clickLink("/something");
await page.waitForSelector("#slug");
expect(manifestRequests).toEqual([
expect.stringMatching(/\/__manifest\?version=[a-z0-9]{8}&p=%2Fsomething/),
expect.stringMatching(/\/__manifest\?p=%2Fsomething/),
]);
manifestRequests = [];

Expand Down Expand Up @@ -1340,10 +1335,10 @@ test.describe("Fog of War", () => {
let appFixture = await createAppFixture(fixture);
let app = new PlaywrightFixture(appFixture, page);

let manifestRequests: PlaywrightRequest[] = [];
let manifestRequests: string[] = [];
page.on("request", (req) => {
if (req.url().includes("/__manifest")) {
manifestRequests.push(req);
manifestRequests.push(req.url());
}
});

Expand All @@ -1360,4 +1355,97 @@ test.describe("Fog of War", () => {
)
).toEqual(["root", "routes/_index", "routes/a"]);
});

test("includes a version query parameter as a cachebuster", async ({
page,
}) => {
let fixture = await createFixture({
config: {
future: {
unstable_lazyRouteDiscovery: true,
},
},
files: {
...getFiles(),
"app/routes/_index.tsx": js`
import { Link } from "@remix-run/react";
export default function Index() {
return (
<>
<h1 id="index">Index</h1>
<Link to="/a">/a</Link>
<Link to="/b">/b</Link>
</>
);
}
`,
},
});
let appFixture = await createAppFixture(fixture);
let app = new PlaywrightFixture(appFixture, page);

let manifestRequests: string[] = [];
page.on("request", (req) => {
if (req.url().includes("/__manifest")) {
manifestRequests.push(req.url());
}
});

await app.goto("/", true);
await new Promise((resolve) => setTimeout(resolve, 250));
expect(manifestRequests).toEqual([
expect.stringMatching(
/\/__manifest\?p=%2F&p=%2Fa&p=%2Fb&version=[a-z0-9]{8}/
),
]);
});

test("sorts url parameters", async ({ page }) => {
let fixture = await createFixture({
config: {
future: {
unstable_lazyRouteDiscovery: true,
},
},
files: {
...getFiles(),
"app/routes/_index.tsx": js`
import { Link } from "@remix-run/react";
export default function Index() {
return (
<>
<h1 id="index">Index</h1>
<Link to="/a">/a</Link>
<Link to="/c">/c</Link>
<Link to="/e">/e</Link>
<Link to="/g">/g</Link>
<Link to="/f">/f</Link>
<Link to="/d">/d</Link>
<Link to="/b">/b</Link>
</>
);
}
`,
},
});
let appFixture = await createAppFixture(fixture);
let app = new PlaywrightFixture(appFixture, page);

let manifestRequests: string[] = [];
page.on("request", (req) => {
if (req.url().includes("/__manifest")) {
manifestRequests.push(req.url());
}
});

await app.goto("/", true);
await new Promise((resolve) => setTimeout(resolve, 250));
expect(manifestRequests).toEqual([
expect.stringMatching(
/\/__manifest\?p=%2F&p=%2Fa&p=%2Fb&p=%2Fc&p=%2Fd&p=%2Fe&p=%2Ff&p=%2F/
),
]);
});
});
2 changes: 1 addition & 1 deletion packages/remix-react/fog-of-war.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,8 +211,8 @@ export async function fetchAndApplyManifestPatches(
): Promise<void> {
let manifestPath = `${basename ?? "/"}/__manifest`.replace(/\/+/g, "/");
let url = new URL(manifestPath, window.location.origin);
paths.sort().forEach((path) => url.searchParams.append("p", path));
url.searchParams.set("version", manifest.version);
paths.forEach((path) => url.searchParams.append("p", path));

// If the URL is nearing the ~8k limit on GET requests, skip this optimization
// step and just let discovery happen on link click. We also wipe out the
Expand Down

0 comments on commit 79067be

Please sign in to comment.