Skip to content
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

implement getBindingsProxy utility #4523

Merged
merged 8 commits into from
Jan 19, 2024
Merged

Conversation

dario-piotrowicz
Copy link
Member

@dario-piotrowicz dario-piotrowicz commented Nov 29, 2023

This PR introduces a new getBindingsProxy that can be used externally to get a proxy to Miniflare bindings that integrate well with wrangler (e.g. with the toml file and the local registry)

You can see a demo of what this would look like here: getBindingsProxy-poc-demo

Note

Initially the utility was going to have two alternative argument APIs, one in which it would accept inline bindings (to use for Pages application) and one in which the bindings would instead be read from the wrangler.toml file (to use for Workers applications).

 We're currently considering if maybe it would be better to have this utility simply always only read the bindings from the wrangler.toml file (it would be a simpler API for users to use and for us to implement and maintain).

I am not sure if both APIs will be implemented or if only the toml one will, regardless I'm scoping this PR only to the toml implementation, since the inline one could be then added afterwords as an additive non-breaking change anyways, so there is really no harm in reducing the PR's scope


To address:

  • implement passing the persist option , also, maybe we should just enable the utility to get the miniflare options and pass them to it as they are?1
  • [ ] Implement utilities to get all the bindings of a certain type (e.g. getD1Bindings, etc...)2
  • [ ] to give maximum flexibility to users, maybe in the return object we should return the miniflare instance itself?1
  • [ ] implement ai fetcher (if possible) to enable workers-ai bindings to be used locally2
  • Add testing for the utility
  • [ ] support reading env variables/secrets from .dev.vars2
  • make sure the utility can also read wrangler.json files (to do here if it's simple or in a followup PR otherwise)
  • add proper tsdocs comments to the utility and its options

Footnotes

  1. The fact that miniflare is used under the hood should be transparent to users, they shouldn't need to be aware of this, especially if/when in the future this utility uses startDevWorker or whatever else layer of abstraction (as discussed with @RamIdeas). 2

  2. Let's get this working with the classic/simple bindings and add things incrementally in followup PRs (incrementally iterating over this in non breaking manners looks to me like the way to go instead of trying to nail every single aspect right out of the gate) 2 3

Copy link

changeset-bot bot commented Nov 29, 2023

🦋 Changeset detected

Latest commit: f3c76bf

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

This PR includes changesets to release 1 package
Name Type
wrangler Minor

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

@dario-piotrowicz dario-piotrowicz changed the title implement getBindingsProxy utility implement getBindingsProxy utility Nov 29, 2023
Copy link
Contributor

github-actions bot commented Nov 29, 2023

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/7586215045/npm-package-wrangler-4523

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7586215045/npm-package-wrangler-4523 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7586215045/npm-package-create-cloudflare-4523 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7586215045/npm-package-miniflare-4523
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7586215045/npm-package-cloudflare-pages-shared-4523

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


wrangler@3.23.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20231218.2
workerd 1.20231218.0 1.20231218.0
workerd --version 1.20231218.0 2023-12-18

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

Copy link

codecov bot commented Nov 29, 2023

Codecov Report

Attention: 63 lines in your changes are missing coverage. Please review.

Comparison is base (222aa4a) 75.66% compared to head (f3c76bf) 71.50%.
Report is 17 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4523      +/-   ##
==========================================
- Coverage   75.66%   71.50%   -4.17%     
==========================================
  Files         243      288      +45     
  Lines       13226    14834    +1608     
  Branches     3396     3729     +333     
==========================================
+ Hits        10008    10607     +599     
- Misses       3218     4227    +1009     
Files Coverage Δ
packages/wrangler/src/api/index.ts 100.00% <100.00%> (ø)
packages/wrangler/src/api/integrations/index.ts 100.00% <100.00%> (ø)
packages/wrangler/src/config/index.ts 89.88% <ø> (ø)
packages/wrangler/src/dev-registry.ts 68.88% <ø> (ø)
packages/wrangler/src/dev.tsx 81.34% <100.00%> (ø)
packages/wrangler/src/dev/miniflare.ts 61.64% <100.00%> (+0.17%) ⬆️
...es/wrangler/src/api/integrations/bindings/index.ts 18.75% <18.75%> (ø)
...wrangler/src/api/integrations/bindings/services.ts 7.50% <7.50%> (ø)

... and 59 files with indirect coverage changes

@irvinebroque
Copy link
Contributor

🚀 docs for this would be here? https://developers.cloudflare.com/workers/wrangler/api/

@dario-piotrowicz
Copy link
Member Author

🚀 docs for this would be here? https://developers.cloudflare.com/workers/wrangler/api/

I hadn't thought of that to be honest 😅

The page you linked makes sense to me 🙂👍 , I am not sure what the plan is for where the startDevWorker related docs, I'd imagine that the docs for getBindingsProxy should be handled in the same way as that

cc. @admah

@admah
Copy link
Contributor

admah commented Jan 8, 2024

@dario-piotrowicz I can put up a docs PR based on the README from that PoC. I do think https://developers.cloudflare.com/workers/wrangler/api/ is the page that that makes the most sense for this documentation.

I'll tag you there and link it to this PR.

@dario-piotrowicz
Copy link
Member Author

@admah, that sounds like a plan!

Thank you very much 🙂

(PS: if you want I am can also open the docs PR, whatever you prefer 🙂)

@dario-piotrowicz dario-piotrowicz marked this pull request as ready for review January 19, 2024 13:22
@dario-piotrowicz dario-piotrowicz requested a review from a team as a code owner January 19, 2024 13:22
@dario-piotrowicz dario-piotrowicz requested a review from a team January 19, 2024 13:22
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Generally looks good to me. Left a few questions and nits. Nothing blocking.
I haven't actually tried this out yet though 😱

.changeset/dull-jobs-deliver.md Outdated Show resolved Hide resolved
packages/wrangler/src/config/index.ts Outdated Show resolved Hide resolved
@dario-piotrowicz dario-piotrowicz merged commit 9f96f28 into main Jan 19, 2024
17 checks passed
@dario-piotrowicz dario-piotrowicz deleted the get-bindings-proxy branch January 19, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants