feat: Create very basic Asset Worker and plumb it into `wrangler dev` #6370

CarmenPopoviciu · 2024-07-30T07:46:55Z

What this PR solves / how to test

[Workers + Assets]
This PR includes work needed to add Assets support for Workers 🚀

Architecture Diagram

Based on the above diagram, this PR does the following:

creates a new package called workers-shared that will host the Asset Server Worker and Router Worker
scaffolds the Asset Server Worker in some very basic form, with basic configuration. Further behaviour implementation will follow in a subsequent PR
does the ground work of plumbing ASW into miniflare

These changes are supported by both wrangler dev and wrangler dev --x-dev-env 💅 ...

Fixes WC-2423

🐙 Please review per commit 🐙

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:
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-07-30T07:46:59Z

🦋 Changeset detected

Latest commit: baa45c0

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	Minor
@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-07-30T12:56:48Z

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/10213550282/npm-package-wrangler-6370

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

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

Or you can use npx with this latest build directly:

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

Additional artifacts:

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

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

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

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

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

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

[email protected] includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240725.0
`workerd`	1.20240725.0	1.20240725.0
`workerd --version`	1.20240725.0	2024-07-25

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

CarmenPopoviciu · 2024-07-31T17:10:23Z

packages/wrangler/scripts/deps.ts

@@ -22,6 +22,7 @@ export const EXTERNAL_DEPENDENCIES = [
 	// and read when we are bundling the worker application
 	"unenv",
 	"workerd/worker.mjs",
+	"@cloudflare/workers-shared/dist",


marking @cloudflare/workers-shared/dist in esbuild as external means that "instead of being bundled, the import will be preserved and will be evaluated at run time instead.". This unfortunately comes with the side effect that @cloudflare/workers-shared/dist won't be considered a dependency, and will therefore be ignored in watch mode. This means that when in dev mode, any changes to the asset-server-worker code (or any code inside @cloudflare/workers-shared for that matter) won't trigger a wrangler re-bundle, which we'd ideally want, since miniflare references the bundled ASW here.

While this will most likely not impact end-users, since they shouldn't be fiddling with workers-shared/ASW anyway, it does create some friction for us as the wrangler team, and our dev experience.

One possible solution I played with, that seemed to do the trick, was to factor the rebuilding of ASW in the wrangler dev process, meaning 👇 (with pnpm -C ../workers-shared run bundle:asset-server --watch being the bit of interest here)

# packages/wrangler/package.json "dev": "pnpm run clean && concurrently -c black,blue --kill-others-on-fail false \"pnpm -C ../workers-shared run bundle:asset-server --watch\" \"pnpm run bundle --watch\" \"pnpm run check:type --watch --preserveWatchOutput\""

But tbh, I'm not sure how I feel abt that, and since I feel like I'd like to have validation of this work before I spend more time on this, I left it out of my PR for the time being

packages/wrangler/src/dev/miniflare.ts

CarmenPopoviciu · 2024-07-31T18:52:55Z

packages/wrangler/src/deployment-bundle/entry.ts

 			file = path.resolve(getBasePath(), "templates/no-op-worker.js");
+		} else if (args.experimentalAssets || config.experimental_assets) {


IMHO this should be a no-op Worker or the User Worker itself, but never the Asset Server Worker (ASW) or the Router Worker (RW). Let me explain...

The entry-point Worker will be bundled by wrangler via esbuild, with some very wrangler-specific esbuild configuration. In the process, wrangler will apply a bunch of middlewares that don't seem relevant to either ASW or RW. wrangler's esbuild configuration was devised for bundling User Workers specifically, and while ASW and RW are Workers themselves, they are external to wrangler as a package, are self contained, and will come with their own bundling opinions and requirements. Therefore conflating bundling between the two packages, without an actual need for it, feels very wrong to me. If we want ASW and RW to be common entities to both wrangler and EWC, and to emulate production in dev as much as possible, ASW & RW should IMO own their own bundling + configuration, and any other devices needed for them to be deployed. To that effect, if wrangler is to have a dependency on these Workers and spin them up in a new Miniflare instance, then it should be on the already bundled versions of these Workers.

This is at least how I see the ideal world, though it remains to be seen if we can pull this off completely. But for now, this is kind of what I'm striving towards.

Happy to hear other thoughts though ^.^

Totally agree. I was originally thinking we'd just import ASW from "@cloudflare/workers-shared"; from within a template and let Wrangler do it's thing, but you're completely correct — ASW2 should get it's own bundling and that is what we should boot up in miniflare, with separation between this and any user worker stuff.

CarmenPopoviciu · 2024-07-31T19:01:59Z

packages/wrangler/src/dev/miniflare.ts

@@ -879,6 +920,9 @@ export async function buildMiniflareOptions(
 					proxy: true,
 				})),
 			},
