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

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

Open
wants to merge 1 commit into
base: early-hints-store-empty-links
Choose a base branch
from
+82 −28
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/cold-turtles-impress.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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}`;
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 could do something like we did for asset preservation v2 where we simultaneously checked the old and new formats to prevent a flood of new cache misses as we roll this out.

const earlyHintsResponse =
await earlyHintsCache.match(earlyHintsCacheKey);
if (earlyHintsResponse) {
Expand Down
Loading