Skip to content

fix: allow assets in multiworker setup#7290

Merged
emily-shen merged 6 commits intomainfrom
emily/assets-multiworker
Feb 7, 2025
Merged

fix: allow assets in multiworker setup#7290
emily-shen merged 6 commits intomainfrom
emily/assets-multiworker

Conversation

@emily-shen
Copy link
Copy Markdown
Contributor

@emily-shen emily-shen commented Nov 19, 2024

#7251 will allow running multiple workers in one Miniflare instance. Currently we only set up the router/asset worker for the first worker passed to miniflare, assuming this will be the only user worker. This PR will make sure assets works regardless of how many 'user workers' are passed in and at what point.


  • Tests
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: I don't think there are any existing docs covering this.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Nov 19, 2024

🦋 Changeset detected

Latest commit: 24463da

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

This PR includes changesets to release 5 packages
Name Type
miniflare Patch
wrangler Patch
@cloudflare/pages-shared Patch
@cloudflare/vite-plugin 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

@emily-shen emily-shen added the e2e Run wrangler + vite-plugin e2e tests on a PR label Nov 19, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 19, 2024

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/13205502548/npm-package-wrangler-7290

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13205502548/npm-package-wrangler-7290 dev path/to/script.js
Additional artifacts:

cloudflare-workers-bindings-extension:

wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13205502548/npm-package-cloudflare-workers-bindings-extension-7290 -O ./cloudflare-workers-bindings-extension.0.0.0-v367f0ce0d.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v367f0ce0d.vsix

create-cloudflare:

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

@cloudflare/kv-asset-handler:

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

miniflare:

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

@cloudflare/pages-shared:

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

@cloudflare/unenv-preset:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13205502548/npm-package-cloudflare-unenv-preset-7290

@cloudflare/vite-plugin:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13205502548/npm-package-cloudflare-vite-plugin-7290

@cloudflare/vitest-pool-workers:

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

@cloudflare/workers-editor-shared:

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

@cloudflare/workers-shared:

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

@cloudflare/workflows-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13205502548/npm-package-cloudflare-workflows-shared-7290

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


wrangler@3.107.3 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20250129.0
workerd 1.20250204.0 1.20250204.0
workerd --version 1.20250204.0 2025-02-04

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

@emily-shen emily-shen changed the title feat: allow assets in multiworker setup fix: allow assets in multiworker setup Nov 19, 2024
@emily-shen emily-shen force-pushed the emily/assets-multiworker branch from ab5187d to f123d65 Compare November 28, 2024 14:33
@emily-shen emily-shen marked this pull request as ready for review November 28, 2024 14:45
@emily-shen emily-shen requested a review from a team as a code owner November 28, 2024 14:45
@emily-shen emily-shen marked this pull request as draft December 2, 2024 15:03
@emily-shen emily-shen force-pushed the emily/assets-multiworker branch from e8a729f to e95ee2a Compare January 2, 2025 16:11
@emily-shen emily-shen marked this pull request as ready for review January 6, 2025 11:52
@emily-shen emily-shen force-pushed the emily/assets-multiworker branch 4 times, most recently from a997838 to 2999970 Compare January 6, 2025 16:01
Comment thread packages/miniflare/src/index.ts Outdated
}
})
) {
// if target worker does have assets, set binding to point to router worker for that worker ("assets:router-targetWorkerName"), not the user worker itself ("core:user:targetWorkerName")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that how this works in production? I'd have expected service bindings to bypass the router worker

Copy link
Copy Markdown
Contributor Author

@emily-shen emily-shen Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah production behaviour seriously confused me at first but fetch on a service binding does definitely hit assets, even if it is a binding to a named entrypoint.

  prod (ie expected behaviour)
Simple fetch handler assets / UW
Fetch on default worker entrypoint Assets / UW
RPC on default entrypoint UW only
Fetch on named entrypoint Assets / UW's named entrypoint UW only
RPC on named entrypoint UW only

Edit for posterity:

Fetch on named entrypoint | Assets / UW's named entrypoint

this is a bug! that makes so much more sense!!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add some tests for this? i.e. in https://github.com/cloudflare/workers-sdk/blob/80a83bb4708ea2e3d4b514d3ebc97aa40c14439c/packages/wrangler/e2e/multiworker-dev.test.ts. In particular it would be good to have a test that validates that RPC service bindings bypass Assets

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

47a09dab086217fc873658567cde510f914c9a2a

@emily-shen emily-shen force-pushed the emily/assets-multiworker branch 4 times, most recently from 1728808 to 47a09da Compare February 3, 2025 20:06
Copy link
Copy Markdown
Contributor

@penalosa penalosa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small comments, but overall 🎉

Comment thread packages/miniflare/src/index.ts Outdated
Comment thread packages/miniflare/src/index.ts Outdated
Comment thread packages/miniflare/src/plugins/assets/index.ts Outdated
Comment thread packages/wrangler/e2e/multiworker-dev.test.ts Outdated
@emily-shen emily-shen force-pushed the emily/assets-multiworker branch from 36b787e to 24463da Compare February 7, 2025 18:08
@emily-shen emily-shen merged commit 0c0374c into main Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

e2e Run wrangler + vite-plugin e2e tests on a PR

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants