-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(wrangler): Add Media binding support #10867
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
Conversation
🦋 Changeset detectedLatest commit: 1daabfc The changes in this PR will be included in the next version bump. 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 |
6f1e934
to
3ac5c00
Compare
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: |
@penalosa on Monday would you mind giving this another review? I fixed everything from your comments on the last PR |
d0d671b
to
324dd74
Compare
|
||
const mf = new Miniflare({ | ||
compatibilityDate: "2025-01-01", | ||
compatibilityDate: "2025-09-06", |
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.
I assume that this is when the media binding became available in production?
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.
Correct
} | ||
|
||
if (bindings.media && remoteBindingsEnabled) { | ||
warnOrError("media", bindings.media.remote, "always-remote"); |
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 always-remote
throw an error if the user has specified remote:false
explicitly?
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.
IMO yeah it should, but I'm hesitant to make a change here since this matches what the AI/MTLS/etc bindings do
(getFlag("REMOTE_BINDINGS") && media.remote === true) || | ||
media.remote === undefined | ||
? false | ||
: undefined, |
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.
Oh I see. undefined
here means "not supported".
I wonder if we could be more explicit with a symbol or string or something?
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.
Pressed submit too soon.
I meant to say "approved with nits".
324dd74
to
fb9c0a8
Compare
fb9c0a8
to
1daabfc
Compare
This PR introduces wrangler support for the upcoming the Media binding.