-
Notifications
You must be signed in to change notification settings - Fork 1k
add remote bindings support to getPlatformProxy
#9688
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits, but overall lgtm
| const { | ||
| rawConfig, | ||
| targetEnvironment, | ||
| options, | ||
| remoteProxyConnectionString, | ||
| remoteBindingsEnabled, | ||
| } = args; |
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.
nit, but this destructing seems a bit messy to me.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the current version FWIW
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That look messy to me. IMO the former version looked better!
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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