[Miniflare 3] Add support for routing to multiple Workers #520
Conversation
|
mrbbot
changed the title
This change ports Miniflare 2's `mounts` router to Miniflare 3. This attempts to replicate the logic of the `route(s)` field in the Wrangler configuration file: https://developers.cloudflare.com/workers/platform/triggers/routes/#matching-behavior Internally, this is implemented by binding all routable workers as service bindings in the entry service. The first worker is always bound as a "fallback", in case no routes match. Validation has been added to ensure we a) have a fallback, b) don't have workers with duplicate names that would cause bindings with the same name, and c) all routable/fallback workers have code so they actually get added as `workerd` services.
mrbbot
force-pushed
the
tre-multi-worker-router
branch
from
8ecc46f to
9229b30
Compare
| @@ -119,6 +123,19 @@ function validateOptions( | |||
| } | |||
There was a problem hiding this comment.
The // @ts-expect-errors appear to be because PluginSharedOptions has readonly keys, which can be fixed (and the errors removed) by removing the as const in packages/tre/src/plugins/index.ts. Would it be possible to make that change, or would it break something else? If it's not possible, could the // @ts-expect-error comments be updated?
There was a problem hiding this comment.
Hmmm, I think removing the as const should be ok. It looks like that still keeps the specific Zod types so options inference/completions still work. Will make that change. 👍
There was a problem hiding this comment.
839b02c (unfortunately the change to enforce code means CoreOptionsSchema has required options that aren't satisfied by the other options types, so we still need some // @ts-expect-errors)
packages/tre/src/index.ts
Outdated
| const allRoutes = new Map<string, string[]>(); | ||
| for (const workerOpts of allWorkerOpts) { | ||
| if (workerOpts.core.routes !== undefined) { | ||
| allRoutes.set(workerOpts.core.name ?? "", workerOpts.core.routes); |
There was a problem hiding this comment.
If there are multiple unnamed workers, will this overwrite some routes?
There was a problem hiding this comment.
I wonder if worker names should be required
There was a problem hiding this comment.
It would, but we've validate names are unique beforehand. Perhaps an assertion that allRoutes doesn't contain the key would be a good idea?
I wonder if worker names should be required
In the single worker case, I don't think they should be. I do agree with you later on though that some of the error messages should be better in this case.
| fallbackWorkerName: string | undefined; | ||
| loopbackPort: number; | ||
| } | ||
| export function getGlobalServices({ |
There was a problem hiding this comment.
This feels like it probably shouldn't be in the core plugin?
There was a problem hiding this comment.
Hmmm, maybe. That might help with the moving CORE_PLUGIN_NAME issue you raised later on too. Will move it out into plugins/shared/services.ts or something. 👍
| throw new MiniflareCoreError( | ||
| "ERR_ROUTABLE_NO_SCRIPT", | ||
| `Worker [${workerIndex}] ${ | ||
| options.name === undefined ? "" : `("${options.name}") ` | ||
| }must have code defined as it's routable or the fallback` | ||
| ); |
There was a problem hiding this comment.
Why can any workers have no code? What does it mean for a worker to not have code defined?
There was a problem hiding this comment.
Hmmm, I suppose that doesn't really make sense, since we don't expose workered's other service types (disk, network, etc) outside of serviceBindings. Will make code required for all Workers. 👍
| const A_MORE_SPECIFIC = -1; | ||
| const B_MORE_SPECIFIC = 1; | ||
|
|
||
| export function parseRoutes(allRoutes: Map<string, string[]>): WorkerRoute[] { |
There was a problem hiding this comment.
I imagine there's code to do this in EWC somewhere—have you checked the logic against that? (I'm assuming this is copied from Miniflare 2)
There was a problem hiding this comment.
I haven't, you're correct this was copied from Miniflare 2, when I didn't have access to this. Will see if I can find it. 👍
There was a problem hiding this comment.
(and a629360 to fixup test)
It doesn't really make sense to have Workers without code. This change updates our `zod` schemas to encode this requirement.
Assert names unique when collecting routes
Move `CORE_PLUGIN_NAME` back to `core`
Add specific error message when defining multiple unnamed workers
Use `Map` when de-duping services
Extract out common plugin/namespace/persist Worker into function
Use same specificity calculation for routes as internal service
This change ports Miniflare 2's
mountsrouter to Miniflare 3. This attempts to replicate the logic of theroute(s)field in the Wrangler configuration file: https://developers.cloudflare.com/workers/platform/triggers/routes/#matching-behaviorInternally, this is implemented by binding all routable workers as service bindings in the entry service. The first worker is always bound as a "fallback", in case no routes match. Validation has been added to ensure we a) have a fallback, b) don't have workers with duplicate names that would cause bindings with the same name, and c) all routable/fallback workers have code so they actually get added as
workerdservices.I've also split some of the core plugin services into "global services". These need to be constructed from a different set of options to the regular plugin services, and relying on the de-duping behaviour here was a bit silly.