+			...(config.experimentalAssets


technically, for assets-only Workers, we should create a miniflare instance with just the AssetServerWorker here. Removing the entry Worker however broke things, and unfortunately I was kinds clueless as to why. BUT...I might have had a breakthrough in the meanwhile, I just need a bit more time to verify my assumptions & polish the code. I would like to ask that we don't block this PR on that 🙏

Yup, fine by me. I think we'll eventually get to a place where we're always at least attaching a Router Worker when assets are present, so don't spend too much time getting this running with ASW2 alone.

indeed, that's kind of the architecture diagram I have in my head as well. The same principle will apply tho, since we'll need to figure out how to make the RW the entry point in the "miniflare pipeline". As it stands right now, that entry point seems to always be the entry-point Worker (which basically wraps the User Worker), so I'll need to have that figured out either way. But again, this can be done as a separate task

packages/workers-shared/README.md

fixtures/workers-with-assets/wrangler.toml

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

packages/wrangler/src/api/dev.ts

packages/wrangler/src/api/startDevWorker/ConfigController.ts

packages/wrangler/src/dev.tsx

packages/wrangler/src/dev/miniflare.ts

GregBrimble · 2024-07-31T20:27:37Z

packages/wrangler/src/dev/miniflare.ts

@@ -879,6 +920,9 @@ export async function buildMiniflareOptions(
 					proxy: true,
 				})),
 			},
+			...(config.experimentalAssets


Yup, fine by me. I think we'll eventually get to a place where we're always at least attaching a Router Worker when assets are present, so don't spend too much time getting this running with ASW2 alone.

GregBrimble · 2024-07-31T20:29:48Z

packages/wrangler/src/deployment-bundle/entry.ts

 			file = path.resolve(getBasePath(), "templates/no-op-worker.js");
+		} else if (args.experimentalAssets || config.experimental_assets) {


Totally agree. I was originally thinking we'd just import ASW from "@cloudflare/workers-shared"; from within a template and let Wrangler do it's thing, but you're completely correct — ASW2 should get it's own bundling and that is what we should boot up in miniflare, with separation between this and any user worker stuff.

fixtures/workers-with-assets/README.md

packages/wrangler/src/api/dev.ts

packages/wrangler/src/api/startDevWorker/types.ts

.changeset/orange-icons-build.md

This commit does the ground work needed in order to add Assets support for Workers in `wrangler dev`. Amongst others, it implements the following: - it creates a new package called `workers-shared` that hosts the `Asset Server Worker`, and the `Router Worker`in the future - it scaffolds the `Asset Server Worker` in some very basic form, with basic configuration. Further behaviour implementation will follow in a subsequent PR - it does the ground work of plumbing ASW into Miniflare

…6370) This commit does the ground work needed in order to add Assets support for Workers in `wrangler dev`. Amongst others, it implements the following: - it creates a new package called `workers-shared` that hosts the `Asset Server Worker`, and the `Router Worker`in the future - it scaffolds the `Asset Server Worker` in some very basic form, with basic configuration. Further behaviour implementation will follow in a subsequent PR - it does the ground work of plumbing ASW into Miniflare

CarmenPopoviciu force-pushed the carmen/dev-with-assets branch from 9a4f07b to e695ec6 Compare July 30, 2024 12:54

CarmenPopoviciu force-pushed the carmen/dev-with-assets branch 4 times, most recently from 89da339 to 33c7761 Compare July 31, 2024 12:08

CarmenPopoviciu commented Jul 31, 2024

View reviewed changes

CarmenPopoviciu force-pushed the carmen/dev-with-assets branch 2 times, most recently from 4b56877 to a2d887a Compare July 31, 2024 17:49

CarmenPopoviciu commented Jul 31, 2024

View reviewed changes

packages/wrangler/src/dev/miniflare.ts Outdated Show resolved Hide resolved

CarmenPopoviciu commented Jul 31, 2024

View reviewed changes

packages/wrangler/src/dev/miniflare.ts Outdated Show resolved Hide resolved

CarmenPopoviciu commented Jul 31, 2024

View reviewed changes

CarmenPopoviciu marked this pull request as ready for review July 31, 2024 19:02

