-
Notifications
You must be signed in to change notification settings - Fork 109
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://c0ef23f4-alchemy-website.alchemy-run.workers.dev This preview was built from commit de7760f 🤖 This comment will be updated automatically when you push new commits to this PR. |
alchemy/src/cloudflare/bucket.ts
Outdated
| 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?
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 usable, just need to report it or figure out what's going on
|
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
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.
You were right... there are bugs that the test is revealing. This might take a little longer 😅
|
Really looking forward to this one. We've got a fairly decent amount of typing shenanigans to make Alchemy binding types play nice with |
|
Just gave this one a shot locally. When using Fwiw our tsconfig is setup for strict. |
| console.log("8. list objects"); | ||
| const listObj = await bucket.list(); | ||
| console.log("9. list objects", listObj); |
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.
Found that the scope was finalizing itself around here, which causes this error to be thrown:
Error: Attempted to use poisoned stub. Stubs to runtime objects must be re-created after calling `Miniflare#setOptions()` or `Miniflare#dispose()`.
❯ ProxyStubHandler.#assertSafe node_modules/.bun/miniflare@4.20251011.1+a744fd195f22f642/node_modules/miniflare/src/plugins/core/proxy/client.ts:370:10
❯ ProxyStubHandler.get node_modules/.bun/miniflare@4.20251011.1+a744fd195f22f642/node_modules/miniflare/src/plugins/core/proxy/client.ts:4
Tempted to abandon ship on this PR... too many footguns
| try { | ||
| let database = await D1Database(id); | ||
| expect(database.id).toBeTruthy(); | ||
| const session = database.withSession("first-primary"); |
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.
Should we add tests that also use .exec and .prepare? These tests only work on sessions
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.