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

fix: delete unused [site] assets #667

Merged
merged 1 commit into from
Mar 22, 2022
Merged

fix: delete unused [site] assets #667

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
9 changes: 9 additions & 0 deletions .changeset/ten-llamas-boil.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"wrangler": patch
---

fix: delete unused `[site]` assets

We discovered critical issues with the way we expire unused assets with `[site]` (see https://github.com/cloudflare/wrangler2/issues/666, https://github.com/cloudflare/wrangler/issues/2224), that we're going back to the legacy manner of handling unused assets, i.e- deleting unused assets.

Fixes https://github.com/cloudflare/wrangler2/issues/666
221 changes: 81 additions & 140 deletions packages/wrangler/src/__tests__/publish.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import * as TOML from "@iarna/toml";
import { mockAccountId, mockApiToken } from "./helpers/mock-account-id";
import {
createFetchResult,
setMockFetchKVGetValue,
setMockRawResponse,
setMockResponse,
unsetAllMocks,
Expand Down Expand Up @@ -588,6 +587,7 @@ export default{
uploading as assets/file-1.2ca234f380.txt...
reading assets/file-2.txt...
uploading as assets/file-2.5938485188.txt...
↗️ Done syncing assets
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
Expand Down Expand Up @@ -643,6 +643,7 @@ export default{
uploading as assets/file-1.2ca234f380.txt...
reading assets/file-2.txt...
uploading as assets/file-2.5938485188.txt...
↗️ Done syncing assets
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
Expand Down Expand Up @@ -692,6 +693,7 @@ export default{
uploading as assets/file-1.2ca234f380.txt...
reading assets/file-2.txt...
uploading as assets/file-2.5938485188.txt...
↗️ Done syncing assets
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
Expand Down Expand Up @@ -739,6 +741,7 @@ export default{
uploading as assets/file-1.2ca234f380.txt...
reading assets/file-2.txt...
uploading as assets/file-2.5938485188.txt...
↗️ Done syncing assets
Uploaded test-name (some-env) (TIMINGS)
Published test-name (some-env) (TIMINGS)
some-env.test-name.test-sub-domain.workers.dev"
Expand Down Expand Up @@ -787,6 +790,7 @@ export default{
uploading as assets/file-1.2ca234f380.txt...
reading assets/file-2.txt...
uploading as assets/file-2.5938485188.txt...
↗️ Done syncing assets
Uploaded test-name-some-env (TIMINGS)
Published test-name-some-env (TIMINGS)
test-name-some-env.test-sub-domain.workers.dev"
Expand Down Expand Up @@ -830,6 +834,7 @@ export default{
skipping - already uploaded
reading assets/file-2.txt...
uploading as assets/file-2.5938485188.txt...
↗️ Done syncing assets
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
Expand Down Expand Up @@ -868,6 +873,7 @@ export default{
expect(std.out).toMatchInlineSnapshot(`
"reading assets/file-1.txt...
uploading as assets/file-1.2ca234f380.txt...
↗️ Done syncing assets
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
Expand Down Expand Up @@ -906,6 +912,7 @@ export default{
expect(std.out).toMatchInlineSnapshot(`
"reading assets/file-1.txt...
uploading as assets/file-1.2ca234f380.txt...
↗️ Done syncing assets
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
Expand Down Expand Up @@ -945,6 +952,7 @@ export default{
expect(std.out).toMatchInlineSnapshot(`
"reading assets/file-1.txt...
uploading as assets/file-1.2ca234f380.txt...
↗️ Done syncing assets
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
Expand Down Expand Up @@ -984,6 +992,7 @@ export default{
expect(std.out).toMatchInlineSnapshot(`
"reading assets/file-1.txt...
uploading as assets/file-1.2ca234f380.txt...
↗️ Done syncing assets
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
Expand Down Expand Up @@ -1023,6 +1032,7 @@ export default{
expect(std.out).toMatchInlineSnapshot(`
"reading assets/file-1.txt...
uploading as assets/file-1.2ca234f380.txt...
↗️ Done syncing assets
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
Expand Down Expand Up @@ -1062,6 +1072,7 @@ export default{
expect(std.out).toMatchInlineSnapshot(`
"reading assets/file-1.txt...
uploading as assets/file-1.2ca234f380.txt...
↗️ Done syncing assets
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
Expand Down Expand Up @@ -1103,6 +1114,7 @@ export default{
expect(std.out).toMatchInlineSnapshot(`
"reading assets/directory-1/file-1.txt...
uploading as assets/directory-1/file-1.2ca234f380.txt...
↗️ Done syncing assets
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
Expand Down Expand Up @@ -1148,6 +1160,7 @@ export default{
expect(std.out).toMatchInlineSnapshot(`
"reading assets/.well-known/file-2.txt...
uploading as assets/.well-known/file-2.5938485188.txt...
↗️ Done syncing assets
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
Expand Down Expand Up @@ -1240,150 +1253,60 @@ export default{
`);
});

describe("expire unused assets", () => {
it("should expire uploaded assets that aren't included anymore", async () => {
const assets = [
{ filePath: "assets/file-1.txt", content: "Content of file-1" },
{ filePath: "assets/file-2.txt", content: "Content of file-2" },
];
const kvNamespace = {
title: "__test-name-workers_sites_assets",
id: "__test-name-workers_sites_assets-id",
};
writeWranglerToml({
main: "./index.js",
site: {
bucket: "assets",
},
});
writeWorkerSource();
writeAssets(assets);
mockUploadWorkerRequest();
mockSubDomainRequest();
mockListKVNamespacesRequest(kvNamespace);
mockKeyListRequest(kvNamespace.id, [
// Put file-1 in the KV namespace
{ name: "assets/file-1.2ca234f380.txt" },
// As well as a couple from a previous upload
{ name: "assets/file-3.somehash.txt" },
{ name: "assets/file-4.anotherhash.txt" },
]);

// Check that we pull down the values of file-3.txt and file-4.txt
setMockFetchKVGetValue(
"some-account-id",
"__test-name-workers_sites_assets-id",
"assets/file-3.somehash.txt",
Buffer.from("Content of file-3", "utf8").toString("base64")
);
setMockFetchKVGetValue(
"some-account-id",
"__test-name-workers_sites_assets-id",
"assets/file-4.anotherhash.txt",
Buffer.from("Content of file-4", "utf8").toString("base64")
);

// and send them back up
mockUploadAssetsToKVRequest(kvNamespace.id, [
...assets.filter((a) => a.filePath !== "assets/file-1.txt"),
{
filePath: "assets/file-3.txt",
content: "Content of file-3",
// expect to expire this asset 300 seconds from now
expiration_ttl: 300,
},
{
filePath: "assets/file-4.txt",
content: "Content of file-4",
// expect to expire this asset 300 seconds from now
expiration_ttl: 300,
},
]);

await runWrangler("publish");

expect(std.out).toMatchInlineSnapshot(`
"reading assets/file-1.txt...
skipping - already uploaded
reading assets/file-2.txt...
uploading as assets/file-2.5938485188.txt...
expiring unused assets/file-3.somehash.txt...
expiring unused assets/file-4.anotherhash.txt...
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
`);
expect(std.err).toMatchInlineSnapshot(`""`);
it("should delete uploaded assets that aren't included anymore", async () => {
const assets = [
{ filePath: "assets/file-1.txt", content: "Content of file-1" },
{ filePath: "assets/file-2.txt", content: "Content of file-2" },
];
const kvNamespace = {
title: "__test-name-workers_sites_assets",
id: "__test-name-workers_sites_assets-id",
};
writeWranglerToml({
main: "./index.js",
site: {
bucket: "assets",
},
});
writeWorkerSource();
writeAssets(assets);
mockUploadWorkerRequest();
mockSubDomainRequest();
mockListKVNamespacesRequest(kvNamespace);
mockKeyListRequest(kvNamespace.id, [
// Put file-1 in the KV namespace
{ name: "assets/file-1.2ca234f380.txt" },
// As well as a couple from a previous upload
{ name: "assets/file-3.somehash.txt" },
{ name: "assets/file-4.anotherhash.txt" },
]);

it("should not try to expire uploaded assets that are already set to expire", async () => {
const assets = [
{
filePath: "assets/file-1.txt",
content: "Content of file-1",
},
{ filePath: "assets/file-2.txt", content: "Content of file-2" },
];
const kvNamespace = {
title: "__test-name-workers_sites_assets",
id: "__test-name-workers_sites_assets-id",
};
writeWranglerToml({
main: "./index.js",
site: {
bucket: "assets",
},
});
writeWorkerSource();
writeAssets(assets);
mockUploadWorkerRequest();
mockSubDomainRequest();
mockListKVNamespacesRequest(kvNamespace);
// These return with an expiration added on each of them
mockKeyListRequest(kvNamespace.id, [
// Put file-1 in the KV namespace, with an expiration
{ name: "assets/file-1.2ca234f380.txt", expiration: 10000 },
// As well as a couple from a previous upload
// This one's set to expire so it won't be uploaded back
{ name: "assets/file-3.somehash.txt", expiration: 10000 },
// This one will be sent back with an expiration value
{ name: "assets/file-4.anotherhash.txt" },
]);

setMockFetchKVGetValue(
"some-account-id",
"__test-name-workers_sites_assets-id",
"assets/file-4.anotherhash.txt",
Buffer.from("Content of file-4", "utf8").toString("base64")
);
// we upload only file-1.txt
mockUploadAssetsToKVRequest(kvNamespace.id, [
...assets.filter((a) => a.filePath !== "assets/file-1.txt"),
]);

mockUploadAssetsToKVRequest(kvNamespace.id, [
// so it'll re-upload file-1 because it's set to expire
// and file-2 because it hasn't been uploaded yet
...assets,
// and it'll expire file-4 since it's just not included anymore
{
filePath: "assets/file-4.txt",
content: "Content of file-4",
// expect to expire this asset 300 seconds from now
expiration_ttl: 300,
},
]);
// and mark file-3 and file-4 for deletion
mockDeleteUnusedAssetsRequest(kvNamespace.id, [
"assets/file-3.somehash.txt",
"assets/file-4.anotherhash.txt",
]);

await runWrangler("publish");
await runWrangler("publish");

expect(std.out).toMatchInlineSnapshot(`
"reading assets/file-1.txt...
uploading as assets/file-1.2ca234f380.txt...
reading assets/file-2.txt...
uploading as assets/file-2.5938485188.txt...
expiring unused assets/file-4.anotherhash.txt...
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
`);
expect(std.err).toMatchInlineSnapshot(`""`);
});
expect(std.out).toMatchInlineSnapshot(`
"reading assets/file-1.txt...
skipping - already uploaded
reading assets/file-2.txt...
uploading as assets/file-2.5938485188.txt...
deleting assets/file-3.somehash.txt from the asset store...
deleting assets/file-4.anotherhash.txt from the asset store...
↗️ Done syncing assets
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
`);
expect(std.err).toMatchInlineSnapshot(`""`);
});
});

Expand Down Expand Up @@ -2761,3 +2684,21 @@ function mockUploadAssetsToKVRequest(
}
);
}

/** Create a mock handler for thr request that does a bulk delete of unused assets */
function mockDeleteUnusedAssetsRequest(
expectedNamespaceId: string,
assets: string[]
) {
setMockResponse(
"/accounts/:accountId/storage/kv/namespaces/:namespaceId/bulk",
"DELETE",
([_url, accountId, namespaceId], { body }) => {
expect(accountId).toEqual("some-account-id");
expect(namespaceId).toEqual(expectedNamespaceId);
const deletes = JSON.parse(body as string);
expect(assets).toEqual(deletes);
return null;
}
);
}
Loading