feat(workers-shared): Add Asset Server Worker behaviour by CarmenPopoviciu · Pull Request #6539 · cloudflare/workers-sdk

CarmenPopoviciu · 2024-08-20T20:03:49Z

What this PR solves / how to test

[Workers + Assets]

This PR adds a proper-ish implementation of a very un-opinionated Asset Server Worker. Proper-ish because it handles the essential things needed for ASW to work, but I expect some other details we'll be needed as Workers + Assets work progresses further.

Fixes WC-2434

Author has addressed the following

Tests
- TODO (before merge)
- Included
- Not necessary because:
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required / Maybe required
- Not required because: no e2e tests capture ASW2
Changeset (Changeset guidelines)
- TODO (before merge)
- Included
- Not necessary because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Not necessary because: we're not there yet

changeset-bot · 2024-08-20T20:03:52Z

🦋 Changeset detected

Latest commit: c0bcf8d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@cloudflare/workers-shared	Minor
wrangler	Patch
@cloudflare/vitest-pool-workers	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-08-20T20:06:15Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10523785777/npm-package-wrangler-6539

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6539/npm-package-wrangler-6539

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10523785777/npm-package-wrangler-6539 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10523785777/npm-package-create-cloudflare-6539 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10523785777/npm-package-cloudflare-kv-asset-handler-6539

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10523785777/npm-package-miniflare-6539

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10523785777/npm-package-cloudflare-pages-shared-6539

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10523785777/npm-package-cloudflare-vitest-pool-workers-6539

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10523785777/npm-package-cloudflare-workers-editor-shared-6539

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10523785777/npm-package-cloudflare-workers-shared-6539

Note that these links will no longer work once the GitHub Actions artifact expires.

wrangler@3.72.2 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240821.0
`workerd`	1.20240821.1	1.20240821.1
`workerd --version`	1.20240821.1	2024-08-21

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

packages/workers-shared/asset-server-worker/src/utils/headers.ts

GregBrimble

Approved with additional small pieces here: #6542

packages/workers-shared/asset-server-worker/src/global.d.ts

CarmenPopoviciu · 2024-08-21T07:25:12Z

@GregBrimble thanks for reviewing this. I've incorporated the changes you suggested in this commit

petebacondarwin

Looking good. I left a bunch of comments.
I'd really really like to see some tests of this worker.

fixtures/workers-with-assets/tests/index.test.ts

packages/workers-shared/asset-server-worker/src/global.d.ts

packages/workers-shared/asset-server-worker/wrangler.toml

packages/wrangler/src/dev/miniflare.ts

petebacondarwin · 2024-08-22T09:43:35Z

