Skip to content

Commit 49815ae

Browse files
jplhomerbrophdawg11
jplhomer
and
brophdawg11
authored
Use comma-separated list for p values in lazy manifest requests (#14321)
* Use comma-separated list for `p` values in lazy manifest requests * Sign the CLA * Rename p -> paths * Add changeset --------- Co-authored-by: Matt Brophy <matt@brophy.org>
1 parent 4c34bdf commit 49815ae

File tree

7 files changed

+33
-22
lines changed

7 files changed

+33
-22
lines changed

‎.changeset/chilly-needles-taste.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
---
2+
"react-router": patch
3+
---
4+
5+
Update Lazy Route Discovery manifest requests to use a singular comma-separated `paths` query param instead of repeated `p` query params
6+
7+
- This is because Cloudflare has a hard limit of 100 URL search param key/value pairs when used as a key for caching purposes
8+
- If more that 100 paths were included, the cache key would be incomplete and could produce false-positive cache hits

‎contributors.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,7 @@
189189
- johnpangalos
190190
- jonkoops
191191
- joseph0926
192+
- jplhomer
192193
- jrakotoharisoa
193194
- jrestall
194195
- juanpprieto

‎integration/fog-of-war-test.ts‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -715,7 +715,7 @@ test.describe("Fog of War", () => {
715715
expect(await app.getHtml("#parent")).toMatch(`Parent`);
716716
expect(await app.getHtml("#child2")).toMatch(`Child 2`);
717717
expect(manifestRequests).toEqual([
718-
expect.stringMatching(/\/__manifest\?p=%2Fparent%2Fchild2&version=/),
718+
expect.stringMatching(/\/__manifest\?paths=%2Fparent%2Fchild2&version=/),
719719
]);
720720
});
721721

@@ -783,7 +783,7 @@ test.describe("Fog of War", () => {
783783
),
784784
).toEqual(["root", "routes/_index", "routes/$slug"]);
785785
expect(manifestRequests).toEqual([
786-
expect.stringMatching(/\/__manifest\?p=%2Fsomething&version=/),
786+
expect.stringMatching(/\/__manifest\?paths=%2Fsomething&version=/),
787787
]);
788788
manifestRequests = [];
789789

@@ -797,7 +797,7 @@ test.describe("Fog of War", () => {
797797
await page.waitForSelector("#static");
798798
expect(await app.getHtml("#static")).toMatch("Static");
799799
expect(manifestRequests).toEqual([
800-
expect.stringMatching(/\/__manifest\?p=%2Fstatic&version=/),
800+
expect.stringMatching(/\/__manifest\?paths=%2Fstatic&version=/),
801801
]);
802802
expect(
803803
await page.evaluate(() =>
@@ -870,7 +870,7 @@ test.describe("Fog of War", () => {
870870
),
871871
).toEqual(["root", "routes/_index", "routes/$"]);
872872
expect(manifestRequests).toEqual([
873-
expect.stringMatching(/\/__manifest\?p=%2Fsomething&version=/),
873+
expect.stringMatching(/\/__manifest\?paths=%2Fsomething&version=/),
874874
]);
875875
manifestRequests = [];
876876

@@ -884,7 +884,7 @@ test.describe("Fog of War", () => {
884884
await page.waitForSelector("#static");
885885
expect(await app.getHtml("#static")).toMatch("Static");
886886
expect(manifestRequests).toEqual([
887-
expect.stringMatching(/\/__manifest\?p=%2Fstatic&version=/),
887+
expect.stringMatching(/\/__manifest\?paths=%2Fstatic&version=/),
888888
]);
889889
expect(
890890
await page.evaluate(() =>
@@ -956,7 +956,7 @@ test.describe("Fog of War", () => {
956956
await page.waitForSelector("#slug");
957957
expect(await app.getHtml("#slug")).toMatch("Slug: a");
958958
expect(manifestRequests).toEqual([
959-
expect.stringMatching(/\/__manifest\?p=%2Fa&version=/),
959+
expect.stringMatching(/\/__manifest\?paths=%2Fa&version=/),
960960
]);
961961
manifestRequests = [];
962962

@@ -977,7 +977,7 @@ test.describe("Fog of War", () => {
977977
await page.waitForSelector("#slug");
978978
expect(await app.getHtml("#slug")).toMatch("Slug: b");
979979
expect(manifestRequests).toEqual([
980-
expect.stringMatching(/\/__manifest\?p=%2Fb&version=/),
980+
expect.stringMatching(/\/__manifest\?paths=%2Fb&version=/),
981981
]);
982982
});
983983

@@ -1044,7 +1044,7 @@ test.describe("Fog of War", () => {
10441044
await page.waitForSelector("#splat");
10451045
expect(await app.getHtml("#splat")).toMatch("Splat: a");
10461046
expect(manifestRequests).toEqual([
1047-
expect.stringMatching(/\/__manifest\?p=%2Fa&version=/),
1047+
expect.stringMatching(/\/__manifest\?paths=%2Fa&version=/),
10481048
]);
10491049
manifestRequests = [];
10501050

@@ -1065,7 +1065,7 @@ test.describe("Fog of War", () => {
10651065
await page.waitForSelector("#splat");
10661066
expect(await app.getHtml("#splat")).toMatch("Splat: b/c");
10671067
expect(manifestRequests).toEqual([
1068-
expect.stringMatching(/\/__manifest\?p=%2Fb%2Fc&version=/),
1068+
expect.stringMatching(/\/__manifest\?paths=%2Fb%2Fc&version=/),
10691069
]);
10701070
});
10711071

@@ -1137,15 +1137,15 @@ test.describe("Fog of War", () => {
11371137
await app.clickLink("/not/a/path");
11381138
await page.waitForSelector("#error");
11391139
expect(manifestRequests).toEqual([
1140-
expect.stringMatching(/\/__manifest\?p=%2Fnot%2Fa%2Fpath&version=/),
1140+
expect.stringMatching(/\/__manifest\?paths=%2Fnot%2Fa%2Fpath&version=/),
11411141
]);
11421142
manifestRequests = [];
11431143

11441144
// Go to a valid slug route
11451145
await app.clickLink("/something");
11461146
await page.waitForSelector("#slug");
11471147
expect(manifestRequests).toEqual([
1148-
expect.stringMatching(/\/__manifest\?p=%2Fsomething&version=/),
1148+
expect.stringMatching(/\/__manifest\?paths=%2Fsomething&version=/),
11491149
]);
11501150
manifestRequests = [];
11511151

@@ -1233,7 +1233,7 @@ test.describe("Fog of War", () => {
12331233
await new Promise((resolve) => setTimeout(resolve, 250));
12341234
expect(manifestRequests).toEqual([
12351235
expect.stringMatching(
1236-
/\/__manifest\?p=%2F&p=%2Fa&p=%2Fb&version=[a-z0-9]{8}/,
1236+
/\/__manifest\?paths=%2F%2C%2Fa%2C%2Fb&version=[a-z0-9]{8}/,
12371237
),
12381238
]);
12391239
});
@@ -1275,7 +1275,7 @@ test.describe("Fog of War", () => {
12751275
await new Promise((resolve) => setTimeout(resolve, 250));
12761276
expect(manifestRequests).toEqual([
12771277
expect.stringMatching(
1278-
/\/__manifest\?p=%2F&p=%2Fa&p=%2Fb&p=%2Fc&p=%2Fd&p=%2Fe&p=%2Ff&p=%2F/,
1278+
/\/__manifest\?paths=%2F%2C%2Fa%2C%2Fb%2C%2Fc%2C%2Fd%2C%2Fe%2C%2Ff%2C%2Fg/,
12791279
),
12801280
]);
12811281
});
@@ -1439,7 +1439,7 @@ test.describe("Fog of War", () => {
14391439
),
14401440
).toEqual(["root", "routes/_index", "routes/a"]);
14411441
expect(manifestRequests).toEqual([
1442-
expect.stringMatching(/\/custom-manifest\?p=%2F&p=%2Fa&version=/),
1442+
expect.stringMatching(/\/custom-manifest\?paths=%2F%2C%2Fa&version=/),
14431443
]);
14441444
manifestRequests = [];
14451445

@@ -1449,7 +1449,7 @@ test.describe("Fog of War", () => {
14491449
// Wait for eager discovery to kick off
14501450
await new Promise((r) => setTimeout(r, 500));
14511451
expect(manifestRequests).toEqual([
1452-
expect.stringMatching(/\/custom-manifest\?p=%2Fa%2Fb&version=/),
1452+
expect.stringMatching(/\/custom-manifest\?paths=%2Fa%2Fb&version=/),
14531453
]);
14541454

14551455
expect(wrongManifestRequests).toEqual([]);

‎packages/react-router/lib/dom/ssr/fog-of-war.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,7 @@ export async function fetchAndApplyManifestPatches(
222222
// https://issues.chromium.org/issues/331406951
223223
// https://github.com/nodejs/node/issues/51518
224224
const searchParams = new URLSearchParams();
225-
paths.sort().forEach((path) => searchParams.append("p", path));
225+
searchParams.set("paths", paths.sort().join(","));
226226
searchParams.set("version", manifest.version);
227227
let url = new URL(
228228
getManifestPath(manifestPath, basename),

‎packages/react-router/lib/rsc/browser.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -976,7 +976,7 @@ function getManifestUrl(paths: string[]): URL | null {
976976
"",
977977
);
978978
let url = new URL(`${basename}/.manifest`, window.location.origin);
979-
paths.sort().forEach((path) => url.searchParams.append("p", path));
979+
url.searchParams.set("paths", paths.sort().join(","));
980980

981981
return url;
982982
}

‎packages/react-router/lib/rsc/server.rsc.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -472,9 +472,9 @@ async function generateManifestResponse(
472472
temporaryReferences: unknown,
473473
) {
474474
let url = new URL(request.url);
475-
let pathnameParams = url.searchParams.getAll("p");
476-
let pathnames = pathnameParams.length
477-
? pathnameParams
475+
let pathParam = url.searchParams.get("paths");
476+
let pathnames = pathParam
477+
? pathParam.split(",").filter(Boolean)
478478
: [url.pathname.replace(/\.manifest$/, "")];
479479
let routeIds = new Set<string>();
480480
let matchedRoutes = pathnames

‎packages/react-router/lib/server-runtime/server.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -354,7 +354,7 @@ async function handleManifestRequest(
354354

355355
let patches: Record<string, EntryRoute> = {};
356356

357-
if (url.searchParams.has("p")) {
357+
if (url.searchParams.has("paths")) {
358358
let paths = new Set<string>();
359359

360360
// In addition to responding with the patches for the requested paths, we
@@ -365,7 +365,9 @@ async function handleManifestRequest(
365365
// for client side matching if the user routes back up to `/parent`.
366366
// This is the same thing we do on initial load in <Scripts> via
367367
// `getPartialManifest()`
368-
url.searchParams.getAll("p").forEach((path) => {
368+
let pathParam = url.searchParams.get("paths") || "";
369+
let requestedPaths = pathParam.split(",").filter(Boolean);
370+
requestedPaths.forEach((path) => {
369371
if (!path.startsWith("/")) {
370372
path = `/${path}`;
371373
}

0 commit comments

Comments
 (0)