Support Images binding in getPlatformProxy()#9954
Conversation
🦋 Changeset detectedLatest commit: cb01dcb The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
petebacondarwin
left a comment
There was a problem hiding this comment.
Interesting approach. Generally looks good. CI is busted right now though.
fixtures/get-platform-proxy/tests/get-platform-proxy.env.test.ts
Outdated
Show resolved
Hide resolved
|
Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the Depending on your changes, running Notes:
|
566b36d to
123b2c7
Compare
fixtures/get-platform-proxy/tests/get-platform-proxy.env.test.ts
Outdated
Show resolved
Hide resolved
| await fetch("https://playground.devprod.cloudflare.dev/flares.png") | ||
| ).body?.tee()!; | ||
|
|
||
| // @ts-expect-error The stream types aren't matching up properly? |
There was a problem hiding this comment.
| // @ts-expect-error The stream types aren't matching up properly? | |
| // @ts-expect-error `ReadableStream` between Node.js and Workers doesn't quite along (Property 'readAtLeast' is missing in type 'import("stream/web").ReadableStreamBYOBReader') |
This method is non-standard
What happens if the IMAGE binding actually starts to use this?
Should we have some kind of transform/polyfill for getPlatformProxy()?
There was a problem hiding this comment.
I think this will be fine at runtime, because by the time the ReadableStream gets to the Images binding it'll be a Workers Readable Stream. I'm not quite sure how we should fix this from a typing perspective though.
There was a problem hiding this comment.
Maybe a wrapper around the env type? e.g. getPlatformProxy() currently returns Env as-is, maybe it should be ConvertToNodeTypes<Env> or something?
Co-authored-by: Pete Bacon Darwin <pete@bacondarwin.com>
Adds support for the Images binding in
getPlatformProxy(). Images bindings are some of the most complex bindings from an API perspective, other than RPC. In particular, they expose a synchronous API that accepts ReadableStream arguments. Multiple synchronous APIs are chained together in a builder pattern (e.g.await env.IMAGES.input(stream).transform(...).output(...)) before the final.output()call is awaited. This breaks our assumptions around functions that accept ReadableStream arguments always being async, and so doesn't work without some special casing.In future, this could be replaced with a client JSRPC implementation (using promise pipelining), but for now special casing the entire binding seems to work well.