packages/workers-shared/package.json

 	"scripts": {
 		"build": "pnpm run clean && pnpm run bundle:asset-server:prod",
-		"bundle:asset-server": "esbuild asset-server-worker/src/index.ts --format=esm --bundle --outfile=dist/asset-server-worker.mjs --sourcemap=external",
+		"bundle:asset-server": "esbuild asset-server-worker/src/index.ts --format=esm --bundle --outfile=dist/asset-server-worker.mjs --sourcemap=external --external:cloudflare:*",


I wonder if we could use wrangler deploy --dry-run --outdir=dist/asset-worker.mjs to do this so that we don't have to keep the esbuild settings in sync?

packages/workers-shared/asset-server-worker/src/index.ts

petebacondarwin · 2024-08-22T09:49:14Z

packages/workers-shared/asset-server-worker/src/index.ts

+			assetEntry
+		);
+		console.log(assetResponse);
+		if (!assetResponse || !assetResponse.value) {


It feels like assetResponse.value being falsy is a programming error on our part?

technically this should be just a sanity check, but felt like we should check nonetheless. I'll ask the EWC folks if we want to throw a different error here. IMO a 500 sounds like the right thing to return

packages/workers-shared/asset-server-worker/src/responses.ts

CarmenPopoviciu · 2024-08-23T08:10:45Z

packages/miniflare/src/workers/kv/assets.worker.ts

+		const newResponse = new Response(response.body, response);
+		// ensure the runtime will return the metadata we need
+		newResponse.headers.append(
+			"cf-kv-metadata",


see https://github.com/cloudflare/workerd/blob/a20cb7eb98309b0cd38be1c294dcdf894308a2d1/src/workerd/api/kv.c%2B%2B#L218-L220

packages/workers-shared/asset-server-worker/src/index.ts

CarmenPopoviciu · 2024-08-23T08:22:39Z

packages/workers-shared/vitest.config.mts

@@ -0,0 +1,12 @@
+import { defineProject, mergeConfig } from "vitest/config";


I'm no vite expert so I would appreciate a sanity check here on this config 🙏

packages/workers-shared/tsconfig.json

packages/workers-shared/asset-server-worker/src/responses.ts

packages/workers-shared/asset-server-worker/src/utils/headers.ts

packages/workers-shared/tsconfig.json

emily-shen · 2024-08-23T09:56:38Z

packages/workers-shared/asset-server-worker/src/assets-manifest.ts

 const TAIL_SIZE = 8;
 const ENTRY_SIZE = PATH_HASH_SIZE + CONTENT_HASH_SIZE + TAIL_SIZE;

+export type AssetEntry = {


I don't think this ends up being used anywhere

oh right. must be some leftover from all the iterations 😅 . will remove as part of WC-2597

emily-shen · 2024-08-23T10:01:42Z

packages/workers-shared/asset-server-worker/src/index.ts

+		let { pathname } = url;
+
+		const assetsManifest = new AssetsManifest(this.env.ASSETS_MANIFEST);
+		pathname = globalThis.decodeURIComponent(pathname);


not sure if this should be decoded because the assetManifest uses encoded pathnames?
Might be worth adding a test for url encoding with a funky pathname. Not high priority though.

ok, so funky path names def don't work, with or without this decoding. Let's have a look together, and I'll add the proper fix/tests in a follow-up PR. Tracking in WC-2597

packages/workers-shared/asset-server-worker/src/responses.ts

andyjessop

Happy with this - very nice work!

CarmenPopoviciu requested a review from a team as a code owner August 20, 2024 20:03

CarmenPopoviciu force-pushed the carmen/asw2 branch 2 times, most recently from e2dd4eb to ad3a948 Compare August 20, 2024 20:30

CarmenPopoviciu changed the title ~~feat(workers-shared): Add Assets Server Worker behaviour~~ feat(workers-shared): Add Asset Server Worker behaviour Aug 20, 2024

GregBrimble reviewed Aug 21, 2024

View reviewed changes

packages/workers-shared/asset-server-worker/src/utils/headers.ts Show resolved Hide resolved

GregBrimble approved these changes Aug 21, 2024

View reviewed changes

packages/workers-shared/asset-server-worker/src/global.d.ts Outdated Show resolved Hide resolved

CarmenPopoviciu force-pushed the carmen/asw2 branch from db338a2 to 5b7156f Compare August 21, 2024 07:26

petebacondarwin added the e2e Run wrangler + vite-plugin e2e tests on a PR label Aug 22, 2024

petebacondarwin reviewed Aug 22, 2024

View reviewed changes

CarmenPopoviciu force-pushed the carmen/asw2 branch 2 times, most recently from 827b4fc to e11c3ef Compare August 22, 2024 19:02

CarmenPopoviciu added 2 commits August 22, 2024 21:05

feat(workers-shared): Add Assets Server Worker behaviour

2b3ebad

PR feedback

01ef430

CarmenPopoviciu force-pushed the carmen/asw2 branch 2 times, most recently from f395487 to 25c0ce4 Compare August 22, 2024 19:26

CarmenPopoviciu removed the e2e Run wrangler + vite-plugin e2e tests on a PR label Aug 22, 2024

PR feedback 2

4f7acde

CarmenPopoviciu force-pushed the carmen/asw2 branch 4 times, most recently from cb8fcab to eed70a0 Compare August 23, 2024 01:23

make metadata work

786c1a7

CarmenPopoviciu force-pushed the carmen/asw2 branch from eed70a0 to 786c1a7 Compare August 23, 2024 08:04

CarmenPopoviciu commented Aug 23, 2024

View reviewed changes

packages/workers-shared/asset-server-worker/src/index.ts Show resolved Hide resolved

CarmenPopoviciu commented Aug 23, 2024

View reviewed changes

packages/workers-shared/tsconfig.json Outdated Show resolved Hide resolved

test method not allowed

98ab8e4

andyjessop reviewed Aug 23, 2024

View reviewed changes

packages/workers-shared/asset-server-worker/src/responses.ts Show resolved Hide resolved

packages/workers-shared/asset-server-worker/src/utils/headers.ts Show resolved Hide resolved

packages/workers-shared/tsconfig.json Outdated Show resolved Hide resolved

PR feedback 3

c0bcf8d

CarmenPopoviciu force-pushed the carmen/asw2 branch from 1154d63 to c0bcf8d Compare August 23, 2024 10:00

emily-shen reviewed Aug 23, 2024

View reviewed changes

andyjessop approved these changes Aug 23, 2024

View reviewed changes

CarmenPopoviciu added the e2e Run wrangler + vite-plugin e2e tests on a PR label Aug 23, 2024

CarmenPopoviciu merged commit 6c057d1 into main Aug 23, 2024

CarmenPopoviciu deleted the carmen/asw2 branch August 23, 2024 10:42

workers-devprod mentioned this pull request Aug 23, 2024

Version Packages #6567

Merged

		@@ -0,0 +1,12 @@
		import { defineProject, mergeConfig } from "vitest/config";

Conversation

CarmenPopoviciu commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR solves / how to test

Author has addressed the following

Uh oh!

changeset-bot bot commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

GregBrimble left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CarmenPopoviciu commented Aug 21, 2024

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petebacondarwin Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petebacondarwin Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

CarmenPopoviciu Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CarmenPopoviciu Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CarmenPopoviciu Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emily-shen Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

CarmenPopoviciu Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emily-shen Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

CarmenPopoviciu Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andyjessop left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

CarmenPopoviciu commented Aug 20, 2024 •

edited

Loading

changeset-bot bot commented Aug 20, 2024 •

edited

Loading

github-actions bot commented Aug 20, 2024 •

edited

Loading

CarmenPopoviciu Aug 23, 2024 •

edited

Loading

CarmenPopoviciu Aug 23, 2024 •

edited

Loading