-
Notifications
You must be signed in to change notification settings - Fork 845
Implement support for ctx.props. #8640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
In `wrangler.toml` you can declare a service binding with props: ``` [[services]] binding = "MY_SERVICE" service = "some-worker" props = { foo = 123, bar = 456 } ``` Unfortunately, it currently doesn't work when wrangler is running different workers in different processes, as `props` cannot be sent to a remote `workerd`. Fixing this would require `workerd` changes, but since we are moving away from the multi-process mode anyway, perhaps it's not worth it. Note: I used Claude Code to find my way around the codebase. It wrote most of the PR. It was not able to figure out that changes were needed under `packages/miniflare`, and I myself had a pretty hard time figuring out where to edit.
🦋 Changeset detectedLatest commit: 0078cdc The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
It's not immediately obvious to me what this "changeset" thing is about (the links don't help). Can someone just push whatever is needed into this branch? Thanks. |
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/14012929496/npm-package-wrangler-8640 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/8640/npm-package-wrangler-8640 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14012929496/npm-package-wrangler-8640 dev path/to/script.js Additional artifacts:cloudflare-workers-bindings-extension: wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14012929496/npm-package-cloudflare-workers-bindings-extension-8640 -O ./cloudflare-workers-bindings-extension.0.0.0-v5a1585b22.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v5a1585b22.vsix create-cloudflare: npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14012929496/npm-package-create-cloudflare-8640 --no-auto-update @cloudflare/kv-asset-handler: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14012929496/npm-package-cloudflare-kv-asset-handler-8640 miniflare: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14012929496/npm-package-miniflare-8640 @cloudflare/pages-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14012929496/npm-package-cloudflare-pages-shared-8640 @cloudflare/unenv-preset: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14012929496/npm-package-cloudflare-unenv-preset-8640 @cloudflare/vite-plugin: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14012929496/npm-package-cloudflare-vite-plugin-8640 @cloudflare/vitest-pool-workers: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14012929496/npm-package-cloudflare-vitest-pool-workers-8640 @cloudflare/workers-editor-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14012929496/npm-package-cloudflare-workers-editor-shared-8640 @cloudflare/workers-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14012929496/npm-package-cloudflare-workers-shared-8640 @cloudflare/workflows-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14012929496/npm-package-cloudflare-workflows-shared-8640 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
I didn't answer the Wrangler v3 backport question because I have no idea what it's about and there doesn't seem to be any explainer in the contributing guide. |
I'd say the back port is not necessary because this is a new feature. The back port is just about our maintenance commitment to Wrangler v3 and copying over any bug fixes. Changesets are the way we auto-generate release notes and bump versions of the packages here in workers-sdk. You can run |
```toml | ||
[[services]] | ||
binding = "MY_SERVICE" | ||
service = "some-worker" | ||
props = { foo = 123, bar = "value" } | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in the wrangler.jsonc
format, if possible
@@ -251,6 +251,7 @@ function getCustomServiceDesignator( | |||
): ServiceDesignator { | |||
let serviceName: string; | |||
let entrypoint: string | undefined; | |||
let props: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe Record<string, unknown> | undefined
?
@@ -278,7 +280,7 @@ function getCustomServiceDesignator( | |||
// Regular user worker | |||
serviceName = getUserServiceName(service); | |||
} | |||
return { name: serviceName, entrypoint }; | |||
return { name: serviceName, entrypoint, ...(props && { props }) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this conditional is needed—passing props
should be fine
export const ServiceDesignatorSchema = z.union([ | ||
z.string(), | ||
z.literal(kCurrentWorker), | ||
z.object({ | ||
name: z.union([z.string(), z.literal(kCurrentWorker)]), | ||
entrypoint: z.ostring(), | ||
props: PropsSchema.optional(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be a separate schema, it can be inline.
export const PropsSchema = z.object({ | ||
json: z.string(), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this schema correct? It seems to be saying that props are a string—should they not be an object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an object that has been JSON-encoded, so it's a string.
This schema matches what the workerd config wants. But I suppose this layer doesn't necessarily have to match workerd config. We could keep it as an arbitrary object at this layer, and JSON-encode it only when building the final workerd config at a lower layer.
I don't really have an opinion on this, but the current approach seems to work fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see in a comment below you prefer passing the raw object down further, ok.
// TODO: We currently test `ctx.props` support with the worker calling | ||
// back to itself, not to another worker, because the multi-process | ||
// dev environment doesn't support ctx.props, so only self-bindings | ||
// actually work. When we move to the single-process dev approach, | ||
// `ctx.props` should then just work cross-worker, and we should update | ||
// this test to test that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct. This test file is for the single process dev approach, and so multi-worker props should work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, perhaps I got confused because I initially had a different bug that prevented props from getting through to workerd config, and I thought I was hitting this problem. I'll try changing it back to cross-worker.
props: service.props | ||
? { json: JSON.stringify(service.props) } | ||
: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the right place to do this. IMO props should be passed as an object to Miniflare and then stringified before they're sent to workerd
Fixes #8558
In
wrangler.toml
you can declare a service binding with props:Unfortunately, it currently doesn't work when wrangler is running different workers in different processes, as
props
cannot be sent to a remoteworkerd
. Fixing this would requireworkerd
changes, but since we are moving away from the multi-process mode anyway, perhaps it's not worth it.Note: I used Claude Code to find my way around the codebase. It wrote most of the PR. It was not able to figure out that changes were needed under
packages/miniflare
, and I myself had a pretty hard time figuring out where to edit.