add remote bindings support to getPlatformProxy#9688
Conversation
🦋 Changeset detectedLatest commit: 86f2189 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
478eb47 to
42c2d23
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: |
| remoteProxyConnectionString, | ||
| remoteBindingsEnabled | ||
| ); | ||
| buildMiniflareBindingOptions(config, remoteProxyConnectionString); |
There was a problem hiding this comment.
This isn't quite right. "enabled" and "has a connection string" are subtly different—even without a connection string, "enabled" will affect things like Images local mode and the logged warnings
There was a problem hiding this comment.
no.... I don't think I quite follow... 😕
well since this change is quite irrelevant to getPlatformProxy I am also happy to revert this aspect of the PR and revisit this later 🤔
put back explicit `remoteBindingsEnabled`
replace @ts-ignore to the more appropriate @ts-expect-error
replace variable `a`
make `preExistingRemoteProxySessionData` param optional
set the process.env cloudflare id and token so that getPlatformProxy can inherit them
add helpful comment
refactor `getMiniflareOptionsFromConfig`
pass an args object to `getMiniflareOptionsFromConfig`
add example to changelog
penalosa
left a comment
There was a problem hiding this comment.
Some nits, but overall lgtm
| const { | ||
| rawConfig, | ||
| targetEnvironment, | ||
| options, | ||
| remoteProxyConnectionString, | ||
| remoteBindingsEnabled, | ||
| } = args; |
There was a problem hiding this comment.
nit, but this destructing seems a bit messy to me.
There was a problem hiding this comment.
I totally agree
I was simply passing the values as an object as requested, I am happy to either revert the change or proceed any way you see fit (like always passing args around?)
There was a problem hiding this comment.
I prefer the current version FWIW
There was a problem hiding this comment.
It's pretty much entirely a style choice, so I'll leave it up to your judgement @dario-piotrowicz, but fwiw I don't think it necessarily makes sense to change this function to accept an object of options. remoteBindingsEnabled is temporary while we're in the experimental phase, and IMO 4 parameters is a perfectly reasonable numbr of parameters to a function. Making it an options object adds this destructuring, which in my mind just means that there's an extra 7 lines of boilerplate obscuring what this function does. If we want to keep it as an options object, I would find either 1) using args.remoteProxyConnectionString etc... consistently throughout the function body or 2) destructuring in the function definition much clearer to read.
remove unnecessary `env` object
remove unnecessary intermediary variable
| const maybeRemoteProxySessionWrap = | ||
| await maybeStartOrUpdateRemoteProxySession(rawConfig.configPath); | ||
| remoteProxySession = maybeRemoteProxySessionWrap?.session; | ||
| remoteProxySession = ( |
There was a problem hiding this comment.
That look messy to me. IMO the former version looked better!
There was a problem hiding this comment.
and by that I also mean that you do not have to implement all comments if they don't make sense to you - even my comments :)
There was a problem hiding this comment.
Yeah, this wasn't quite what I was suggestion. I was thinking of (await maybeStartOrUpdateRemoteProxySession(config.configPath))?. session.
That said, this was mostly because the intermediate variable didn't add much clarity. What about:
({session: remoteProxySession } = await maybeStartOrUpdateRemoteProxySession(config.configPath)
…seba/containers_scope_debug * 'main' of ssh://github.com/cloudflare/workers-sdk: Version Packages (cloudflare#9697) add remote bindings support to `getPlatformProxy` (cloudflare#9688) feat(containers): add support for handling images that link to the CF registry (cloudflare#9596) CC-5418: Set instance_type in wrangler (cloudflare#9633) remove warnings during config validations on `experimental_remote` fields (cloudflare#9678) add debug logs for workerd (cloudflare#9640) `wrangler containers apply` uses `observability` configuration (cloudflare#9558) Version Packages (cloudflare#9658) Temporarily skip Openapi C3 e2e tests (cloudflare#9691) Skip authed fixture on forks (cloudflare#9681)
…seba/containers_scope * 'main' of ssh://github.com/cloudflare/workers-sdk: Add CLAUDE.md for Claude Code guidance (cloudflare#9563) Version Packages (cloudflare#9697) add remote bindings support to `getPlatformProxy` (cloudflare#9688) feat(containers): add support for handling images that link to the CF registry (cloudflare#9596) CC-5418: Set instance_type in wrangler (cloudflare#9633) remove warnings during config validations on `experimental_remote` fields (cloudflare#9678) add debug logs for workerd (cloudflare#9640) `wrangler containers apply` uses `observability` configuration (cloudflare#9558) Version Packages (cloudflare#9658) Temporarily skip Openapi C3 e2e tests (cloudflare#9691) Skip authed fixture on forks (cloudflare#9681)
Fixes https://jira.cfdata.org/browse/DEVX-2028
add remote bindings support to
getPlatformProxyremoteBindingsoption togetPlatformProxydocs cloudflare-docs#23136