-
Notifications
You must be signed in to change notification settings - Fork 564
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
[miniflare] feat: add support for wrapped bindings #4348
Conversation
🦋 Changeset detectedLatest commit: 96553f7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
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/6848503046/npm-package-wrangler-4348 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6848503046/npm-package-wrangler-4348 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6848503046/npm-package-wrangler-4348 dev path/to/script.js Additional artifacts:npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6848503046/npm-package-miniflare-4348 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6848503046/npm-package-cloudflare-pages-shared-4348 Note that these links will no longer work once the GitHub Actions artifact expires.
| Please ensure constraints are pinned, and |
Codecov Report
@@ Coverage Diff @@
## main #4348 +/- ##
==========================================
+ Coverage 75.43% 75.46% +0.03%
==========================================
Files 225 225
Lines 12449 12449
Branches 3227 3227
==========================================
+ Hits 9391 9395 +4
+ Misses 3058 3054 -4 |
_ but depend on internal bindings that the public version of workerd doesn't support_ If everyone put the API surface of these implementations in workerd in some form (directly, as a submodule, etc.), and the only difference between workerd and EW was the backend — would that simplify? i.e. is there a world where the developer-facing API surface area isn't ever reimplmented — and we're purely swapping out the backend that the API talks to in different environments? @mikea since relates to wrapped bindings |
@irvinebroque definitely! If we could move the JSG parts of the |
This usually applies to pre-release development. teams often prefer to develop internally and release to workerd once it has been announced. There's also an issue of internal-only APIs: right now there's no way to add them to wrangler, this might help us use the same external version of workerd but with internal bindings configured. |
We should push to build any API surface area more out in the open, just
gate behind experimental flags.
Supportive of this change just hunting for ways to push ourselves to build
in the open and not have divergent implementations.
…On Mon, Nov 6, 2023 at 1:37 PM Mike Aizatsky ***@***.***> wrote:
i.e. is there a world where the developer-facing API surface area isn't
ever reimplmented
This usually applies to pre-release development. teams often prefer to
develop internally and release to workerd once it has been announced.
There's also an issue of internal-only APIs: right now there's no way to
add them to wrangler, this might help us use the same external version of
workerd but with internal bindings configured.
—
Reply to this email directly, view it on GitHub
<#4348 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJKHWHGW72P3GRYSE7DB4TYDE37HAVCNFSM6AAAAAA67QIMOKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJWGE3TINBSGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This change adds a new `wrappedBindings` worker option for configuring `workerd`'s wrapped bindings. These allow custom bindings to be written as JavaScript functions accepting an `env` parameter of "inner bindings" and returning the value to bind. The `wrappedBindings` option maps binding names to designators. The worker defined by the designator is used for the wrapped binding's source and inner bindings. The worker becomes un-routable, and cannot be used for service or Durable Object bindings. Using a worker allows us to piggyback on all of Miniflare's existing bindings. For example, the wrapped binding worker can declare a function-valued service binding for accessing Node.js state. JSON bindings can be specified with the designator to override specific bindings.
18cd423
to
96553f7
Compare
@@ -315,6 +315,114 @@ parameter in module format Workers. | |||
handler. This allows you to access data and functions defined in Node.js | |||
from your Worker. | |||
|
|||
<!--prettier-ignore-start--> | |||
|
|||
- `wrappedBindings?: Record<string, string | { scriptName: string, entrypoint?: string, bindings?: Record<string, Json> }>` |
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.
Unless scriptName
is to match the capnp implementation, would something like workerName
fit better here, as we try and move away from the script
language?
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.
Potentially yeah. This was meant to match the Durable Objects API which uses script name for a similar purpose.
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.
That's fair—better to match the API then 👍
Fixes #4358
What this PR solves / how to test:
This change adds a new
wrappedBindings
worker option for configuringworkerd
's wrapped bindings. These allow custom bindings to be written as JavaScript functions accepting anenv
parameter of "inner bindings" and returning the value to bind.This unblocks internal teams that would like to migrate to Miniflare 3, but depend on internal bindings that the public version of
workerd
doesn't support. Wrapped bindings allow these teams to write mock implementations.The
wrappedBindings
option maps binding names to designators. The worker defined by the designator is used for the wrapped binding's source and inner bindings. The worker becomes un-routable, and cannot be used for service or Durable Object bindings. Using a worker allows us to piggyback on all of Miniflare's existing bindings. For example, the wrapped binding worker can declare a function-valued service binding for accessing Node.js state. JSON bindings can be specified with the designator to override specific bindings.Author has addressed the following:
Note for PR author:
We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label
highlight pr review
so future reviewers can take inspiration and learn from it.