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

Sort lazy route discovery manifest request params #9888

Merged
merged 3 commits into from
Aug 20, 2024
Merged

Sort lazy route discovery manifest request params #9888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d00129
Sort lazy route discovery manifest request params
brophdawg11 Aug 19, 2024
bc7a3db
Fix lint error
brophdawg11 Aug 19, 2024
d9910c6
Update .changeset/dirty-adults-sparkle.md
brophdawg11 Aug 20, 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/dirty-adults-sparkle.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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