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

add bindingNames to getPlatformProxy result #5229

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dario-piotrowicz
Copy link
Member

What this PR solves / how to test

This PR adds a new bindingNames field in the getPlatformProxy result that users can use to get the names of their bindings grouped by type.

For example they can use it to get all the bindings of a specific type in the following way:

const { env, bindingNames } = getPlatformProxy();

const allKvNames = bindingNames.kv; // ['MY_KV', 'MY_KV_1']
const allR2Names = bindingNames.r2; // ['MY_R2']
// ...

const extract = (names; string[]) => Object.entries(env).map(
   [key, binding]) => names.includes[key] ? binding ; null
).filter(Boolean);
const allKvs = extract(allKvNames);
const allR2s = extract(allR2Names);
// ...

Notes:

  • this uses duck typing checks to discern what a binding type is, this checking is used only for the getPlatformProxy utility, but in the future if beneficial it could be exposed to users as a generic (also usable in workerd) utility?
  • another check that could be performed is using the Miniflare custom inspector like so:
    import { inspect, } from "node:util";
    
    const inspectStr = inspect(env.MY_KV);
    const match = inspectStr.match(/ProxyStub \{ name: '(.*?)', poisoned: (?:false|true) \}/);
    const name = match[1]; // 'KvNamespace'
    this would be much simpler but much more brittle and it would only work with the Miniflare magic proxies
  • I implemented bindingNames which only returns the names because I found this to be the simplest/cleanest
    way not to have issues with types, alternatively I could have had something like bindingsByType or groupedEnv,
    etc... but with such solutions they binding types would come from a specific @cloudflare/workers-types entrypoint
    which might not be what the user wants/expects...

fixes #4792

Author has addressed the following

Copy link

changeset-bot bot commented Mar 12, 2024

⚠️ No Changeset found

Latest commit: 024fd7d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Mar 12, 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/8300311688/npm-package-wrangler-5229

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8300311688/npm-package-wrangler-5229 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8300311688/npm-package-create-cloudflare-5229 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8300311688/npm-package-cloudflare-kv-asset-handler-5229
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8300311688/npm-package-miniflare-5229
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8300311688/npm-package-cloudflare-pages-shared-5229
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8300311688/npm-package-cloudflare-vitest-pool-workers-5229

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


wrangler@3.34.2 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240304.2
workerd 1.20240304.0 1.20240304.0
workerd --version 1.20240304.0 2024-03-04

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

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 6.15385% with 61 lines in your changes are missing coverage. Please review.

Project coverage is 71.27%. Comparing base (aabae0f) to head (024fd7d).
Report is 25 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5229      +/-   ##
==========================================
+ Coverage   70.47%   71.27%   +0.79%     
==========================================
  Files         298      311      +13     
  Lines       15539    16156     +617     
  Branches     3999     4121     +122     
==========================================
+ Hits        10951    11515     +564     
- Misses       4588     4641      +53     
Files Coverage Δ
...es/wrangler/src/api/integrations/platform/index.ts 20.63% <50.00%> (+4.76%) ⬆️
packages/wrangler/src/api/integrations/utils.ts 16.66% <16.66%> (ø)
...gler/src/api/integrations/platform/bindingNames.ts 1.81% <1.81%> (ø)

... and 30 files with indirect coverage changes

@alexanderniebuhr
Copy link
Contributor

@dario-piotrowicz thanks for letting me know of this PR.

Just want to give some insights from Astro's side. We would love to see this introduced to the API. We are currently building a DevToolbarApp, which will allow users to interact with the bindings directly from the browser without going back to the terminal, this is a significant improvement in DX.

Currently there is no way for us to know which binding is of which type. We would love to know which bindings are of a specific type.
We tried to workaround this and found some manually way for the user to type, manually reading the wrangler.toml or using the inspector check, mentioned above. - All of those are either not very bullet proof or require a lot of custom logic, which was the main reason for us to adopt getPlatformProxy to get rid of custom implementations.

We also believe this feature could be useful for other frameworks and users, even if they don't have it requested yet.

I'm happy to discuss this further in any of our dedicated communication channels, just let me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Untriaged
Development

Successfully merging this pull request may close these issues.

🚀 Feature Request [getBindingsProxy]: add a convenient way to get hold of bindings of a specific types
2 participants