-
Notifications
You must be signed in to change notification settings - Fork 1k
stop getPlatformProxy crashing with internal DOs #8697
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: e9636eb 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 |
|
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14174103654/npm-package-wrangler-8697You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/8697/npm-package-wrangler-8697Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14174103654/npm-package-wrangler-8697 dev path/to/script.jsAdditional artifacts:cloudflare-workers-bindings-extension: wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14174103654/npm-package-cloudflare-workers-bindings-extension-8697 -O ./cloudflare-workers-bindings-extension.0.0.0-vb21be6f24.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-vb21be6f24.vsixcreate-cloudflare: npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14174103654/npm-package-create-cloudflare-8697 --no-auto-update@cloudflare/kv-asset-handler: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14174103654/npm-package-cloudflare-kv-asset-handler-8697miniflare: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14174103654/npm-package-miniflare-8697@cloudflare/pages-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14174103654/npm-package-cloudflare-pages-shared-8697@cloudflare/unenv-preset: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14174103654/npm-package-cloudflare-unenv-preset-8697@cloudflare/vite-plugin: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14174103654/npm-package-cloudflare-vite-plugin-8697@cloudflare/vitest-pool-workers: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14174103654/npm-package-cloudflare-vitest-pool-workers-8697@cloudflare/workers-editor-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14174103654/npm-package-cloudflare-workers-editor-shared-8697@cloudflare/workers-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14174103654/npm-package-cloudflare-workers-shared-8697@cloudflare/workflows-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14174103654/npm-package-cloudflare-workflows-shared-8697Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
fixtures/get-platform-proxy/tests/get-platform-proxy.env.test.ts
Outdated
Show resolved
Hide resolved
| ${localBindings.map((b) => `- ${JSON.stringify(b)}`).join("\n")} | ||
| These will not work in local development, but they should work in production. | ||
| If you want to develop these locally, you can define your DO "externally" in another worker. |
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.
Defining a DO "in a worker" is confusing (at least to me)
| If you want to develop these locally, you can define your DO "externally" in another worker. | |
| If you want to develop these locally, you can define your DO "externally" in another wrangler configuration. |
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.
How about 👇
If you want to develop these locally, you can define your DO in a separate Worker, specified by a separate configuration file.
tho it is not clear what the recommended thing to do would be when multiple DOs are defined. I'm assuming each DO in a separate Worker?
dario-piotrowicz
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.
Looks good to me 😄
|
nvm the warning message is getting ridiculously long - i'm going to create a section in the docs to link to |
There's already some content for the DO issue here: https://developers.cloudflare.com/workers/wrangler/api/#supported-bindings 🙂 |
|
docs PR: (sorry @dario-piotrowicz I didn't see your comment until after I finished 😅 ) |
vicb
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.
Thanks a lot for fixing this @emily-shen 🙏
01078de to
950fbfc
Compare
950fbfc to
d32ae37
Compare
|
pending docs review and landing first, since this PR references the newly added section |
|
docs PR was merged. This is no longer blocked and I will proceed to merge it |
…rnal DOs (#8713) * pass name to getplatformproxy * fixup * remove out of date warning * stop gpp crashing with internal DOs * changeset * fix e2e * pr feedback * elaborate on error message * move to a docs link * fix failing test * copy improvement * fix * update docs link * update link --------- Co-authored-by: emily-shen <[email protected]>
this sets the name of the getPlatformProxy 'worker' to the provided worker name, so miniflare stops crashing when the proxy DO worker tries to bind to it.
Internal DOs still don't work, but at least it doesn't crash now? And it has a be
Fixes #8687
getPlatformProxyfrom crashing with internal DOs #8713