-
Notifications
You must be signed in to change notification settings - Fork 100
feat(cloudflare): node bindings for kv, d1, r2, and queue #1255
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
commit: |
🚀 Website Preview DeployedYour website preview is ready! Preview URL: https://611c68fb-alchemy-website.alchemy-run.workers.dev This preview was built from commit b2e805c 🤖 This comment will be updated automatically when you push new commits to this PR. |
| @@ -1,4 +1,3 @@ | |||
| import type { R2PutOptions } from "@cloudflare/workers-types/experimental/index.ts"; | |||
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.
Removed because it was causing a type error. Now it relies on the global from @cloudflare/workers-types.
| resumeMultipartUpload: (promise) => (key: string, uploadId: string) => | ||
| makeAsyncProxy( | ||
| { key, uploadId }, | ||
| promise.then((bucket) => bucket.resumeMultipartUpload(key, uploadId)), | ||
| { | ||
| uploadPart: true, | ||
| abort: true, | ||
| complete: true, | ||
| }, | ||
| ), |
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.
resumeMultipartUpload does not return a promise. To match the type of the runtime API, we make this synchronous by returning another proxy.
This is also done for D1 prepared statements and sessions.
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.
Can you show me an example of what the proxy fixes? Are all the properties Promise<T> or functions returning Promises?
| // TODO(john): this is a problem with @cloudflare/workers-types, it should not be nullable unless options.onlyIf is used | ||
| let putObj = (await bucket.put(testKey, testContent)) as R2Object; |
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.
Don't merge this PR until I report this to the CF team - this is a weird one
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.
Why not merge it? Is it unusable?
|
Node bindings doesn't make sense to me. These are just APIs |
| withSession: (promise) => (constraintOrBookmark) => | ||
| makeAsyncProxy( | ||
| {}, | ||
| promise.then((database) => | ||
| database.withSession(constraintOrBookmark), | ||
| ), | ||
| { | ||
| prepare: (session) => (query) => | ||
| makePreparedStatementProxy( | ||
| session.then((session) => session.prepare(query)), | ||
| ), | ||
| batch: true, | ||
| getBookmark: () => () => { | ||
| throw new Error( | ||
| "D1DatabaseSession.getBookmark is not implemented", | ||
| ); | ||
| }, | ||
| }, | ||
| ), |
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.
We will need to add some tests for each of these APIs on all the resources given how much custom logic there is. Please add tests
|
Really looking forward to this one. We've got a fairly decent amount of typing shenanigans to make Alchemy binding types play nice with |
| @@ -1,10 +1,8 @@ | |||
| import type { R2PutOptions } from "@cloudflare/workers-types/experimental/index.ts"; | |||
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.
The R2PutOptions type from experimental is identical to the non-experimental type. I'm not sure the experimental type is needed any longer.
|
Just gave this one a shot locally. When using Fwiw our tsconfig is setup for strict. |
Provides access to runtime APIs for KV, D1, R2, and Queue resources. Uses a proxy to lazily initialize a Miniflare instance with either the remote binding or the local one.
Not sure if "node binding" is the right way to describe this - let me know if you have any other ideas.