fix(wrangler): Support switching between static and dynamic Workers

[CarmenPopoviciu]

What this PR solves / how to test

This commit fixes the current behaviour of watch mode for Workers with assets, and adds support for switching between static and dynamic Workers within a single wrangler dev session.

Fixes WC-2721

Author has addressed the following

• Tests
  • TODO (before merge)
  • Tests included
  • Tests not necessary because:
• E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
  • I don't know
  • Required
  • Not required because:
• Changeset (Changeset guidelines)
  • TODO (before merge)
  • Changeset included
  • Changeset not necessary because:
• Public documentation
  • TODO (before merge)
  • Cloudflare docs PR(s):
  • Documentation not necessary because: not documented

[changeset-bot]

🦋 Changeset detected

Latest commit: e6a366d

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

This PR includes changesets to release 2 packages

Name	Type
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

[CarmenPopoviciu]

technically, we could support this for all Workers, not just Workers with assets. Is this something we would like to do?

[CarmenPopoviciu]

after internal discussions, the answer is yes. wrangler dev --x-dev-env already supports this, no reason why wrangler dev shouldn't

[github-actions]

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/10958433741/npm-package-wrangler-6775

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

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

Or you can use npx with this latest build directly:

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

Additional artifacts:

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

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

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

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

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

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

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

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.20240909.4
workerd	1.20240909.0	1.20240909.0
workerd --version	1.20240909.0	2024-09-09

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

[penalosa]

LGTM. Does this work with the --x-dev-env flow as well?

[CarmenPopoviciu]

LGTM. Does this work with the --x-dev-env flow as well?

it does yes! Tested manually, but also covered by our e2e tests

[RamIdeas]

I assume this bug does not exist with --x-dev-env because config+args are re-resolved correctly each time config is updated?

[CarmenPopoviciu]

I assume this bug does not exist with --x-dev-env because config+args are re-resolved correctly each time config is updated?

indeed. I didn't have to fix anything for -x-dev-env. Can't wait for that to become the default

[GregBrimble]

In my testing, I see the following behaviors work:

Switching between

• Static to both
• Both to Worker
• Worker to both
• Worker to static
• Static to Worker

---

RPC to a regular Worker

• default entrypoint RPC
• default entrypoint RPC fetch
• named entrypoint RPC
• named entrypoint RPC fetch

---

RPC to a Worker with both a script and assets

• default entrypoint RPC
• default entrypoint RPC fetch asset
• default entrypoint RPC fetch Worker
• named entrypoint RPC
• named entrypoint RPC fetch asset
• named entrypoint RPC fetch Worker

---

RPC to a Worker with just assets

• default entrypoint RPC fetch asset
• named entrypoint RPC fetch asset

[CarmenPopoviciu]

oh boy...this is def more complex than what I thought the expectation was here. My understanding was that this should support switching between assets only and W + A. I'm surprised tbh it works for anything other than. If. we want to support such a wide range of scenarios, I'll need to dbl check with the team to get a feel of whether this is smth we do want to support.

[GregBrimble]

We don't need RPC for launch, (other than just ensuring we don't break "RPC to a regular Worker"). But getting Worker to both and Worker to static would be nice if possible.

[GregBrimble]

I also think this PR improves the current state. It fixes going from "Static to both" and "Static to Worker", so feel free to merge this as-is and we can address "Worker to both" and "Worker to static" in a follow-up ticket.

[CarmenPopoviciu]

kk. I'll keep that in mind, but will bring this to the team anyway just so we have a clear picture of the general opinion here

[petebacondarwin]

I haven't thought this through but it feels like we could just say that any change to wrangler.toml causes a complete restart of the dev server and that would naturally pick up the change to asset config and worker main config? Avoiding any need to special case this?

[CarmenPopoviciu]

technically yes, and I think that --x-dev-env gives us the behaviour we're looking for, tho I'd need to do more proper testing to confirm. However, the default wrangler dev, which uses the Dev React component, needs to re-render that component on every wrangler.toml change, which in turn will trigger the dev server to restart. The main entrypoint is passed in as a prop to <Dev entry={entry}> (where const entry = getEntry()), so unless we re-compute it on a config file change, any main changes won't be reflected even with a dev server restart. So, unless I'm completely misunderstanding the code, we'd need to call getEntry() on every toml change to ensure main entry point changes are reflected in what dev server serves

alternatively, we could do smth like <Dev entry={getEntry()} maybe? I doesn't like getEntry() is a computationally expensive fn, so I don't think there'd be any performance concerns

---- EDIT -----

and I think that --x-dev-env gives us the behaviour we're looking for

yes it does, so no reason why wrangler dev shouldn't

alternatively, we could do smth like <Dev entry={getEntry()}

no we cannot, because getEntry() can throw, and without a try--catch the dev session would terminate, which is smth we don't want. Therefore keeping getEntry() in the chockidar change handler, which already wraps these fn calls in a try...catch block, is the way to go

[CarmenPopoviciu]

after discussing with the team, and doing some more testing, I'm going to go head and capture your suggestions @GregBrimble in this PR.

I've addressed your feedback in f6b33b7.

Please have a look and let me know how that test list is looking. I've tested a bunch of permutations, except RPC.

With these changes, we are bringing the wrangler dev and wrangler dev --x-dev-env experience on par, AFAICT at least 🚀

[GregBrimble]

Updated my test sheet, and sure enough, everything works great! Thanks @CarmenPopoviciu !