CarmenPopoviciu requested a review from a team as a code owner July 31, 2024 19:02

CarmenPopoviciu changed the title ~~[WIP] feat: Create very basic Asset Worker and plumb into wrangler dev~~ [WIP] feat: Create very basic Asset Worker and plumb it into wrangler dev Jul 31, 2024

CarmenPopoviciu changed the title ~~[WIP] feat: Create very basic Asset Worker and plumb it into wrangler dev~~ feat: Create very basic Asset Worker and plumb it into wrangler dev Jul 31, 2024

GregBrimble reviewed Jul 31, 2024

View reviewed changes

packages/workers-shared/README.md Outdated Show resolved Hide resolved

GregBrimble reviewed Jul 31, 2024

View reviewed changes

fixtures/workers-with-assets/wrangler.toml Outdated Show resolved Hide resolved

GregBrimble approved these changes Jul 31, 2024

View reviewed changes

CarmenPopoviciu force-pushed the carmen/dev-with-assets branch 2 times, most recently from 4ddc962 to 4858991 Compare August 1, 2024 12:23

RamIdeas reviewed Aug 1, 2024

View reviewed changes

RamIdeas approved these changes Aug 1, 2024

View reviewed changes

CarmenPopoviciu force-pushed the carmen/dev-with-assets branch from a74abdb to 0ce9eca Compare August 2, 2024 08:10

CarmenPopoviciu added the e2e Run e2e tests on a PR label Aug 2, 2024

andyjessop reviewed Aug 2, 2024

View reviewed changes

.changeset/orange-icons-build.md Outdated Show resolved Hide resolved

CarmenPopoviciu force-pushed the carmen/dev-with-assets branch from 0ce9eca to baa45c0 Compare August 2, 2024 09:46

CarmenPopoviciu merged commit 8a3c6c0 into main Aug 2, 2024
20 checks passed

CarmenPopoviciu deleted the carmen/dev-with-assets branch August 2, 2024 09:52

workers-devprod mentioned this pull request Aug 2, 2024

Version Packages #6400

Merged

edmundhung mentioned this pull request Aug 5, 2024

chore: update package names #6421

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Create very basic Asset Worker and plumb it into `wrangler dev` #6370

feat: Create very basic Asset Worker and plumb it into `wrangler dev` #6370

CarmenPopoviciu commented Jul 30, 2024 •

edited

Loading

changeset-bot bot commented Jul 30, 2024 •

edited

Loading

github-actions bot commented Jul 30, 2024 •

edited

Loading

CarmenPopoviciu Jul 31, 2024

CarmenPopoviciu Jul 31, 2024 •

edited

Loading

GregBrimble Jul 31, 2024

CarmenPopoviciu Jul 31, 2024

GregBrimble Jul 31, 2024

CarmenPopoviciu Aug 1, 2024

GregBrimble Jul 31, 2024

GregBrimble Jul 31, 2024

		file = path.resolve(getBasePath(), "templates/no-op-worker.js");
		} else if (args.experimentalAssets \|\| config.experimental_assets) {

feat: Create very basic Asset Worker and plumb it into wrangler dev #6370

feat: Create very basic Asset Worker and plumb it into wrangler dev #6370

Conversation

CarmenPopoviciu commented Jul 30, 2024 • edited Loading

What this PR solves / how to test

Author has addressed the following

changeset-bot bot commented Jul 30, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Jul 30, 2024 • edited Loading

CarmenPopoviciu Jul 31, 2024

Choose a reason for hiding this comment

CarmenPopoviciu Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

GregBrimble Jul 31, 2024

Choose a reason for hiding this comment

CarmenPopoviciu Jul 31, 2024

Choose a reason for hiding this comment

GregBrimble Jul 31, 2024

Choose a reason for hiding this comment

CarmenPopoviciu Aug 1, 2024

Choose a reason for hiding this comment

GregBrimble Jul 31, 2024

Choose a reason for hiding this comment

GregBrimble Jul 31, 2024

Choose a reason for hiding this comment

feat: Create very basic Asset Worker and plumb it into `wrangler dev` #6370

feat: Create very basic Asset Worker and plumb it into `wrangler dev` #6370

CarmenPopoviciu commented Jul 30, 2024 •

edited

Loading

changeset-bot bot commented Jul 30, 2024 •

edited

Loading

github-actions bot commented Jul 30, 2024 •

edited

Loading

CarmenPopoviciu Jul 31, 2024 •

edited

Loading