Skip to content

Commit

Permalink
Key the Early Hints off of the asset key rather than request path (#7564
Browse files Browse the repository at this point in the history 
)

* Refactor ETag handling

* Check if-none-match before fulfilling preservation cache

* Skip parsing Early Hints for known empty results

* Key the Early Hints off of the asset key rather than request path

* Fix rebased changesets
  • Loading branch information
@GregBrimble
GregBrimble authored Jan 16, 2025
1 parent fe1e344 commit 147ab7d
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 28 deletions.
5 changes: 5 additions & 0 deletions .changeset/cold-turtles-impress.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@cloudflare/pages-shared": patch
---

fix: Key the Early Hints cache entries off of the asset key rather than the request path
99 changes: 73 additions & 26 deletions packages/pages-shared/__tests__/asset-server/handler.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -435,11 +435,27 @@ describe("asset-server handler", () => {

const findAssetEntryForPath = async (path: string) => {
if (path === "/index.html") {
return "index.html";
return "asset-key-index.html";
}

return null;
};
const fetchAsset = () =>
Promise.resolve(
Object.assign(
new Response(`
<!DOCTYPE html>
<html>
<body>
<link rel="preload" as="image" href="/a.png" />
<link rel="preload" as="image" href="/b.png" />
<link rel="modulepreload" href="lib.js" />
<link rel="preconnect" href="cloudflare.com" />
</body>
</html>`),
{ contentType: "text/html" }
)
);

// Create cache storage to reuse between requests
const { caches } = createCacheStorage();
Expand All @@ -450,22 +466,7 @@ describe("asset-server handler", () => {
metadata,
findAssetEntryForPath,
caches,
fetchAsset: () =>
Promise.resolve(
Object.assign(
new Response(`
<!DOCTYPE html>
<html>
<body>
<link rel="preload" as="image" href="/a.png" />
<link rel="preload" as="image" href="/b.png" />
<link rel="modulepreload" href="lib.js" />
<link rel="preconnect" href="cloudflare.com" />
</body>
</html>`),
{ contentType: "text/html" }
)
),
fetchAsset,
});

const { response, spies } = await getResponse();
Expand All @@ -476,17 +477,20 @@ describe("asset-server handler", () => {
await Promise.all(spies.waitUntil);

const earlyHintsCache = await caches.open(`eh:${deploymentId}`);
const earlyHintsRes = await earlyHintsCache.match("https://example.com/");
const earlyHintsRes = await earlyHintsCache.match(
"https://example.com/asset-key-index.html"
);

if (!earlyHintsRes) {
throw new Error(
"Did not match early hints cache on https://example.com/"
"Did not match early hints cache on https://example.com/asset-key-index.html"
);
}

expect(earlyHintsRes.headers.get("link")).toMatchInlineSnapshot(
`"</a.png>; rel="preload"; as=image, </b.png>; rel="preload"; as=image, <lib.js>; rel="modulepreload", <cloudflare.com>; rel="preconnect""`
);
expect(response.headers.get("link")).toBeNull();

// Do it again, but this time ensure that we didn't write to cache again
const { response: response2, spies: spies2 } = await getResponse();
Expand All @@ -497,17 +501,54 @@ describe("asset-server handler", () => {

await Promise.all(spies2.waitUntil);

const earlyHintsRes2 = await earlyHintsCache.match("https://example.com/");
const earlyHintsRes2 = await earlyHintsCache.match(
"https://example.com/asset-key-index.html"
);

if (!earlyHintsRes2) {
throw new Error(
"Did not match early hints cache on https://example.com/"
"Did not match early hints cache on https://example.com/asset-key-index.html"
);
}

expect(earlyHintsRes2.headers.get("link")).toMatchInlineSnapshot(
`"</a.png>; rel="preload"; as=image, </b.png>; rel="preload"; as=image, <lib.js>; rel="modulepreload", <cloudflare.com>; rel="preconnect""`
);
expect(response2.headers.get("link")).toMatchInlineSnapshot(
`"</a.png>; rel="preload"; as=image, </b.png>; rel="preload"; as=image, <lib.js>; rel="modulepreload", <cloudflare.com>; rel="preconnect""`
);

// Now make sure that requests for other paths which resolve to the same asset share the EH cache result
const { response: response3, spies: spies3 } = await getTestResponse({
request: new Request("https://example.com/foo"),
metadata,
findAssetEntryForPath,
caches,
fetchAsset,
});

expect(response3.status).toBe(200);
// waitUntil should not be called at all (SPA)
expect(spies3.waitUntil.length).toBe(0);

await Promise.all(spies3.waitUntil);

const earlyHintsRes3 = await earlyHintsCache.match(
"https://example.com/asset-key-index.html"
);

if (!earlyHintsRes3) {
throw new Error(
"Did not match early hints cache on https://example.com/asset-key-index.html"
);
}

expect(earlyHintsRes3.headers.get("link")).toMatchInlineSnapshot(
`"</a.png>; rel="preload"; as=image, </b.png>; rel="preload"; as=image, <lib.js>; rel="modulepreload", <cloudflare.com>; rel="preconnect""`
);
expect(response3.headers.get("link")).toMatchInlineSnapshot(
`"</a.png>; rel="preload"; as=image, </b.png>; rel="preload"; as=image, <lib.js>; rel="modulepreload", <cloudflare.com>; rel="preconnect""`
);
});

test("early hints should cache empty link headers", async () => {
Expand All @@ -516,7 +557,7 @@ describe("asset-server handler", () => {

const findAssetEntryForPath = async (path: string) => {
if (path === "/index.html") {
return "index.html";
return "asset-key-index.html";
}

return null;
Expand Down Expand Up @@ -554,15 +595,18 @@ describe("asset-server handler", () => {
await Promise.all(spies.waitUntil);

const earlyHintsCache = await caches.open(`eh:${deploymentId}`);
const earlyHintsRes = await earlyHintsCache.match("https://example.com/");
const earlyHintsRes = await earlyHintsCache.match(
"https://example.com/asset-key-index.html"
);

if (!earlyHintsRes) {
throw new Error(
"Did not match early hints cache on https://example.com/"
"Did not match early hints cache on https://example.com/asset-key-index.html"
);
}

expect(earlyHintsRes.headers.get("link")).toBeNull();
expect(response.headers.get("link")).toBeNull();

// Do it again, but this time ensure that we didn't write to cache again
const { response: response2, spies: spies2 } = await getResponse();
Expand All @@ -573,15 +617,18 @@ describe("asset-server handler", () => {

await Promise.all(spies2.waitUntil);

const earlyHintsRes2 = await earlyHintsCache.match("https://example.com/");
const earlyHintsRes2 = await earlyHintsCache.match(
"https://example.com/asset-key-index.html"
);

if (!earlyHintsRes2) {
throw new Error(
"Did not match early hints cache on https://example.com/"
"Did not match early hints cache on https://example.com/asset-key-index.html"
);
}

expect(earlyHintsRes2.headers.get("link")).toBeNull();
expect(response2.headers.get("link")).toBeNull();
});

test.todo(
Expand Down
6 changes: 4 additions & 2 deletions packages/pages-shared/asset-server/handler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@ export async function generateHandler<

async function attachHeaders(response: Response) {
const existingHeaders = new Headers(response.headers);
const eTag = existingHeaders.get("eTag")?.match(/^"(.*)"$/)?.[1];

const extraHeaders = new Headers({
"access-control-allow-origin": "*",
Expand All @@ -364,13 +365,14 @@ export async function generateHandler<

if (
earlyHintsCache &&
isHTMLContentType(response.headers.get("Content-Type"))
isHTMLContentType(response.headers.get("Content-Type")) &&
eTag
) {
const preEarlyHintsHeaders = new Headers(headers);

// "Early Hints cache entries are keyed by request URI and ignore query strings."
// https://developers.cloudflare.com/cache/about/early-hints/
const earlyHintsCacheKey = `${protocol}//${host}${pathname}`;
const earlyHintsCacheKey = `${protocol}//${host}/${eTag}`;
const earlyHintsResponse =
await earlyHintsCache.match(earlyHintsCacheKey);
if (earlyHintsResponse) {
Expand Down

0 comments on commit 147ab7d

Please sign in to comment.